BQ stopping criteria that depend on model statistics #463
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 inLoopState
(only X and Y are).So, how can stopping criteria access model statistics to assess stopping?
LoopState
class in the core package. For example theloop_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:OuterLoop
whereloop_state.update()
is called (only the model updaters are accessible which, in their basic form, do not even necessarily contain a model).update
is currently called).update_model_stats
to the coreLoopState
class which is called in the loop after the models are updated, similar to howloop_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)
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 inLoopState
but also inOuterLoop
that both need to be overridden in specific packages if needed. So it is less principled since a method inOuterLoop
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.