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
Implement options for pseudo-time stepping #243
Conversation
src/EnsembleKalmanInversions.jl
Outdated
iteration_summaries :: S | ||
resampler :: R | ||
unconstrained_parameters :: X | ||
forward_map_output :: G | ||
convergence_rate :: C | ||
pseudo_stepping :: C | ||
precomputed_matrices :: P |
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.
Recommdnation: generalize noise_covariance
to include storage of precomputed matrice, then define an interface with a few lines:
noise_covariance(eki::EnsembleKalmanInversion) = noise_covariance(eki.noise_covariance)
inverse_noise_covariance(eki::EnsembleKalmanInversion) = inverse_noise_covariance(eki.noise_covariance)
sqrt_inverse_noise_covariance(eki::EnsembleKalmanInversion) = sqrt_inverse_noise_covariance(eki.noise_covariance)
noise_covariance(Γy) = Γy
inverse_noise_covariance(Γy) = inv(Γy)
sqrt_inv_noise_covariance(Γy) = sqrt(inv(Γy))
struct PrecomputedInverseNoiseCovariance
covariance
inverse_covariance
sqrt_inverse_covariance
end
PrecomputedInverseNoiseCovariance(Γy) = PrecomputedInverseNoiseCovariance(Γy, inv(Γy), sqrt(inv(Γy)))
noise_covariance(pinc::PrecomputedInverseNoiseCovariance) = pinc.covariance
inverse_noise_covariance(pinc::PrecomputedInverseNoiseCovariance) = pinc.inverse_covariance
sqrt_inv_noise_covariance(pinc::PrecomputedInverseNoiseCovariance) = pinc.sqrt_inverse_covariance
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.
Thanks for the suggestion! I had implemented precomputed_arrays
before seeing your reply to #245. My concern is that it might confuse users who probably have no use for inverse_covariance
or sqrt_inverse_covariance
. Also, I have since decided to add Γθ
, μθ
, and inv_sqrt_Γθ
, which all correspond to EKI prior, to the list of precomputed arrays. We could add a PrecomputedPriors
object to store this information, but I don't want to drown users or developers in a jungle of made-up names and abstractions.
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.
Certainly, there's no way around using words and/or symbols to describe things. I prefer names to symbols both as a developer and user, but if others prefer Γθ
to noise_covariance
then we should go with that.
I agree that we often won't have use for inverse_covariance
. So I suggest that when we don't need it, noise_covariance
is just Matrix
, UniformScaling
, etc. When we do need it, noise_covariance
is the more complicated object PrecomputedInverseNoiseCovariance
. This will solve the problems you've brought up:
- Users aren't drowned in excess names, because the objects they build only contain the information they need.
- Developers aren't drowned in excess names, because each "noise covariance" has its own implementation. Thus you only need to read the part of the code that's specific to your problem.
@@ -41,6 +42,8 @@ mutable struct EnsembleKalmanInversion{E, I, M, O, S, R, X, G, C, F} | |||
unconstrained_parameters :: X | |||
forward_map_output :: G | |||
pseudo_stepping :: C | |||
precomputed_arrays :: P |
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.
What are "precomputed_arrays"?
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.
I agree that we should move the fixes to a new PR.
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.
It's all developer stuff; it doesn't affect any of the user scripts.
- `process`: The Ensemble Kalman process. Default: `Inversion(). | ||
|
||
- `tikhonov`: Whether to incorporate prior information in the EKI objective via Tikhonov regularization. | ||
See Chada et al. "Tikhonov Regularization Within Ensemble Kalman Inversion." SIAM J. Numer. Anal. 2020. |
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.
Can you clarify when this is used? Does this pertain to adaptive stepping (eg to a particular scheme?) or does this pertain to information stored in the iteration summaries?
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.
tikhonov
determines whether Tikhonov regularization (corresponding to the prior misfit term PseudoSteppingSchemes.jl
:
observations(eki) = eki.tikhonov ? eki.precomputed_arrays[:y_augmented] : eki.mapped_observations
obs_noise_covariance(eki) = eki.tikhonov ? eki.precomputed_arrays[:Σ] : eki.noise_covariance
inv_obs_noise_covariance(eki) = eki.tikhonov ? eki.precomputed_arrays[:inv_Σ] : eki.precomputed_arrays[:inv_Γy]
Ideally these augmented arrays should only be computed once, hence the need to store some of the precomputed_arrays
.
:η_mean_augmented => vcat(zeros(length(y)), -μθ), | ||
:Σ => Σ, | ||
:inv_Σ => inv(Σ), | ||
:inv_sqrt_Σ => inv(sqrt(Σ))) |
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.
Can you add a comment that documents what each of these properties mean? When we have a type that represents this data, we can add that to the docstring.
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.
If I were to implement an object with non-symbol attributes to store all the information in precomputed_arrays
, it would look something like this:
struct PrecomputedArrays{M, T}
inverse_observation_noise_covariance :: M
inverse_sqrt_observation_noise_covariance :: M
prior_covariance :: M
inverse_sqrt_prior_covariance :: M
prior_means :: T
augmented_mapped_observations :: T
augmented_observation_noise_mean :: M
augmented_observation_noise_covariance :: M
inverse_augmented_observation_noise_covariance :: M
inverse_sqrt_augmented_observation_noise_covariance :: M
end
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.
Can you also report which pseudo-time-stepping methods require which data?
pseudo_Δt = nothing, | ||
pseudo_stepping = eki.pseudo_stepping, | ||
covariance_inflation = 0.0, | ||
momentum_parameter = 0.0) |
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.
Alignment is off here
""" | ||
pseudo_step!(eki::EnsembleKalmanInversion; | ||
pseudo_Δt = eki.pseudo_Δt, | ||
pseudo_stepping = eki.pseudo_stepping) |
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.
Alignment is off in this docstring
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.
We just need a bit more documentation so that other developers can contribute, modify, and refactor this code in the future. Ideally, we don't use dictionaries for important properties. Something that isn't clear to me is whether precomputed_arrays
and precomputed_augmented_arrays
are needed for all pseudostepping schemes, or just some. It would seem that they can't always be needed, since they didn't exist previously.
This branch builds on
glw/pseudotime
and adds several options for pseudo-time stepping schemes in the PseudoSteppingSchemes module.To do: