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

Implement ConcatenatedVectorNormMap + develop projects/OceanBoundaryLayerParameterizations #167

Merged
merged 82 commits into from Apr 7, 2022

Conversation

adelinehillier
Copy link
Collaborator

@adelinehillier adelinehillier commented Feb 4, 2022

  1. Implements missing functionality for ConcatenatedVectorNormMap and modifies it so that it produces the square root of the norm rather than the norm.
  2. Adds a property objective_values to IterationSummary, a vector of NamedTuples (Φ1 , Φ2) indicating, for each ensemble member, the data misfit Φ1 = || Γy^(-¹/₂) * (y .- G) ||² and prior misfit Φ2 = || Γθ^(-¹/₂) * (θ .- μθ) ||², so that the magnitudes of these values can be compared. This is provides a rough sense of how strongly regularized the loss function is given the observation noise covariance.
  3. Deletes projects directory.
  4. Updates EKI API to include several adaptive time stepping options. The current scheme is renamed Default()

src/InverseProblems.jl Outdated Show resolved Hide resolved
src/InverseProblems.jl Outdated Show resolved Hide resolved
Comment on lines 14 to 20
Given forward map `G` and *unconstrained* parameters `θ`, return a
tuple `(Φ1, Φ2)` of terms in the EKI regularized objective function, where
Φ = (1/2)*(Φ1 + Φ2). Φ1 measures output misfit `|| Γy^(-¹/₂) * (y .- G(θ)) ||²` and
Φ2 measures prior misfit `|| Γθ^(-¹/₂) * (θ .- μθ) ||²`, where `y` is the observation
map, `G(θ)` is the forward map, `Γy` is the observation noise covariance, `Γθ` is
the prior covariance, and `μθ` represents the prior means. Note that `Γ^(-1/2) =
inv(sqrt(Γ))`.
Copy link
Collaborator

@navidcy navidcy Mar 19, 2022

Choose a reason for hiding this comment

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

Suggested change
Given forward map `G` and *unconstrained* parameters `θ`, return a
tuple `(Φ1, Φ2)` of terms in the EKI regularized objective function, where
Φ = (1/2)*(Φ1 + Φ2). Φ1 measures output misfit `|| Γy^(-¹/₂) * (y .- G(θ)) ||²` and
Φ2 measures prior misfit `|| Γθ^(-¹/₂) * (θ .- μθ) ||²`, where `y` is the observation
map, `G(θ)` is the forward map, `Γy` is the observation noise covariance, `Γθ` is
the prior covariance, and `μθ` represents the prior means. Note that `Γ^(-1/2) =
inv(sqrt(Γ))`.
Given forward map `G` and *unconstrained* parameters `θ`, return a
tuple `(Φ₁, Φ₂)` of terms in the EKI regularized objective function, where
`Φ = ½(Φ₁ + Φ₂)`. `Φ₁` measures output misfit `|| Γy^(-½) * (y - G(θ)) ||²` and
`Φ₂` measures prior misfit `|| Γθ^(-½) * (θ - μθ) ||²`, where `y` is the observation
map, `G(θ)` is the forward map, `Γy` is the observation noise covariance, `Γθ` is
the prior covariance, and `μθ` represents the prior means. Note that `Γ^(-½) =
inv(Γ)`.

src/iteration_summary.jl Outdated Show resolved Hide resolved
src/iteration_summary.jl Outdated Show resolved Hide resolved
Copy link
Collaborator

@navidcy navidcy left a comment

Choose a reason for hiding this comment

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

mostly aesthetic
do we need LaTeXStrings as main package dep?

adelinehillier and others added 17 commits March 19, 2022 19:40
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
@adelinehillier
Copy link
Collaborator Author

All tests pass and the changes in this PR are fairly minimal. @glwagner would you mind re-reviewing?

@navidcy navidcy requested a review from glwagner March 28, 2022 05:48
parameters :: P # constrained
ensemble_mean :: M # constrained
ensemble_cov :: C # constrained
ensemble_var :: V
mean_square_errors :: E
objective_values :: O
Copy link
Member

Choose a reason for hiding this comment

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

Should we have two properties, output_misfit and prior_misfit?

@@ -238,7 +239,7 @@ end
# it's not adaptive
adaptive_step_parameters(::Nothing, Xⁿ, Gⁿ, y, Γy, process) = step_parameters(X, G, y, Γy, process)

function step_parameters(eki::EnsembleKalmanInversion, convergence_rate)
function step_parameters(eki::EnsembleKalmanInversion, convergence_rate, adaptive_step_parameters)
Copy link
Member

Choose a reason for hiding this comment

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

I didn't quite grasp the logic here. This implements a feature that allows users to pass a function adaptive_step_parameters that overrides the adaptive_step_parameters in the source?

Should we use multiple dispatch to control the behavior of adaptive_step_parameters instead? This would mean generalizing the convergence_rate parameter (which is now either nothing or Float64) to something more general (eg adaptivity...)

If we stick with this design, then I guess we will find ourselves providing alternative adaptive_step_parameters functions that users can import into their scripts, and pass into iterate. But those new functions will ignore the property convergence_rate (and may require closing any additional parameters of those alternative functions... ?) That might get a little tricky.

As is, without any alternatives, only advanced users who are ready to rewrite functions like

function adaptive_step_parameters(convergence_rate, Xⁿ, Gⁿ, y, Γy, process)
can use this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, this was intended to be a temporary hack. I'm all for implementing a good time-stepping API in the next PR. I'll go ahead and merge.

Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

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

We can merge this, though it looks like we will need to implement an API for utilizing different adaptive time-stepping methods in the future.

@adelinehillier adelinehillier merged commit 4c55e2f into main Apr 7, 2022
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

3 participants