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

BQ stopping criteria that depend on model statistics #463

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mmahsereci
Copy link
Contributor

@mmahsereci mmahsereci commented May 7, 2024

Issue #, if available:

Description of changes:

@apaleyes This PR is a draft so far, but it would be great if you could have a look at the following points since it might require some changes in the core package, too.

The PR adds the COV stopping criterion for BQ. The stopping criterion needs access to the current integral mean and variance estimate. Stopping criteria currently only consume the LoopState to decide when to stop. The mentioned statistics are currently not stored in LoopState (only X and Y are).

So, how can stopping criteria access model statistics to assess stopping?

  1. One way is to change the LoopState class in the core package. For example the loop_state.update method could also request the models in addition to X and Y and then store the relevant statistics. But this is a bit tricky since:
    • the models are not directly accessible in the OuterLoop where loop_state.update() is called (only the model updaters are accessible which, in their basic form, do not even necessarily contain a model).
    • The relevant model stats need to be added to the loop state after the models are updated and not after the new points are collected (where update is currently called).
    • So one way would be to add a method update_model_stats to the core LoopState class which is called in the loop after the models are updated, similar to how loop_state.update is called. This method could do nothing initially but could be overridden by specific packages. It is unclear however, what the inputs to this method are supposed to be (the model updaters without models??) and potentially the model updaters need to be adjusted to contain models.

Here is another approach (which reflects the code in this PR)

  1. The core OuterLoop already contains a method _update_models. I added another method called _update_loop_state which does nothing in the base class, but can be overridden by specific outer loops. In comparison to the first approach, this one not only requires a new method in LoopState but also in OuterLoop that both need to be overridden in specific packages if needed. So it is less principled since a method in OuterLoop somehow meddles with how the loop state is being updated. But therefore there are no additional assumptions on the core model updaters.

I personally think that the first approach is neater but would require more changes in the core package. The second approach feels a bit hacky but most of the changes happen in the quadrature package instead of the core package.

Before I implement tests etc, let me know please what you think about both options or if you have another idea on how to incorporate stopping criteria that are based on model statistics (in BQ there are several, I just added the COV criterion as an example).

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mmahsereci mmahsereci marked this pull request as draft May 7, 2024 16:39

def __init__(self, eps: float, delay: int = 1) -> None:

if delay < 1:

Choose a reason for hiding this comment

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

Suggested change
if delay < 1:
if delay < 1 or not isinstance(delay, int):

because if you pass float('inf'), then you will have an infinite loop right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants