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): correct typing of AnnData.X #1616

Merged
merged 11 commits into from
Aug 30, 2024
Merged

(fix): correct typing of AnnData.X #1616

merged 11 commits into from
Aug 30, 2024

Conversation

ilan-gold
Copy link
Contributor

@ilan-gold ilan-gold commented Aug 28, 2024

Copy link

codecov bot commented Aug 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.32%. Comparing base (8f3299b) to head (1322408).
Report is 70 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1616      +/-   ##
==========================================
- Coverage   86.85%   84.32%   -2.53%     
==========================================
  Files          37       37              
  Lines        6027     6005      -22     
==========================================
- Hits         5235     5064     -171     
- Misses        792      941     +149     
Files with missing lines Coverage Δ
src/anndata/_core/anndata.py 83.72% <100.00%> (ø)
src/anndata/_core/merge.py 83.91% <ø> (-11.08%) ⬇️
src/anndata/_core/storage.py 100.00% <100.00%> (ø)
src/anndata/_types.py 87.50% <100.00%> (-0.60%) ⬇️

... and 7 files with indirect coverage changes

@ilan-gold ilan-gold added this to the 0.10.9 milestone Aug 28, 2024
@ilan-gold ilan-gold requested a review from flying-sheep August 28, 2024 12:20
@ilan-gold ilan-gold modified the milestones: 0.10.9, 0.10.10 Aug 28, 2024
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.

Shouldn’t we unify this with ArrayDataStructureType?

I don’t quite know why that one is an enum, maybe we can replace it with

ArrayDataStructureType = Union[All|Of|This|Here]

and then do

from typing import get_args

isinstance(get_args(ArrayDataStructureType))

@ilan-gold
Copy link
Contributor Author

I don’t quite know why that one is an enum, maybe we can replace it with

I also didn't know but don't really question these sorts of things. I imagine it served a purpose at some point, but like I said in the related PR, that seems to have changed recently.

@flying-sheep
Copy link
Member

I guess then we should unify the two. Either as part of this PR or later, as you wish

@flying-sheep
Copy link
Member

flying-sheep commented Aug 29, 2024

We could maybe use __instancecheck__ and is_array_api_obj:

import array_api_compat

class ArrayApiMeta(type):
    def __instancecheck__(cls, instance: object) -> bool:
        return array_api_compat.is_array_api_obj(instance)

class ArrayApi(meta=ArrayApiMeta):
    pass

@ilan-gold
Copy link
Contributor Author

I'm going to push back on the array-api here because I think it's out of scope and testing for it will be non-trivial. I think we should make a separate branch/PR for that specifically, and stick it in 0.12. We can test for jax, which now has a working CPU implementation (like numpy). This would be a very cool feature.

@ilan-gold
Copy link
Contributor Author

ilan-gold commented Aug 30, 2024

There's even an issue for this: #1195. I will milestone/sprint it so we have it on the radar now.

@ilan-gold ilan-gold requested a review from flying-sheep August 30, 2024 10:51
@flying-sheep flying-sheep enabled auto-merge (squash) August 30, 2024 11:05
@flying-sheep flying-sheep merged commit a92bda8 into main Aug 30, 2024
15 checks passed
@flying-sheep flying-sheep deleted the ig/X_typing branch August 30, 2024 11:38
@scverse scverse deleted a comment from lumberbot-app bot Aug 30, 2024
flying-sheep added a commit that referenced this pull request Aug 30, 2024
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.

2 participants