Skip to content

Declare cell_type, and split by batch when is_train is missing - #28

Merged
rcannood merged 1 commit into
mainfrom
fix/declare-is-train-cell-type
Jul 29, 2026
Merged

Declare cell_type, and split by batch when is_train is missing#28
rcannood merged 1 commit into
mainfrom
fix/declare-is-train-cell-type

Conversation

@rcannood

@rcannood rcannood commented Jul 28, 2026

Copy link
Copy Markdown
Member

Describe your changes

process_dataset reads two obs columns that the API never declares:

is_train <- which(ad1$obs[["is_train"]] == "train")
is_test  <- which(!ad1$obs[["is_train"]] == "train")
...
ct <- as.character(ad1$obs[["cell_type"]][is_test])

process_datasets/main.nf validates its inputs against file_common_dataset_mod{1,2}.yaml and drops anything that fails -- so a dataset that honours the published contract but happens to lack is_train sails through QC. In R, which(NULL == "train") is integer(0), so both index sets come back empty and we write four zero-row objects without complaining.

Reproduced on bmmc_cite with obs["is_train"] removed:

Creating train/test split
Create train objects
Create test objects
Saving output files as h5ad
exit: 0
train_mod1 -> (0, 134)
train_mod2 -> (0, 134)
test_mod1  -> (0, 134)
test_mod2  -> (0, 134)

is_train is optional

I originally declared it required, but that's wrong: obs["is_train"] is the NeurIPS 2021 competition's own split, and there's no reason another dataset would carry it. Declared optional, with a fallback that holds out a quarter of the batches -- keeping the spirit of the competition split, where test cells come from donors the method hasn't seen -- or a quarter of the cells when there's only one batch. It's seeded off --seed, so it's reproducible.

All three paths, on bmmc_cite:

=== with_is_train
Creating train/test split
Train cells: 431, test cells: 163
=== no_is_train
No obs['is_train'], holding out batches: s1d3, s4d8, s3d7
Train cells: 446, test cells: 148
=== one_batch
No obs['is_train'] and only one batch, holding out a quarter of the cells
Train cells: 446, test cells: 148

(the two fallbacks landing on 148 is a coincidence of floor(594/4); they select different cells, and no path leaks a cell between train and test.)

cell_type is required

Declared and enforced up front, so a missing annotation fails immediately rather than a few hundred lines later inside the subsampling branch.

Also added a line logging the split sizes -- "how many cells ended up on each side" was previously invisible in the logs.

Part of a series of PRs coming out of a pre-run review of the benchmark.

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 rcannood mentioned this pull request Jul 29, 2026
8 tasks
obs['is_train'] carries the NeurIPS 2021 competition split, so it is
declared optional -- but process_dataset used it without checking, and a
dataset without it produced four empty h5ads and exit 0. Fall back to
holding out a quarter of the batches, or a quarter of the cells if there
is only one batch.

obs['cell_type'] is declared required, since the test subsample is
weighted by it.
@rcannood
rcannood force-pushed the fix/declare-is-train-cell-type branch from e049307 to 4e57a79 Compare July 29, 2026 13:19
@rcannood rcannood changed the title Declare is_train and cell_type in the dataset API Declare cell_type, and split by batch when is_train is missing Jul 29, 2026
@rcannood
rcannood merged commit 6276cbc into main Jul 29, 2026
2 checks passed
@rcannood
rcannood deleted the fix/declare-is-train-cell-type branch July 29, 2026 13:27
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