chore: add sparse index dtype checks to assert_equal#2362
Conversation
When exact=True, assert_equal now checks that indptr and indices dtypes match between sparse matrices, catching silent precision mismatches (e.g. int32 vs int64) that were previously lost during dense conversion. Fixes scverse#860
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2362 +/- ##
==========================================
- Coverage 87.35% 85.39% -1.96%
==========================================
Files 49 49
Lines 7784 7787 +3
==========================================
- Hits 6800 6650 -150
- Misses 984 1137 +153
|
|
Please update your PR title to be |
| assert a.indptr.dtype == b.indptr.dtype, ( | ||
| f"{elem_name}: indptr dtype mismatch: {a.indptr.dtype} vs {b.indptr.dtype}" | ||
| ) | ||
| assert a.indices.dtype == b.indices.dtype, ( | ||
| f"{elem_name}: indices dtype mismatch: {a.indices.dtype} vs {b.indices.dtype}" | ||
| ) |
There was a problem hiding this comment.
| assert a.indptr.dtype == b.indptr.dtype, ( | |
| f"{elem_name}: indptr dtype mismatch: {a.indptr.dtype} vs {b.indptr.dtype}" | |
| ) | |
| assert a.indices.dtype == b.indices.dtype, ( | |
| f"{elem_name}: indices dtype mismatch: {a.indices.dtype} vs {b.indices.dtype}" | |
| ) | |
| assert a.indptr.dtype == b.indptr.dtype, ( | |
| f"{elem_name}: indptr dtype mismatch" | |
| ) | |
| assert a.indices.dtype == b.indices.dtype, ( | |
| f"{elem_name}: indices dtype mismatch" | |
| ) |
I think the mismatch's contents are already reported by assert
There was a problem hiding this comment.
Done — simplified the assert messages and included elem_name for context. Updated in 6cb2b35.
| b.indptr = b.indptr.astype(np.int64) | ||
| b.indices = b.indices.astype(np.int64) |
There was a problem hiding this comment.
Please parametrize this test to change each of these one-at-a-time and then have the match check that indices or indptr is in the message accordingly
There was a problem hiding this comment.
Done — parametrized with @pytest.mark.parametrize("attr", ["indices", "indptr"]), each tested one-at-a-time with match=attr. Updated in 6cb2b35.
LiudengZhang
changed the title
Address review feedback: use elem_name-only messages in asserts and parametrize test by attribute (indices/indptr).
for more information, see https://pre-commit.ci
| """assert_equal(exact=True) should detect indptr/indices dtype mismatches.""" | ||
| a = sparse.csr_matrix(np.eye(3)) | ||
| b = sparse.csr_matrix(np.eye(3)) | ||
| setattr(b, attr, getattr(b, attr).astype(np.int32)) |
There was a problem hiding this comment.
6cb2b35#diff-08a6944653efc734f4c8cb856fc854b9f21d655ca69796af2b170c53bff7919eL255-L256 this si what you want?
| setattr(b, attr, getattr(b, attr).astype(np.int32)) | |
| setattr(b, attr, getattr(b, attr).astype(np.int64)) |
There was a problem hiding this comment.
Good catch — updated to int64. The default scipy index dtype is int32, so int32 was a no-op.
…cks to assert_equal) (#2371)
2 participants
Summary
assert_equalfor sparse matrices to checkindptrandindicesdtype consistency whenexact=Trueasarray()before comparison, silently losing index dtype information (e.g. int32 vs int64 mismatches went undetected)Changes
src/anndata/tests/helpers.py: Addedindptr/indicesdtype assertions inassert_equal_sparsebefore dense conversion, gated onexact=Trueand both operands being sparsetests/test_helpers.py: Addedtest_assert_equal_sparse_index_dtype— creates CSR matrices with identical values but different index dtypes, verifies exact comparison catches the mismatchTest plan
pytest tests/test_helpers.py::test_assert_equal_sparse_index_dtype— passestest_helpers.pypass (awkward failures are pre-existing env issue)Fixes #860