-
Notifications
You must be signed in to change notification settings - Fork 57
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
Make quad
policies stateless
#744
Make quad
policies stateless
#744
Conversation
Codecov Report
@@ Coverage Diff @@
## main #744 +/- ##
==========================================
+ Coverage 90.51% 90.54% +0.03%
==========================================
Files 202 202
Lines 7538 7552 +14
Branches 974 974
==========================================
+ Hits 6823 6838 +15
+ Misses 481 480 -1
Partials 234 234
|
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.
All looks good, except for the implicit use of the global random state. I think we should always force the user to provide their own random number generator to avoid unexpected side effects or non-determinism. With the changes coming in #581 this will become mandatory anyways.
Sure, I'll remove the default. What about wrappers like |
Yes, there the user should also provide their own RNG. It should still be optional and raise an error if at some point the compiled BQ method does need random numbers. Same as for the 'inegrate' method. |
In a Nutshell
The BQ policies were not stateless. This PR makes them stateless by passing the
rng
to the call function instead it being an attribute of the policy instances. The PR also adds some long overdue shape tests for the policies.Detailed Description
should be reviewed after #743 is merged since it branches off of it.Related Issues
Closes #...