Corrected EKS implementation #83
Conversation
Project.toml
Outdated
| @@ -9,6 +9,7 @@ Distributions = "31c24e10-a181-5473-b8eb-7969acd0382f" | |||
| DocStringExtensions = "ffbed154-4ef7-542d-bbb7-c09d3a79fcae" | |||
| GaussianProcesses = "891a1506-143c-57d2-908e-e1f8e92e6de9" | |||
| LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e" | |||
| Plots = "91a5bcdd-55d7-5caf-9e0b-520d859cae80" | |||
There was a problem hiding this comment.
Do we want to keep this dependency on Plots or do something else? @jakebolewski @bielim @odunbar
There was a problem hiding this comment.
I think a good thing to do with plots (for this PR) is described here https://discourse.julialang.org/t/add-package-to-project-toml-when-needed-only-for-testing/20552/3
In our setting I think it amounts to the following
- Add
Testto the Project.toml - Add (manually) new sections
[extras]and[targets]into Project.toml, - Copy the
Testline and thePlotslines into[extras] - in the
[targets]section puttest = ["Test","Plots"]
Then when you call Pkg.test("MyPackage"), (not run runtest.jl directly) it will also load the Plots package.
NB I haven't done this before but hopefully it will work
There was a problem hiding this comment.
Let's try this, yes (this is what worked for @glwagner in Oceananigans, right?)
Codecov Report
@@ Coverage Diff @@
## master #83 +/- ##
==========================================
+ Coverage 77.32% 77.84% +0.51%
==========================================
Files 7 7
Lines 494 492 -2
==========================================
+ Hits 382 383 +1
+ Misses 112 109 -3
Continue to review full report at Codecov.
|
| u_mean = mean(u', dims=2) | ||
| # g_mean: N_params x 1 | ||
| u_mean = mean(u', dims=2) | ||
| # g_mean: N_params x 1 | ||
| g_mean = mean(g', dims=2) | ||
| # g_cov: N_params x N_params | ||
| g_cov = cov(g, corrected=false) |
There was a problem hiding this comment.
corrected=false is a default and can be removed
There was a problem hiding this comment.
According to the docs, the default is actually corrected=true, so we need to explicitly set the flag to false here.
test/EKP/runtests-linear-model.jl
Outdated
| # EKI provides a solution closer to the ordinary Least Squares estimate | ||
| @test norm(ols_mean - eki_final_result) < norm(ols_mean - eks_final_result) | ||
| # EKS provides a solution closer to the posterior mean | ||
| @test norm(posterior_mean - eks_final_result) < norm(posterior_mean - eki_final_result) |
There was a problem hiding this comment.
Are these tests always going to be true? Or just in this test case, e.g this depends on realizations of the data you provide etc?
There was a problem hiding this comment.
As discussed, we will improve the tests in a future PR, this one is mainly about implementing the EKS.
test/EKP/runtests-linear-model.jl
Outdated
| # In words: the ensemble covariance is still a bit ill-dispersed since the | ||
| # algorithm employed still does not include the correction term for finite-sized | ||
| # ensembles. | ||
| @test abs(sum(diag(posterior_cov_inv\cov(eksobj.u[end]))) - n_par) > 1e-5 |
There was a problem hiding this comment.
This is maybe more of a feature of the current algorithm as opposed to a test. e.g are we actually implying we fail this test if the covariance is not ill-dispersed?
There was a problem hiding this comment.
As discussed, we will improve the tests in a future PR, this one is mainly about implementing the EKS.
There was a problem hiding this comment.
I suggest that we make runtests-linear-model.jl the new runtests.jl and delete the old one, since runtests-linear-model.jl is basically an improved version of the example in runtests.jl. But it needs some more @test statements - in general it would be good if all functions, objects and object attributes in EKP.jl were "touched" at least once in these tests.
Project.toml
Outdated
| @@ -9,6 +9,7 @@ Distributions = "31c24e10-a181-5473-b8eb-7969acd0382f" | |||
| DocStringExtensions = "ffbed154-4ef7-542d-bbb7-c09d3a79fcae" | |||
| GaussianProcesses = "891a1506-143c-57d2-908e-e1f8e92e6de9" | |||
| LinearAlgebra = "37e2e46d-f89d-539d-b4ee-838fcccc9c8e" | |||
| Plots = "91a5bcdd-55d7-5caf-9e0b-520d859cae80" | |||
There was a problem hiding this comment.
Let's try this, yes (this is what worked for @glwagner in Oceananigans, right?)
|
bors r+ |
83: Corrected EKS implementation r=bielim a=agarbuno This kills a bug in the EKS method and add a linear example as a test. Co-authored-by: Alfredo Garbuno <alfredo.garbuno@itam.mx> Co-authored-by: odunbar <47412152+odunbar@users.noreply.github.com> Co-authored-by: Melanie <melanie@charney.bieli.email>
|
Build failed: |
|
bors r+ |
83: Corrected EKS implementation r=jakebolewski a=agarbuno This kills a bug in the EKS method and add a linear example as a test. Co-authored-by: Alfredo Garbuno <alfredo.garbuno@itam.mx> Co-authored-by: odunbar <47412152+odunbar@users.noreply.github.com> Co-authored-by: Melanie <melanie@charney.bieli.email>
|
Build failed: |
…mentation (#84) Also, restructure `Project.toml` such that Plots.jl is only required for running tests. Co-authored-by: Melanie <melanie@charney.bieli.email>
…old one They both essentially test the same thing, but `runtests-linear-model.jl` uses a better example problem. Recommendation: In general, unit tests should also test "trivial code" (e.g., getters and setters) and check for things like type stability. The ones we currently have focus on functionality and could use some more of these simpler checks - ideally, all functions and objects should get "touched" at least once. A future PR to expand the tests in this spirit is warranted.
jakebolewski
force-pushed
the
ag/eks-implementation
branch
from
962c54c to
660f597
Compare
|
bors r+ |
|
Build succeeded: |
This kills a bug in the EKS method and add a linear example as a test.