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

Replace custom attrdict with ml_collections implementation. #1696

Merged
merged 16 commits into from
Oct 10, 2022

Conversation

martinkim0
Copy link
Contributor

@martinkim0 martinkim0 commented Sep 19, 2022

Closes #1690.

@martinkim0 martinkim0 self-assigned this Sep 19, 2022
@martinkim0 martinkim0 added enhancement dependencies Pull requests that update a dependency file labels Sep 19, 2022
@codecov
Copy link

codecov bot commented Sep 19, 2022

Codecov Report

Base: 90.71% // Head: 90.84% // Increases project coverage by +0.12% 🎉

Coverage data is based on head (c7c34d2) compared to base (d09368d).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1696      +/-   ##
==========================================
+ Coverage   90.71%   90.84%   +0.12%     
==========================================
  Files         116      116              
  Lines        9791     9764      -27     
==========================================
- Hits         8882     8870      -12     
+ Misses        909      894      -15     
Impacted Files Coverage Δ
scvi/data/_utils.py 94.21% <ø> (+7.09%) ⬆️
scvi/module/_totalvae.py 88.19% <ø> (+0.64%) ⬆️
scvi/data/_manager.py 98.19% <100.00%> (ø)
scvi/data/fields/_protein.py 98.21% <100.00%> (ø)
scvi/model/_totalvi.py 87.59% <100.00%> (ø)
scvi/utils/_attrdict.py 100.00% <100.00%> (+11.76%) ⬆️
scvi/module/_multivae.py 82.07% <0.00%> (+0.22%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@martinkim0
Copy link
Contributor Author

Seems that ml_collections does not allow ConfigDicts to be initialized with nested dictionaries where inner dictionaries have non-iterable types (e.g. ints) as keys (#13). This comes up as a problem when initializing ~scvi.model.TOTALVI, since "protein_batch_mask" has ints as keys.

@martinkim0 martinkim0 changed the title Replace custom attrdict with ml_collections implementation. [WIP] Replace custom attrdict with ml_collections implementation. Sep 26, 2022
@martinkim0 martinkim0 changed the title [WIP] Replace custom attrdict with ml_collections implementation. Replace custom attrdict with ml_collections implementation. Sep 28, 2022
@martinkim0 martinkim0 marked this pull request as ready for review September 28, 2022 18:29
docs/api/developer.md Outdated Show resolved Hide resolved
scvi/utils/_attrdict.py Outdated Show resolved Hide resolved
@martinkim0 martinkim0 enabled auto-merge (squash) October 6, 2022 19:09
@martinkim0 martinkim0 requested a review from adamgayoso October 6, 2022 19:09
@adamgayoso adamgayoso requested a review from watiss October 10, 2022 15:41
Copy link
Member

@adamgayoso adamgayoso left a comment

Choose a reason for hiding this comment

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

LGTM

@adamgayoso adamgayoso added this to the 0.18.0 milestone Oct 10, 2022
@martinkim0 martinkim0 merged commit c32f985 into master Oct 10, 2022
@adamgayoso adamgayoso deleted the wip-1690-mlcollections branch November 1, 2022 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sub custom attrdict for ml collections
2 participants