Repository navigation
fix: have ingest no longer truncate to settings.N_PCS - #4401
Conversation
When pp.neighbors runs with its own n_pcs=None default, it uses every stored PCA component unsliced (_get_pca_or_small_x). Ingest's fallback branch for "no use_rep/n_pcs recorded" instead hardcoded settings.N_PCS (50), silently using a different, truncated representation than the one that actually produced the reference neighbor graph whenever more than 50 PCs were computed (e.g. sc.pp.pca(adata, n_comps=100) followed by sc.pp.neighbors(adata)). Fixes it by using the full stored X_pca, matching pp.neighbors' actual default behavior instead of assuming it always caps at settings.N_PCS.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4401 +/- ##
=======================================
Coverage 84.37% 84.37%
=======================================
Files 134 134
Lines 12752 12752
=======================================
Hits 10760 10760
Misses 1992 1992
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Checking back in on this one — still green and mergeable, just waiting on the milestone/label gate (I don't have permission to set that myself). Happy to make any further changes if needed. |
|
sure! no need to add a test for somthing this small, but a release note would make sense! |
Signed-off-by: Siddhardha Nanda <99672439+SID-6921@users.noreply.github.com>
|
Added a release note fragment (docs/release-notes/4401.fix.md). Let me know if the wording needs adjusting. |
Documentation build overview
277 files changed ·
|
…te to settings.N_PCS) (#4474) Co-authored-by: Siddhardha Nanda <99672439+SID-6921@users.noreply.github.com>
Ingestfalls back to a hardcodedsettings.N_PCS(50) whenever the referenceadata'sneighborsparams recorded neitheruse_repnorn_pcs— but that's exactly the statepp.neighborsleaves when called with its own defaults, and its default (n_pcs=None) uses the full stored PCA representation unsliced (_get_pca_or_small_x), not 50 components. Whenever more than 50 PCs were computed (e.g.sc.pp.pca(adata, n_comps=100)thensc.pp.neighbors(adata)),Ingestsilently builds its KNN index and transforms new data in a different, truncated representation than the one that actually produced the reference neighbor graph — no error, just a quietly wrong embedding/label mapping forsc.tl.ingest.Found via sibling comparison between
Ingest._init_neighbors's three branches and_get_pca_or_small_x(the function that actually computespp.neighbors' representation) — the first two branches already match the stored params correctly; only the fallback diverged.Repro
AI assistance
Ingest's three_init_neighborsbranches against_get_pca_or_small_x's actual default behavior, then wrote the fix and regression testpbmc68k_reducedtest fixture hits an unrelated zarr-version incompatibility in this environment, so the new test couldn't be run through the full suite here — logic was verified directly against the samesc.pp.pca/sc.pp.neighbors/sc.tl.Ingestcalls the test uses)Test results
Added
test_representation_more_than_n_pcs_defaulttotests/test_ingest.py, coveringn_comps = settings.N_PCS + 20. Manually verified the underlying fix acrossn_compsof 30, 50, and 100 against a syntheticAnnData(below, below+at, and above thesettings.N_PCSthreshold) — all three now correctly match the PCspp.neighborsactually used, whereas 100 previously and incorrectly truncated to 50.