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): subsetting with empty list #1243

Merged
merged 11 commits into from Dec 3, 2023
Merged

Conversation

ilan-gold
Copy link
Contributor

@ilan-gold ilan-gold commented Nov 28, 2023

@ilan-gold ilan-gold changed the title (fix): fix empty list subset (fix): subsetting with empty list Nov 28, 2023
Copy link

codecov bot commented Nov 28, 2023

Codecov Report

Merging #1243 (9fb9f33) into main (ff69d1e) will decrease coverage by 33.20%.
The diff coverage is 0.00%.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1243       +/-   ##
===========================================
- Coverage   85.27%   52.07%   -33.20%     
===========================================
  Files          34       34               
  Lines        5404     5413        +9     
===========================================
- Hits         4608     2819     -1789     
- Misses        796     2594     +1798     
Flag Coverage Δ
gpu-tests 52.07% <0.00%> (-0.13%) ⬇️

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

Files Coverage Δ
anndata/_core/index.py 59.47% <0.00%> (-32.89%) ⬇️

... and 23 files with indirect coverage changes

Copy link
Member

@flying-sheep flying-sheep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the idea is that if people want this fast, they should pass an array in the first place, I guess.

Maybe the error message should be more friendly?

@flying-sheep flying-sheep added this to the 0.10.4 milestone Nov 30, 2023
@ilan-gold
Copy link
Contributor Author

@flying-sheep So you're saying we should catch the error and then report back that the data types are mixed? Just a side note, this behavior of mixed types doesn't seem to work so this would not be a regression:

from anndata.tests.helpers import gen_adata
adata = gen_adata((100, 100))
adata[[0, 2], :] # good
adata[['cell5'], :] # good
adata[[0, 2, 'cell5'], :] # bad

@flying-sheep
Copy link
Member

Since we add custom handling here, raising a custom error (KeyError?) would be a nice thing to tell users what they’re doing wrong.

@ilan-gold
Copy link
Contributor Author

Sounds good @flying-sheep

anndata/_core/index.py Outdated Show resolved Hide resolved
@flying-sheep
Copy link
Member

why no release note?

@ilan-gold
Copy link
Contributor Author

I guess I didn't know what qualified as "unnecessary" for a release note and this seemed like one of those things.

@flying-sheep
Copy link
Member

Hmm, guess if that's not in the contribution guide we should add it, and if it is, we should link it.

As far as I understand, release notes are not encouraged if an unreleased feature gets modified or if the change is purely development process focused (tests, doc building, ...). This is a bugfix to a live feature, so it should get a release note.

anndata/_core/index.py Outdated Show resolved Hide resolved
anndata/tests/test_views.py Outdated Show resolved Hide resolved
@flying-sheep flying-sheep enabled auto-merge (squash) December 3, 2023 12:33
@flying-sheep flying-sheep merged commit 4745b1d into main Dec 3, 2023
14 checks passed
@flying-sheep flying-sheep deleted the ig/fix_subset_empty_list branch December 3, 2023 12:41
meeseeksmachine pushed a commit to meeseeksmachine/anndata that referenced this pull request Dec 3, 2023
flying-sheep pushed a commit that referenced this pull request Dec 3, 2023
#1248)

Co-authored-by: Ilan Gold <ilanbassgold@gmail.com>
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.

Subsetting w/ empty list when X is dense throws error
2 participants