Skip to content

ruff is clean at both scopes — and three of the reports were real - #71

Merged
shanikawm merged 1 commit into
mainfrom
chore/ruff-clean
Jul 26, 2026
Merged

ruff is clean at both scopes — and three of the reports were real#71
shanikawm merged 1 commit into
mainfrom
chore/ruff-clean

Conversation

@shanikawm

Copy link
Copy Markdown
Contributor

ruff check shanuz 51 → 0, ruff check . 201 → 0.

Most of it is what it looks like: unused imports left behind by the Assay5 refactor, dead locals, semicolon-joined statements. But three reports were pointing at something real, and those are the reason this is worth reading.

A test had been shadowed and never ran

test_anchors_seurat_parity.py defined test_pca_loadings_are_exact_not_randomized at line 275 and again at line 422 (F811). Python keeps the second, so the first was collected by nothing.

It is not a duplicate. The surviving copy checks _pca_loadings against np.linalg.svd and asserts orthonormality and seed-independence. The shadowed one additionally builds sklearn's randomized result and asserts it disagrees — the guard that the exact SVD is load-bearing rather than an equivalent spelling:

assert rand_pc.min() < 0.9, (
    "if randomized SVD agreed on every PC the exact solver would not matter"
)

That is the assertion behind the 45% → 100% RPCA anchor-recall fix, and it had not executed since it was written. Renamed to test_randomized_svd_would_visibly_disagree_on_the_trailing_pcs; it passes.

A regress-out test with no baseline

test_sctransform_vars_to_regress_removes_covariate_effect computed before (F841) and never asserted on it — and it was the wrong baseline regardless, taken from a different object built from un-injected counts. What remained was:

assert np.nanmean(corr_after) < 0.05

which is equally satisfied by a fixture that never carried the covariate in the first place. Rewritten to run SCTransform twice over the same injected counts, once plain and once regressing, and to require the signal to be present before requiring its removal: unregressed 0.2526, regressed 0.0000.

Verified the new half is load-bearing by zeroing the injection — corr_before drops to 0.073 and the test now fails there, where the old one passed.

find_markers named one missing ident but not the other

ident_1 empty said which ident; ident_2 empty raised a constant "No cells found for comparison group." (F541). Both name it now, and since ident_2=None means "every other ident", that case reports there is nothing left outside ident_1 instead of a parameter the caller never passed. Two new tests — the path had no coverage at all.

The rest

Dead locals worth naming: two computed-and-discarded colour maps in vln_plot and dim_plot, and a v3 layout flag in read_10x that no branch consumed.

The 121 E402s in tests/ and tutorials/ are all one deliberate line — the sys.path bootstrap those files need to run straight from a clone. Ignored by scope in pyproject.toml rather than by 121 scattered # noqa: E402.

Verification

before after
ruff check shanuz 51 0
ruff check . 201 0
suite 915 / 25 skipped 918 / 25 skipped
mypy shanuz 0 0

+1 from the un-shadowed test, +2 new. The pbmc3k tutorial was re-run end to end and its --report against Seurat 5.5.1 is unchanged (2,638 barcodes, 2,000 HVGs, kNN 52,760, 8 vs 9 clusters). 5 mutations, 5 killed.

One thing this does not fix

CI installs ruff unpinned and currently resolves 0.16.0, whose default rule set is much larger — uvx ruff@0.16.0 check shanuz reports 644, dominated by UP045/UP037/UP007 (Optional[X]X | None, ~500 sites) and I001 import sorting. Everything above is measured against the pinned local 0.15.20. Closing that gap is a separate decision: pin ruff in CI, or take the annotation rewrite.

🤖 Generated with Claude Code

`ruff check shanuz` and `ruff check .` both report zero, down from 51 and
201. Most of it is cleanup — unused imports left behind by the Assay5
refactor, dead locals, semicolon-joined statements — but three of the
reports were pointing at something real.

`test_anchors_seurat_parity.py` defined
test_pca_loadings_are_exact_not_randomized twice (F811). Python keeps the
second, so the first never ran. It is not a duplicate: it also proves
sklearn's randomized solver *disagrees* on the trailing PCs, which is the
whole reason the exact SVD is there. Renamed; it passes.

test_sctransform_vars_to_regress_removes_covariate_effect computed a
`before` baseline (F841) and never asserted on it — and it was the wrong
baseline anyway, taken from a different object built from un-injected
counts. Asserting only that the regressed residuals are uncorrelated with
the covariate is equally satisfied by a fixture that never carried the
covariate. Rewritten to run SCTransform twice over the same injected
counts: unregressed 0.25, regressed 0.00. Verified load-bearing by
zeroing the injection — the new assertion fires at 0.07 where the old
test passed.

find_markers raised a constant string for an empty ident_2 (F541) while
the ident_1 branch named the ident it looked for. Both name it now, and
ident_2=None reports that nothing is left outside ident_1 rather than
naming a parameter the caller never passed. Two new tests; that path had
no coverage at all.

The 121 E402s in tests/ and tutorials/ are the deliberate sys.path
bootstrap those files need to run from a clone. Ignored by scope in
pyproject.toml rather than by 121 scattered noqa comments.

Suite 915 → 918: +1 from the un-shadowed test, +2 new. mypy stays at 0.
pbmc3k tutorial re-run end to end and its R comparison is unchanged.
5 mutations, 5 killed.

Note CI installs ruff *unpinned* and currently resolves 0.16.0, whose
default rule set is much larger (PEP 604 annotations, import sorting):
644 on `shanuz`. Everything here is measured against the pinned local
0.15.20. Closing that gap is a separate call — pin CI, or take the
~500-site annotation rewrite.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@shanikawm
shanikawm merged commit 3780e51 into main Jul 26, 2026
3 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