fix: only warn from log1p when the target representation was already transformed - #4298
Draft
Mohit-Ak wants to merge 2 commits into
Draft
fix: only warn from log1p when the target representation was already transformed#4298Mohit-Ak wants to merge 2 commits into
Mohit-Ak wants to merge 2 commits into
Conversation
…formed sc.pp.log1p recorded its state in adata.uns["log1p"] without noting which representation it transformed, and the guard only tested for the key's presence. Transforming X and then a layer therefore warned that "adata.X seems to be already log-transformed" even though X was touched once, while genuinely transforming the same layer twice never warned. Record the transformed representations in uns["log1p"]["reps"] and check membership against the representation the current call targets. State written by older scanpy versions has no "reps" key and is read as ["X"], which is what it could only have meant. check_rep_results() gains an ignore_uns argument so representation-dependent uns state can be excluded from its cross-representation equality check.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4298 +/- ##
=======================================
Coverage ? 79.30%
=======================================
Files ? 128
Lines ? 13488
Branches ? 0
=======================================
Hits ? 10697
Misses ? 2791
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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
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.
sc.pp.log1precords that it ran inadata.uns["log1p"], and the "already log-transformed" guard only checks whether that key exists. Since the key carries no information about which representation was transformed, the warning is decoupled from what the call actually touches:X was transformed exactly once, so both warnings are false positives.
While reproducing this I found the same root cause also produces the opposite failure, which the issue doesn't mention: because the state is never per-representation, transforming the same layer twice — a real double-log — stays silent. So the guard is currently both too noisy and too quiet.
Behaviour of every combination I could construct, before and after:
X→layer="spliced"layer="spliced"→Xlayer="spliced"→layer="unspliced"X→obsm="rep"X→Xlayer="spliced"twiceobsm="rep"twiceThe fix
uns["log1p"]now also records which representations have been transformed, and the guard tests membership against the representation the current call targets rather than mere key presence:basekeeps its existing meaning and position, so the readers of that key are unaffected. The warning also now names the representation it's actually about (adata.layers['spliced'] seems to be already log-transformed.) instead of always sayingadata.X.On backwards compatibility: state written by earlier versions is a bare
{"base": ...}with no"reps". That's read as["X"]— beforelayer=/obsm=were tracked at all,Xis the only thing the marker could have meant — so an.h5adwritten by an older scanpy still warns on a repeatlog1p(adata)and doesn't spuriously warn on a first-time layer transform. There's a test for this. I also checked an h5ad round-trip:repscomes back as an object ndarray rather than a list, which the membership test handles.A test-helper change worth flagging
check_rep_results()asserts that processing X, a layer and an obsm leaves identical AnnData objects, andunsis part of that comparison. Recording which representation was processed makes those objects legitimately differ, sotest_log1p_repfailed (18 parametrisations) until I gave the helper a way to exclude that path:ignore_unstakes"key"or"key/subkey"paths and defaults to empty, so every other caller is unchanged. This is a narrow version of the# TODO: Allow specifying paths to ignore on comparisonalready noted at the top of that module. Note the assertion is still meaningful — only the deliberately representation-dependent sub-key is skipped, sobaseequality is still enforced across representations. If you'd rather this helper stayed untouched and the state lived somewherecheck_rep_resultsdoesn't compare, say so and I'll rework it.Testing
Added
test_log1p_warns_per_representation(7 parametrised call sequences, the table above) andtest_log1p_legacy_uns_state_implies_x.To confirm the new tests actually pin the behaviour, I reverted only
_simple.pyand kept the tests:The 5 failures are exactly the false-positive rows plus the legacy-state case; the same-rep-twice rows pass before and after, since those were already correct.
Wider run, to check nothing else depends on the shape of that
unsentry (grepsaysbaseis the only key read anywhere else):ruff checkandruff format --checkclean on the four touched files at the pinned v0.16.1.Fixes #4285