-
Notifications
You must be signed in to change notification settings - Fork 23
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] deprecate PyTorch loader prototype #1365
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1365 +/- ##
=======================================
Coverage 91.94% 91.94%
=======================================
Files 83 83
Lines 6742 6745 +3
=======================================
+ Hits 6199 6202 +3
Misses 543 543
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Just a minor point, @ebezzi will get back to you for a review on the notebooks
api/python/cellxgene_census/src/cellxgene_census/experimental/ml/pytorch.py
Outdated
Show resolved
Hide resolved
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. The notebook seems clear enough. Just two points:
- Would it be worth specifying that this notebook originally used another module that is now deprecated? I'm thinking that there might be some users who might be confused if they were using this as a reference for some of their work (we will announce the deprecation, but not many people follow our announcements).
- Curious why the model trained with this dataloader seems to perform slightly worse than the previous one. Could be because this is a newer Census version? That said, I don't think this is worth an investigation. It would be nice if we could some kind of regression testing against a real model (e.g. scvi), but this shouldn't be a blocker.
@ebezzi thanks!
|
The PyTorch loader built-in to the
cellxgene-census
API is "graduating" as TileDB-SOMA-ML. This adds runtime+docs deprecation notices (leaving it functional for now), and revises the tutorial notebook to use TileDB-SOMA-ML.