fix: write through to correct parent element in ArrayView - #2596
flying-sheep wants to merge 7 commits into
Conversation
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
85b3109 to
52557f3
Compare
There was a problem hiding this comment.
Hi here is an AI report. Feel free to ignore all except the regression case. This is an old one I revived and rerun the tests and they are still valid.
main |
this PR | |
|---|---|---|
el[1:3][0,0] = x, el.T[0,0] = x |
silently written to the wrong cell | raises ✅ |
bdata.obsm["x"] = view.obsm["x"] (dense) |
stored the foreign view | detached copy ✅ |
np.zeros_like(el)[:] = 1, el.astype("f4")[0] = x |
wrong cell | raises — but these alias nothing |
view.X[:][1,0] = x, el[...][1,0] = x |
correct (warn + copy-on-write) | raises ❌ |
bdata.X = view.X (sparse / dask / DataFrame) |
stored the foreign view | unchanged ❌ |
⚠️ Over-strict — derivatives that own their memory now raise (fix below)
np.zeros_like(el), el.astype("f4"), el.flatten(), el[[1,3]] alias nothing, so they can't hit the wrong cells — numpy semantics (write lands in the copy, AnnData untouched) is correct there.
+def _inherited_view_args(arr: np.ndarray, obj: object) -> ElementRef | None:
+ """Only arrays aliasing the element can write to the wrong cells."""
+ if (ref := getattr(obj, "_view_args", None)) is None:
+ return None
+ return ref if np.may_share_memory(arr, obj) else None
+
def __array_finalize__(self, obj: np.ndarray | None) -> None:
self._is_element = False
- if obj is not None:
- self._view_args = getattr(obj, "_view_args", None)
+ self._view_args = _inherited_view_args(self, obj)Cost: the fancy and mask params of test_derived_view_write_error have to go — they own their memory too. Your call whether catching el[[1,3]][0] = … as a user mistake is worth diverging from numpy for astype/zeros_like.
Tests: test_derived_view_owning_memory_is_writable[zeros_like|astype|flatten|fancy|mask]
❌ Regression — view.X[:] and el[...] worked on main (no fix from me)
An exact alias maps indices 1:1 onto the element, so main's redirect landed on the right cell — view.X[:][1,0] = x warned, copied on write, and hit [1,0]. It now raises. The X = adata.X[:] h5py idiom lands here.
I tried the check in __array_finalize__ — numpy applies .T geometry after finalizing, so transpose slips through as an alias and test_derived_view_write_error[transpose] breaks. And at write time via ref.get() — the pointers don't compare equal. Neither is cheap, and raising is safe, so maybe just name el[:] in the error message.
Tests: test_exact_alias_write_is_copy_on_modify[full-slice|ellipsis|full-extent] — left plain, not xfail, since nothing fixes them.
❌ Pre-existing — coerce_array only de-views ArrayView (fix below)
The invariant in its comment doesn't hold for the other element types, so an element of one AnnData can still be a view of another and writing to it actualizes the wrong object:
def test_write_to_element_does_not_touch_the_donor() -> None:
donor = ad.AnnData(sparse.random(10, 10, density=0.5, format="csr", random_state=0))
donor_view = donor[:5, :]
other = ad.AnnData(sparse.eye_array(5, 10, format="csr"))
other.layers["l"] = donor_view.X
other.layers["l"][0, 0] = 42.0 # must not warn about, or actualize, `donor_view`
assert donor_view.is_view
assert other.layers["l"][0, 0] == 42.0Fails with ImplicitModificationWarning: Trying to modify attribute '.X' of view, initializing view as actual — donor_view is actualized, other isn't. Same for a dask obsm; DataFrameView leaks silently.
Every view class' copy() already returns the plain type (DataFrameView→DataFrame, SparseCSRMatrixView→csr_matrix, DaskArrayView→Array, …), so the general fix is a smaller diff and lets _detach go:
-from .views import ArrayView
+from .views import _SetItemMixin
- if isinstance(value, ArrayView):
+ if isinstance(value, _SetItemMixin):
# An element must not be a view of some other AnnData:
# writing to it would warn about, and actualize, that one instead of this one.
- value = value.copy() if value._view_args is not None else value._detach()
+ # Every view class’ `copy` returns the plain, detached type.
+ value = value.copy()Tests: test_element_is_never_a_foreign_view[sparse|sparse-array|dask], test_write_to_element_does_not_touch_the_donor
Relnote
- The guard is
__setitem__-only:el.fill(),el.put(),np.copyto(el, …),el.flat[0] = …,el += 1
Full test file — 12 failing on this branch; the two patches above turn 9 of them green (test_views.py + test_concatenate.py: 3999 passed). The 3 test_exact_alias_write_is_copy_on_modify params stay red.
"""Gaps in #2596 — add to tests/test_views.py."""
from __future__ import annotations
from typing import TYPE_CHECKING, cast
import numpy as np
import pytest
from scipy import sparse
import anndata as ad
from anndata import ImplicitModificationWarning
from anndata._core.views import ArrayView
if TYPE_CHECKING:
from collections.abc import Callable
# --- 1. `coerce_array` de-views dense only -----------------------------------
@pytest.mark.parametrize(
("value", "plain_type"),
[
pytest.param(lambda: np.arange(50.0).reshape(10, 5), np.ndarray, id="dense"),
pytest.param(
lambda: sparse.random(10, 5, density=0.5, format="csr", random_state=0),
sparse.csr_matrix,
id="sparse",
),
pytest.param(
lambda: sparse.random_array((10, 5), density=0.5, format="csr", random_state=0),
sparse.csr_array,
id="sparse-array",
),
pytest.param(
lambda: __import__("dask.array", fromlist=["x"]).from_array(
np.arange(50.0).reshape(10, 5), chunks=5
),
None, # dask.array.Array
id="dask",
),
],
)
def test_element_is_never_a_foreign_view(
value: Callable[[], object], plain_type: type | None
) -> None:
"""No element may be a view of *another* AnnData, whatever its type.
Writing to it would warn about, and actualize, that other object.
"""
adata = ad.AnnData(np.zeros((10, 10)), obsm={"o": value()})
view = adata[:5, :]
other = ad.AnnData(np.zeros((5, 10)), obsm={"o": view.obsm["o"]})
stored = other.obsm["o"]
assert getattr(stored, "_view_args", None) is None, type(stored)
if plain_type is not None:
assert type(stored) is plain_type
def test_write_to_element_does_not_touch_the_donor() -> None:
"""The regression the `coerce_array` de-view exists to prevent, for sparse."""
donor = ad.AnnData(sparse.random(10, 10, density=0.5, format="csr", random_state=0))
donor_view = donor[:5, :]
other = ad.AnnData(sparse.eye_array(5, 10, format="csr"))
other.layers["l"] = donor_view.X
other.layers["l"][0, 0] = 42.0 # must not warn about, or actualize, `donor_view`
assert donor_view.is_view
assert other.layers["l"][0, 0] == 42.0
# --- 2. the guard also refuses derivatives that own their memory -------------
# NOTE: adopting this means dropping the `fancy` and `mask` params of
# `test_derived_view_write_error` — they own their memory too.
@pytest.mark.parametrize(
"derive",
[
pytest.param(lambda el: np.zeros_like(el), id="zeros_like"),
pytest.param(lambda el: el.astype(np.float32), id="astype"),
pytest.param(lambda el: el.flatten(), id="flatten"),
pytest.param(lambda el: el[[1, 3]], id="fancy"),
pytest.param(lambda el: el[np.arange(5) % 2 == 1], id="mask"),
],
)
def test_derived_view_owning_memory_is_writable(
derive: Callable[[ArrayView], np.ndarray],
) -> None:
"""A derivative that doesn’t alias the element is an ordinary array.
Writing to it can’t hit the wrong cells, so it behaves like numpy:
the write lands in the copy and the AnnData is untouched.
"""
adata = ad.AnnData(np.zeros((10, 10)), obsm={"o": np.arange(50.0).reshape(10, 5)})
element = cast("ArrayView", adata[:5, :].obsm["o"])
target = derive(element)
assert not np.shares_memory(target, element), "test targets non-aliasing derivatives"
target.reshape(-1)[0] = 777.0
assert target.reshape(-1)[0] == 777.0
np.testing.assert_array_equal(
adata.obsm["o"], np.arange(50.0).reshape(10, 5), err_msg="parent must not change"
)
def test_element_write_still_warns() -> None:
"""Control: the element itself keeps copy-on-modify."""
adata = ad.AnnData(np.zeros((10, 10)), obsm={"o": np.arange(50.0).reshape(10, 5)})
view = adata[:5, :]
with pytest.warns(ImplicitModificationWarning):
view.obsm["o"][1, 0] = 777.0
assert np.asarray(view.obsm["o"])[1, 0] == 777.0
assert adata.obsm["o"][1, 0] == 5.0
# --- 3. an exact alias addresses the element, so its write is correct ---------
# NOTE: unfixed — fails on this branch and with the two patches above.
@pytest.mark.parametrize(
"derive",
[
pytest.param(lambda el: el[:], id="full-slice"),
pytest.param(lambda el: el[...], id="ellipsis"),
pytest.param(lambda el: el[0:5], id="full-extent"),
],
)
def test_exact_alias_write_is_copy_on_modify(
derive: Callable[[ArrayView], np.ndarray],
) -> None:
"""`view.X[:]` maps indices 1:1 onto the element, as `main` handled correctly."""
adata = ad.AnnData(np.zeros((10, 10)), obsm={"o": np.arange(50.0).reshape(10, 5)})
view = adata[:5, :]
target = derive(cast("ArrayView", view.obsm["o"]))
with pytest.warns(ImplicitModificationWarning):
target[1, 0] = 777.0
assert np.asarray(view.obsm["o"])[1, 0] == 777.0
assert adata.obsm["o"][1, 0] == 5.0, "parent must not be mutated"24c7c00 to
c452e79
Compare
see #2582 (review)
TODO: