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

Make quad policies stateless #744

Merged

Conversation

mmahsereci
Copy link
Contributor

@mmahsereci mmahsereci commented Nov 21, 2022

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 #...

@mmahsereci mmahsereci marked this pull request as draft November 21, 2022 20:00
@mmahsereci mmahsereci self-assigned this Nov 21, 2022
@mmahsereci mmahsereci added the quad Issues related to quadrature label Nov 21, 2022
@mmahsereci mmahsereci added this to In progress in ProbNum Development via automation Nov 21, 2022
@codecov
Copy link

codecov bot commented Nov 22, 2022

Codecov Report

Merging #744 (7c0839d) into main (92e59ad) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            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              
Impacted Files Coverage Δ
src/probnum/quad/_bayesquad.py 100.00% <100.00%> (ø)
src/probnum/quad/solvers/_bayesian_quadrature.py 96.73% <100.00%> (+1.04%) ⬆️
src/probnum/quad/solvers/policies/_policy.py 100.00% <100.00%> (ø)
...rc/probnum/quad/solvers/policies/_random_policy.py 100.00% <100.00%> (ø)
...um/quad/solvers/policies/_van_der_corput_policy.py 100.00% <100.00%> (ø)
...bnum/quad/solvers/stopping_criteria/_max_nevals.py 100.00% <100.00%> (ø)

@mmahsereci mmahsereci marked this pull request as ready for review November 23, 2022 16:17
Copy link
Contributor

@JonathanWenger JonathanWenger left a 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.

src/probnum/quad/solvers/_bayesian_quadrature.py Outdated Show resolved Hide resolved
src/probnum/quad/solvers/_bayesian_quadrature.py Outdated Show resolved Hide resolved
tests/test_quad/test_bayesian_quadrature.py Outdated Show resolved Hide resolved
tests/test_quad/test_bayesian_quadrature.py Outdated Show resolved Hide resolved
ProbNum Development automation moved this from In progress to Review in progress Nov 24, 2022
@mmahsereci
Copy link
Contributor Author

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 bayesquad that are not really part of the core library? I assume the default rng there needs to go as well @JonathanWenger ? And thanks for reviewing so promptly!

@JonathanWenger
Copy link
Contributor

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.

@mmahsereci mmahsereci merged commit 8dde541 into probabilistic-numerics:main Nov 24, 2022
ProbNum Development automation moved this from Review in progress to Done Nov 24, 2022
@mmahsereci mmahsereci deleted the mm-quad-stateless-policy branch November 24, 2022 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
quad Issues related to quadrature
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants