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

(chore): export read_elem and write_elem from the main package #1598

Merged
merged 24 commits into from
Aug 30, 2024

Conversation

ilan-gold
Copy link
Contributor

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

To do this properly, needed to export RWAble and the sparse backed classes

@ilan-gold ilan-gold added this to the 0.11.0 milestone Aug 14, 2024
Copy link

codecov bot commented Aug 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.35%. Comparing base (a92bda8) to head (a39d178).
Report is 69 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1598      +/-   ##
==========================================
- Coverage   86.81%   84.35%   -2.46%     
==========================================
  Files          37       37              
  Lines        6005     6016      +11     
==========================================
- Hits         5213     5075     -138     
- Misses        792      941     +149     
Files with missing lines Coverage Δ
src/anndata/__init__.py 93.54% <100.00%> (+1.54%) ⬆️
src/anndata/_core/sparse_dataset.py 92.92% <ø> (ø)
src/anndata/_types.py 87.50% <ø> (ø)
src/anndata/experimental/__init__.py 100.00% <100.00%> (ø)

... and 8 files with indirect coverage changes

@ilan-gold ilan-gold force-pushed the ig/{read,write}_elem_out_of_experimental branch from 269f198 to 99e0216 Compare August 14, 2024 18:06
@ilan-gold ilan-gold self-assigned this Aug 14, 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.

see above: define public API of CS{R,C}DataSet

@ilan-gold
Copy link
Contributor Author

ilan-gold commented Aug 28, 2024

@flying-sheep I have been thinking about this a bit. Is it bad practice to keep the CS{C,R}Dataset classes experimental while exporting something from core that can deal with them? I don't think so. Even though X has the wrong type annotation, it should include CS{R,C}Dataset, and this would be an example of a public API referencing an experimental one.

@flying-sheep
Copy link
Member

You’re right, that’s not a problem.

We could also e.g. export Protocols as CSR/CSCDataset that only expose or describe what we know we’ll keep (e.g. shape, dtype, that users can slice them via __getitem__ and convert them into arrays, …)



class CustomExperimental(types.ModuleType):
def __getattribute__(self, key: str) -> Any:
Copy link
Member

Choose a reason for hiding this comment

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

why not just define def __getattr__(key: str) -> Any: ... in the experimental module directly?

https://peps.python.org/pep-0562/ exists since 3.7

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has the advantage of never having to thinking about this again. As we move things out of experimental into the public API, we'll never have to update this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe you could do __getattr__ without a circular import? I guess I can try but I just kind of assumed not

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although now that I think about it, maybe a blanket warning isn't such a good idea...would want to be explicit about deprecations

Copy link
Member

Choose a reason for hiding this comment

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

You can just import inside of __getattr__, no?

experimental/__init__.py:

[…]

def __getattr__(key: str) -> Any:
    import anndata

    if key in anndata.__all__: ...

@ilan-gold ilan-gold enabled auto-merge (squash) August 30, 2024 12:06
@flying-sheep flying-sheep disabled auto-merge August 30, 2024 12:25
@flying-sheep flying-sheep enabled auto-merge (squash) August 30, 2024 12:26
@flying-sheep flying-sheep merged commit dde809d into main Aug 30, 2024
13 checks passed
@flying-sheep flying-sheep deleted the ig/{read,write}_elem_out_of_experimental branch August 30, 2024 12:28
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.

Move read_elem, write_elem from experimental into main API
2 participants