Skip to content
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): empty boolean mask on backed sparse matrix #1321

Merged
merged 12 commits into from
Jan 25, 2024
Merged

Conversation

ilan-gold
Copy link
Contributor

@ilan-gold ilan-gold commented Jan 19, 2024

@ilan-gold ilan-gold changed the base branch from main to one_group_bool_mask_fix January 19, 2024 15:00
@ilan-gold
Copy link
Contributor Author

blocked by #1320 (comment)

@ilan-gold ilan-gold added this to the 0.10.5 milestone Jan 19, 2024
@ilan-gold ilan-gold self-assigned this Jan 19, 2024
Copy link

codecov bot commented Jan 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (99253c7) 85.67% compared to head (5b13eb8) 83.56%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1321      +/-   ##
==========================================
- Coverage   85.67%   83.56%   -2.11%     
==========================================
  Files          34       34              
  Lines        5452     5458       +6     
==========================================
- Hits         4671     4561     -110     
- Misses        781      897     +116     
Flag Coverage Δ
gpu-tests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
anndata/_core/sparse_dataset.py 94.51% <100.00%> (+0.76%) ⬆️

... and 7 files with indirect coverage changes

@ilan-gold ilan-gold marked this pull request as ready for review January 19, 2024 17:03
@ilan-gold ilan-gold requested a review from ivirshup January 19, 2024 17:22
Base automatically changed from one_group_bool_mask_fix to main January 23, 2024 12:30
anndata/_core/sparse_dataset.py Outdated Show resolved Hide resolved
anndata/tests/test_backed_sparse.py Show resolved Hide resolved
@ivirshup
Copy link
Member

blocked by #1320 (comment)

Do you still consider this blocked? I think it's fine to assume it acts like you'd expect a numpy array too

@ilan-gold
Copy link
Contributor Author

blocked by #1320 (comment)

Do you still consider this blocked? I think it's fine to assume it acts like you'd expect a numpy array too

Nope, the issue is filed. You thought it was a bug too so we can't wait for a scipy release over this.

@ilan-gold
Copy link
Contributor Author

@ivirshup I would wait for an answer from the scipy people confirming scipy/scipy#19919 (comment) before merging, just to be sure empty array creation is correct. We're so close to confirmation of future behavior that I think it's worth waiting unless we're in a rush, in which case you can merge.

@ivirshup
Copy link
Member

I think that the array classes in scipy are going to end up returning something with .shape = (0, 0) here, but also that it's not a super big deal if we end up changing that.

@ivirshup ivirshup merged commit 3fcd755 into main Jan 25, 2024
13 checks passed
@ivirshup ivirshup deleted the empty_bool_mask_fix branch January 25, 2024 16:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SparseDataset errors out with empty boolean mask
2 participants