-
Notifications
You must be signed in to change notification settings - Fork 22
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
[python] Add get_obs #1151
[python] Add get_obs #1151
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1151 +/- ##
==========================================
+ Coverage 91.12% 91.16% +0.04%
==========================================
Files 77 77
Lines 5857 5896 +39
==========================================
+ Hits 5337 5375 +38
- Misses 520 521 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
Overall this looks good to me. I have one question about the implementation in the test case.
@pablo-gar @ebezzi - Can y'all take a look too please?
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.
LGTM but please wait for @ebezzi and @pablo-gar to review as well before merging to main
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.
@prathapsridharan @ebezzi I see a pattern in the file structure in api/python/cellxgene_census/src/cellxgene_census
whereby the SOMAExperiment helpers are under _experiment.py
, the anndata helpers are _get_anndata.py
Would it make sense to have the proposed helpers in this PR under a new _axis_metadata.py
or _get_axis_metadata.py
file?
I defer to you both as this touches dev eng design.
LGTM otherwise
I think we can shift things around later if needed but for now this organization seems fine to me |
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.
Typo in the docs, but otherwise LGTM.
api/python/cellxgene_census/src/cellxgene_census/_get_anndata.py
Outdated
Show resolved
Hide resolved
api/python/cellxgene_census/src/cellxgene_census/_get_anndata.py
Outdated
Show resolved
Hide resolved
Co-authored-by: Emanuele Bezzi <[email protected]>
Thanks for the catch! Applied your changes. |
Fixes #1037
TODO:
get_var