-
Notifications
You must be signed in to change notification settings - Fork 5
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
Use GaussianRandomFields
to calculate optimal proposal covariance
#216
Use GaussianRandomFields
to calculate optimal proposal covariance
#216
Conversation
Turns out I had not being using my local development version of ParticleDA.jl/test/runtests.jl Line 296 in c278bbc
ParticleDA.jl/test/runtests.jl Line 366 in c278bbc
to reflect that The optimal filter integration test also fails due to mismatch from the reference values. If there was a mismatch between the covariance being assumed in the state noise generation and optimal proposal distribution implementation that would possibly explain this. I'll check whether the values we are getting from the |
Accounts for fix to covariance function definition in optimal proposal
I've updated the tests to fix the problems described above, plus adjusting some manual covariance function definitions in I also noticed what I think is a bug in |
test/optimal_filter_validation.yaml
Outdated
@@ -11,4 +11,4 @@ model: | |||
obs_noise_std: 0.01 | |||
sigma: 1.0 | |||
lambda: 1.0 | |||
nu: 2.5 | |||
nu: 0.5 |
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.
Any particular reason for changing this?
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.
This is what I was referring to with
to be consistent with the Matern covariance function being used (specifically using the simplified form of the Matern covariance for ν=0.5 as the test case)
As the Matern covariance function is particularly simple for nu=0.5
I changed to using this value so that in the optimal_filter_validation.jl
definition of R
ParticleDA.jl/test/optimal_filter_validation.jl
Lines 99 to 105 in 1f3e8e3
function R(x::Float64, y::Float64, th) | |
Cov = (th.s^2)*exp(-norm([x, y])/th.l) | |
return(Cov) | |
end |
we can just use a simple exponential expression rather than having to use
SpecialFunctions
to load the gamma
and besselk
functions. It might still be better to do this though I guess from a robustness standpoint so that the test is valid for any value of nu
. Or at least I should perhaps put a comment in the YAML file to make clear this value shouldn't be changed?
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 can just use a simple exponential expression rather than having to use
SpecialFunctions
to load thegamma
andbesselk
functions.
If the worry is to load another package during the tests, SpecialFunctions
is already pulled in by GaussianRandomFields
, so it isn't much more work.
It might still be better to do this though I guess from a robustness standpoint so that the test is valid for any value of
nu
.
I think that'd be better, yes.
Or at least I should perhaps put a comment in the YAML file to make clear this value shouldn't be changed?
At very least this, yes, please.
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
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.
Looks great to me, after deciding what value of nu
to use in the tests
test/optimal_filter_validation.jl
Outdated
Cov = (th.s^2)*exp(-(abs(x)+abs(y))/(2.0*th.l)) | ||
Cov = (th.s^2)*exp(-hypot(x, y) / th.l) |
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.
These two expressions don't look equal, was this an intentional correction?
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.
See discussion above #216 (comment) 😉
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.
Ah! I didn't notice the resolved discussion
# Covariance between observations R_22, from equationd 3-4 in in Dietrich & Newsam 96 | ||
# TODO: Ask Alex why we add sigma^2 on the diagonal | ||
function covariance_stations!(cov::AbstractMatrix{T}, grid::Grid, stations::NamedTuple, noise_params::NamedTuple, std::T) where T | ||
# Covariance between observations R̅₂₂ = R₂₂ + Σ, where R₂₂ is as defined in equations 3 and 4 in | ||
# Dietrich & Newsam 96 and Σ is the observation noise covariance (see note at end of | ||
# Section 2 in Dietrich & Newsam 1996) | ||
function covariance_stations!(cov::AbstractMatrix{T}, grid::Grid, stations::NamedTuple, covariance_structure::IsotropicCovarianceStructure{T}, std::T) where T |
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 catching and fixing the TODO 😄
I've now put the full definition of the Matern covariance function in |
Looks perfect! Thanks a lot again! |
Fixes #213 and fixes #214.
Updates the
covariance
method insrc/OptimalFilter.jl
to use theapply
method in theGaussianRandomFields
package rather than hardcoding a particular covariance function. The expected interface of a model is also slightly updated to haveget_model_noise_params
return a structure defining the parameters of the covariance function. These changes both ensure the covariance function definition used in sampling the state noise for the model and assumed in the optimal proposal distribution match (which may not currently be the case as described in #213) and means the optimal proposal distribution directly generalises to any isotropic covariance function defined in theGaussianRandomFields
package.I also updated some of the comments in
src/OptimalFilter.jl
to clarify the match to the equations / notation in Dietrich and Newsam (1996), including specifically resolving the TODO described in #214.