-
Notifications
You must be signed in to change notification settings - Fork 12
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
✨ Add n_observations function to read n_obs from AnnData and tiledbsoma #2097
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2097 +/- ##
==========================================
- Coverage 92.52% 92.45% -0.08%
==========================================
Files 54 55 +1
Lines 6435 6480 +45
==========================================
+ Hits 5954 5991 +37
- Misses 481 489 +8 ☔ View full report in Codecov by Sentry. |
Do we want it to be used by default? |
This shouldn't be part of the Artifact API. If it's used internally while constructing the Artifact object, that's OK! Can you more comprehensively document why this PR exists? The Slack link is internal and hence nobody external will understand why you're doing what you're doing here. |
This looks perfectly great. We should just call this in |
Ok, makes sense. It still needs a bit of improvement though. |
Yes, I think so! |
Re https://laminlabs.slack.com/archives/C04FPE8V01W/p1726658747846539
This function infers
n_obs
fromanndata
(bothzarr
andhdf5
) andtiledbsoma
. I am reluctant to add this toArtifact
by default though.