Skip to content

Fix guanlab_dengkw_pm - #32

Merged
rcannood merged 3 commits into
mainfrom
fix/guanlab-dead-else-branch
Jul 30, 2026
Merged

Fix guanlab_dengkw_pm#32
rcannood merged 3 commits into
mainfrom
fix/guanlab-dead-else-branch

Conversation

@rcannood

@rcannood rcannood commented Jul 29, 2026

Copy link
Copy Markdown
Member

Describe your changes

Three problems in guanlab_dengkw_pm, checked against the original submission.

1. The consensus scheme had been replaced. The original averages ten kernel ridge models -- five reshuffles of the batches into two halves, seeded with 1000:

np.random.seed(1000)
for _ in range(5):
    np.random.shuffle(batches)
    for batch in [batches[:batch_len//2], batches[batch_len//2:]]:
        ...
y_pred /= 10

The port did something else entirely: one fixed two-way split when either modality is ADT, leave-one-batch-out otherwise, with no reshuffling and no seed. So the result depended on the order the batches happened to arrive in, and for ADT pairs it averaged two models where the original averaged ten. Restored the original, with --n_repeats and --seed as arguments.

Effect on the test resources -- 23 of 32 metric/dataset combinations improve:

dataset metric old new delta
bmmc_multiome/swap overall_pearson 0.3474 0.3648 +0.0175
bmmc_multiome/swap mean_pearson_per_cell 0.3416 0.3590 +0.0173
bmmc_multiome/swap rmse 1.0748 1.0555 -0.0194
bmmc_multiome/swap mae 0.5507 0.5400 -0.0107
bmmc_cite/swap (all 8) all improve
bmmc_cite/normal (6 of 8) small
bmmc_multiome/normal (7 of 8) small regressions, mae -0.0100

bmmc_multiome/normal is the one that gets slightly worse, which makes sense -- that is the pair where the old code's leave-one-batch-out trained each model on more batches. But the point is fidelity to the published method rather than picking whichever variant scores best, and the differences here are muted anyway: these fixtures have few batches, while the real datasets have nine or more, where "half the batches" and "all but one batch" differ a lot more.

2. embedder_mod2 is defined conditionally but used unconditionally.

if n_mod2 is not None and n_mod2 < input_train_mod2.n_vars:
    embedder_mod2 = TruncatedSVD(n_components=n_mod2)
    train_gs = embedder_mod2.fit_transform(...)
else:
    train_gs = input_train_mod2.to_df(layer="normalized").values...   # no embedder_mod2
...
    y_pred += (krr.predict(test_norm) @ embedder_mod2.components_)    # NameError

Present in the original too. The else fires when the target has <= n_mod2 features (50 or 70), which no dataset we currently ship does -- ADT is 134, GEX ~14k, ATAC 10k -- so it has never been hit. And the re-mapping is wrong there anyway: in that branch the KRR already predicts in the target space. Now tracked with embedder_mod2 = None and the re-mapping skipped:

n_mod2=5,  n_vars=30, svd=yes -> y_pred (10, 30)
n_mod2=50, n_vars=30, svd=no  -> y_pred (10, 30)

3. n_comp_dict covers only the four NeurIPS pairs. Anything else is a bare KeyError, which reads as "the method is broken" rather than "the method doesn't apply here". Switched to exit_non_applicable() (exit 99), which also means adding src/utils/exit_codes.py, matching task_batch_integration.

Also dropped --distance_method and --n_pcs, which were declared but never read.

One thing deliberately not ported

The original clips ATAC predictions to [0, 1]:

if mod2_type == "ATAC":
    np.clip(y_pred, a_min=0, a_max=1, out=y_pred)

That assumed the starter kit's binarised ATAC. Here the ATAC target is log_cp10k, reaching 8.8 with 2.9% of values above 1, so the clip would cap most of the signal. Left out.

All four pairs run green:

bmmc_cite/normal      GEX->ADT   EXIT=0  5s  10 fits
bmmc_cite/swap        ADT->GEX   EXIT=0  6s  10 fits
bmmc_multiome/normal  GEX->ATAC  EXIT=0  7s  10 fits
bmmc_multiome/swap    ATAC->GEX  EXIT=0  7s  10 fits

Checklist before requesting a review

  • I have performed a self-review of my code

  • Check the correct box. Does this PR contain:

    • Breaking changes
    • New functionality
    • Major changes
    • Minor changes
    • Bug fixes
  • Proposed changes are described in the CHANGELOG.md

  • CI Tests succeed and look good!

rcannood added 3 commits July 29, 2026 09:29
* Only apply the mod2 SVD re-mapping when that SVD was fitted
* Exit 99 for modality pairs the method has no hyperparameters for
* Add src/utils/exit_codes.py

embedder_mod2 was only defined inside the if, but used unconditionally,
so a target with fewer features than n_mod2 hit a NameError.
The port fitted one model per left-out batch (or a single fixed two-way split for
ADT), rather than the original's five reshuffles into halves. Results depended on
batch ordering and no seed was set.

* Average ten models over five reshuffled two-way splits, as in submission_170636
* Expose `--n_repeats` and `--seed`; drop the unused `--distance_method`/`--n_pcs`
@rcannood rcannood changed the title Fix the dead else branch in guanlab_dengkw_pm Fix guanlab_dengkw_pm Jul 30, 2026
@rcannood
rcannood merged commit 221a029 into main Jul 30, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant