-
Notifications
You must be signed in to change notification settings - Fork 154
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(fix): raise error on non-integer floating types in iterables #1746
base: main
Are you sure you want to change the base?
Conversation
ilan-gold
commented
Nov 8, 2024
•
edited
Loading
edited
- Closes Unexpected Behavior: AnnData Allows Indexing with Float Arrays Without Error #1735
- Tests added
- Release note added (or unnecessary)
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1746 +/- ##
==========================================
- Coverage 86.99% 84.53% -2.47%
==========================================
Files 40 40
Lines 6053 6057 +4
==========================================
- Hits 5266 5120 -146
- Misses 787 937 +150
|
src/anndata/_core/index.py
Outdated
@@ -82,6 +82,10 @@ def name_idx(i): | |||
indexer = np.array(indexer) | |||
if len(indexer) == 0: | |||
indexer = indexer.astype(int) | |||
if isinstance(indexer, np.ndarray) and indexer.dtype == float: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the comparison so it doesn’t only work with float64:
>>> np.dtype(np.float64) == float
True
>>> np.dtype(np.float32) == float
False
(and parametrize the tests please)
@pytest.mark.parametrize( | ||
"index", | ||
[ | ||
pytest.param(sparse.csr_matrix(np.random.random((1, 10))), id="sparse"), | ||
pytest.param([1.2, 3.4], id="list"), | ||
*( | ||
pytest.param(np.array([1.2, 2.3], dtype=dtype), id=f"ndarray-{dtype}") | ||
for dtype in [np.float32, np.float64] | ||
), | ||
], | ||
) | ||
def test_index_float_sequence_raises_error(index): | ||
with pytest.raises(IndexError, match=r"has floating point values"): | ||
gen_adata((10, 10))[index] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pytest.mark.parametrize( | |
"index", | |
[ | |
pytest.param(sparse.csr_matrix(np.random.random((1, 10))), id="sparse"), | |
pytest.param([1.2, 3.4], id="list"), | |
*( | |
pytest.param(np.array([1.2, 2.3], dtype=dtype), id=f"ndarray-{dtype}") | |
for dtype in [np.float32, np.float64] | |
), | |
], | |
) | |
def test_index_float_sequence_raises_error(index): | |
with pytest.raises(IndexError, match=r"has floating point values"): | |
gen_adata((10, 10))[index] | |
@pytest.mark.parametrize( | |
"index", | |
[ | |
pytest.param(sparse.csr_matrix(np.random.random((1, 10))), id="sparse"), | |
pytest.param([1.2, 3.4], id="list"), | |
pytest.param(np.array([1.2, 2.3], dtype=dtype), id=f"ndarray-{dtype}") | |
], | |
) | |
@pytest.mark.parametrize("dtype", [np.float32, np.float64]) | |
def test_index_float_sequence_raises_error(index, dtype): | |
index = index.astype(dtype) | |
with pytest.raises(IndexError, match=r"has floating point values"): | |
gen_adata((10, 10))[index] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flying-sheep is the goal here to ensure dtype is checked on sparse matrices as well? I think it's enough to check the numpy arrays. Also we can't astype
a list. Also dtype
is not accessible outside of its parametrization/the test.