-
Notifications
You must be signed in to change notification settings - Fork 25
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 type checks to obsm
, obsp
, varm
and varp
early in ingestion
#2131
[python] Add type checks to obsm
, obsp
, varm
and varp
early in ingestion
#2131
Conversation
obsm
, obsp
, varm
and varp
early in ingestionobsm
, obsp
, varm
and varp
early in ingestion
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2131 +/- ##
===========================================
+ Coverage 78.52% 91.25% +12.73%
===========================================
Files 136 35 -101
Lines 10759 3807 -6952
Branches 209 0 -209
===========================================
- Hits 8448 3474 -4974
+ Misses 2211 333 -1878
+ Partials 100 0 -100
Flags with carried forward coverage won't be shown. Click here to find out more.
|
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.
@XanthosXanthopoulos thanks for working on this! :)
Let's also add a unit-test case
- In
apis/python/tests/test_basic_anndata_io.py
- You can get
adata
- Make a
bdata
withbdata = ad.AnnData(X=adata.X, obs=adata.obs, var=adata.var, obsm={"testing": adata.obs})
-- this will result in a dataframe being in anobsm
slot - Wrap a
from_anndata
call inwith pytest.raises(TypeError)
- Assert the destination SOMA URI doesn't exist at all (i.e. there was no partial write)
for key in anndata.obsm.keys(): | ||
if not isinstance(anndata.obsm[key], get_args(Matrix)): | ||
raise TypeError( | ||
f"Obsm value at {key} in not of type {list(cl.__name__ for cl in get_args(Matrix))}" |
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.
Users will expect obsm
not Obsm
-- same for the other three
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.
🚢 Thanks @XanthosXanthopoulos !!! 🚢
@XanthosXanthopoulos please feel free to self-merge once CI is green |
The backport to
To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-release-1.7 release-1.7
# Navigate to the new working tree
cd .worktrees/backport-release-1.7
# Create a new branch
git switch --create backport-2131-to-release-1.7
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick --mainline 1 ef6cb19ef8dd159db08d602acdb68409da51cdee
# Push it to GitHub
git push --set-upstream origin backport-2131-to-release-1.7
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-release-1.7 Then, create a pull request where the |
…ame-on-ingest [python] Add type checks to `obsm`, `obsp`, `varm` and `varp` early in ingestion
…ame-on-ingest [python] Add type checks to `obsm`, `obsp`, `varm` and `varp` early in ingestion
…arm` and `varp` early in ingestion (#2138) * Merge pull request #2131 from single-cell-data/xan/early-error-dataframe-on-ingest [python] Add type checks to `obsm`, `obsp`, `varm` and `varp` early in ingestion * Merge pull request #2131 from single-cell-data/xan/early-error-dataframe-on-ingest [python] Add type checks to `obsm`, `obsp`, `varm` and `varp` early in ingestion * complete merge --------- Co-authored-by: XanthosXanthopoulos <[email protected]>
Issue and/or context:
closes #2118
Changes:
Adds explicit type checks to
from_anndata
forobsm
,obsp
,varm
andvarp
before the actual ingestion start to catch the case where aDataFrame
is passed.Notes for Reviewer: