-
Notifications
You must be signed in to change notification settings - Fork 196
Add protocol for estimator builder #1633
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 protocol for estimator builder #1633
Conversation
…ocol for all the childs
…place callable annotation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1633 +/- ##
=======================================
Coverage ? 82.56%
=======================================
Files ? 135
Lines ? 10957
Branches ? 0
=======================================
Hits ? 9047
Misses ? 1910
Partials ? 0
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.
Looks good, thanks! A couple of comments and questions remain.
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.
some last comments, but almost done! 👍
13a416b
to
8b41b1a
Compare
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.
Almost done, please just revert the changes done in sbi_types.py
so that it in sync with main
, i.e., does not show up in this PR anymore. thanks! 🙏
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.
This pull request builds upon #1557 and addresses remaining comments by updating
DensityEstimatorBuilder
Protocol to be more flexible. This change allows the protocol to work with different types of conditional density estimators.