-
Notifications
You must be signed in to change notification settings - Fork 196
Deprecation Warnings for build_posterior stringly typed parameters #1627
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1627 +/- ##
=======================================
Coverage ? 82.55%
=======================================
Files ? 135
Lines ? 10948
Branches ? 0
=======================================
Hits ? 9038
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.
Great work! 👏
added a couple of comments and suggestion that should be easy to address.
sbi/inference/trainers/fmpe/fmpe.py
Outdated
self.build_posterior, | ||
locals(), | ||
{ | ||
"vectorfield_sampling_parameters", | ||
}, |
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.
formatting is a bit verbose here, can we reduce the number of lines by removing the trailing commas? If no, it's fine.
…rior arguments" This reverts commit be39800.
I have updated the previous implementation for raising deprecation warnings. Instead of duplicating the warning checks across each trainer subclass, I have centralized the check in the base class. |
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.
Good refactoring! added a couple of last comments 👍
Co-authored-by: Jan <janfb@users.noreply.github.com>
Co-authored-by: Jan <janfb@users.noreply.github.com>
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 all good now - thanks! 🙏
This PR adds deprecation warnings for the stringly-typed
build_posterior
method parameters in all trainer classes and warns users to use theposterior_parameters
parameter to pass posterior parameters instead of using the dictionary configurations. In addition, all the tutorials and tests have been updated to use the strongly typed dataclasses introduced in this PR. It also includes a how-to guide for using thePosteriorParameters
data classes.