Fix VCSCAnnData.copy() silently dropping X/raw_X - #48
Merged
Conversation
VCSCAnnData stores X/raw_X in private _vcs_X/_vcs_raw_X attributes (anndata's own X validation rejects a VCSCArray/VCSRArray directly), but never overrode copy(), so the inherited anndata.AnnData.copy() copies the standard (unused, always-None) _X attribute instead: X silently comes back None, and the returned object is downgraded to a plain AnnData rather than preserving this class (or a subclass, such as one overriding the X property for a lazy-normalized view). Any caller relying on `adata[mask].copy()` -- e.g. parafac2's BiCV train/test splitting -- hits this immediately. Add an explicit override that copies every field, including a real VCSCArray/VCSRArray copy of X/raw_X, and returns type(self). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The inherited anndata.AnnData.to_memory() has the same root cause as copy() (previous commit): it only knows about the standard, unused _X attribute, not this class's _vcs_X/_vcs_raw_X, and reconstructs a plain AnnData that silently loses X. This class never actually supports a lazily backed X/raw_X, so to_memory() now just delegates to copy(). scrise's BiCV rank selection calls to_memory() on its input before the train/test split loop, so this was hit immediately after fixing copy(). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
__getitem__ already returned a new, eagerly-copied object (rather than a lazy view) since anndata's own view machinery can't handle _vcs_X, but it hardcoded the returned type to VCSCAnnData instead of type(self) -- so a subclass overriding X (e.g. one that always hands back a value normalized fresh from _vcs_X, as with copy()/to_memory() in the previous two commits) silently reverted to the raw, un-normalized array after any slice. scrise's BiCV rank selection slices its input for every train/test split, so this surfaced immediately after fixing copy()/to_memory(): the sliced object's X was the raw VCSRArray, which doesn't implement the norm_sq() a normalized view needs. 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
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.
Summary
VCSCAnnDatastoresX/raw_Xin private_vcs_X/_vcs_raw_Xattributes,since anndata's own
Xvalidation rejects aVCSCArray/VCSRArraydirectly(see the class docstring). It never overrides
.copy(), though, so theinherited
anndata.AnnData.copy()copies the standard (unused, always-Nonefor this class)
_Xattribute instead:Xsilently comes backNone, and the returned object is downgraded to aplain
AnnDatarather than preservingVCSCAnnData(or a subclass, such asone overriding the
Xproperty for a lazy-normalized view -- as BAL-Pf2'sown
import_data()does). Any caller relying onadata[mask].copy()hitsthis immediately -- in particular
parafac2's BiCV rank-selection routinedoes exactly this for its train/test splits, which is how this surfaced.
Adds an explicit
copy()override that copies every field (mirroring__getitem__'s existing field-by-field approach), including a realVCSCArray/VCSRArraycopy ofX/raw_X, and returnstype(self).Test plan
uv run pytest(1266 passed, 53 skipped, 1 pre-existing unrelatedfailure --
test_property_normalization.py::test_recipe_matches_reference[scanpy-VCSRArray],a numeric edge case on an all-constant column that reproduces
identically on
mainwithout this change) -- includes new tests intests/test_anndata_class.pycovering:X/raw_Xpreserved andindependent after
.copy(),obs/var/unspreserved, a subclassoverriding the
Xproperty round-tripping through.copy(), and theexact
adata[mask][:, mask].copy()patternparafac2's BiCV uses.uv run ruff check ./uv run ruff format --check ./uv run codespelluv run ty check