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
LoopStateto 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?
LoopStateclass in the core package. For example theloop_state.updatemethod could also request the models in addition to X and Y and then store the relevant statistics. But this is a bit tricky since:OuterLoopwhereloop_state.update()is called (only the model updaters are accessible which, in their basic form, do not even necessarily contain a model).updateis currently called).update_model_statsto the coreLoopStateclass which is called in the loop after the models are updated, similar to howloop_state.updateis 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)
OuterLoopalready contains a method_update_models. I added another method called_update_loop_statewhich 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 inLoopStatebut also inOuterLoopthat both need to be overridden in specific packages if needed. So it is less principled since a method inOuterLoopsomehow 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.