Simplify dev - #11
Merged
Merged
Conversation
…, hoist invariant loop work - tests/downsampling/utils.py: parameterize assert_stability_table with expected_bootstraps, and add a shared run_downsample_clusters_rule() helper - test_downsample_clusters_rule.py / test_downsample_scaling.py: use the new shared helpers instead of duplicating the Snakemake-invocation and stability-table-assertion logic - downsample_clusters.R: compute vars_to_regress once and call SCTransform a single time instead of duplicating the call in an if/else; hoist the per-replicate-invariant `dat` tibble out of GetJaccardMaxByCluster's cluster loop - make_medium_fixture.R: jitter only the sparse matrix's nonzero values in place instead of densifying the whole count matrix; drop a PercentageFeatureSet() data.frame check that can't fail; regenerate the medium fixture with the fixed generator Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…fixture generator The prior simplify pass assumed PercentageFeatureSet() always returns a data.frame and replaced a defensive is.data.frame() check with an unconditional [[1]]. Confirmed by direct execution that this repo's installed Seurat (5.5.0) returns a plain numeric vector instead, so [[1]] extracted only the first cell's value and R silently broadcast it across every cell's percent.mt. Masked in this fixture only because it has no genes matching "^MT-" (percent.mt is 0 either way). Restored the is.data.frame() branch. Also, switching the jitter step to only perturb the sparse matrix's nonzero (@x) entries meant cells resampled more than once from the same source cell had a non-negligible chance (P(X=0)~=0.98 per jittered entry) of staying byte-identical, reintroducing exact-duplicate cells the jitter exists to avoid (measured: 163/2500 cells, 6.5%). Added explicit de-duplication that bumps one entry per repeat draw of the same source index, independent of jitter's randomness. Regenerated fixture verified to have 0 duplicate cells (down from 163) and 0 all-zero genes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…rize the bump Code review found the prior de-dup loop's bump row `((k-1) %% nrow)+1` was keyed on a global repeat-draw counter and, for the real 300->2500 fixture, wraps (2200 repeat draws > 2000 genes) so rows 1..200 are reused. Two repeat draws of the same source cell whose positions differed by exactly nrow could then get bumped at the same row and, if their jitter also coincided, stay byte-identical -- the loop never re-checked, and the "guarantee distinctness" comment overstated a heuristic. The shipped fixture happened to be duplicate- free at seed 12345, but a different seed/size could silently reintroduce the duplicate cells this logic exists to prevent. Key the bump row on the per-source occurrence rank instead (Nth draw of a source bumped at row N-1), which is injective within each source group and bounded by the max multiplicity (17 here) so it never wraps; assert it stays under the gene count. Replace the per-element sparse [<- loop (O(repeats x nnz) dgCMatrix reallocations) with a single vectorized matrix-index assignment, and correct the comment to describe best-effort separation rather than a guarantee. Regenerated fixture verified: 0 duplicate cells, occurrence ranks peak at 17 (<< 2000 genes, no wrap). Full downsampling suite (rule + workflow + scaling) passes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
code review simplification and bug detection