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

[WIP] Add implementation of sparse EKI #70

Merged
merged 27 commits into from
Dec 21, 2021
Merged

[WIP] Add implementation of sparse EKI #70

merged 27 commits into from
Dec 21, 2021

Conversation

jinlong83
Copy link
Contributor

This PR adds the option of sparse EKI to Ensemble Kalman Inversion process:

  • Added the option of sparse EKI to EnsembleKalmanInversion.jl
  • Added an example (LossMinimization/loss_minimization_sparse_eki.jl)

@odunbar
Copy link
Collaborator

odunbar commented Nov 5, 2021

Hi Jinlong, the way we have been adding different methods is by keeping them separated rather than using flags. As the sparsity preserving EKI is sufficiently different from EKI maybe we could do it in this case. (You can use the Sampler type as a guide (in EnsembleKalmanSampler.jl))

Could you create a new file SparseEnsembleKalmanInversion.jl and inside you can add

  • a Type called SparseInversion which takes in all the "Sparse method only" parameters
  • an update_ensemble(ekp::EnsembleKalmanProcess{FT, IT, SparseInversion{FT}}, ...) method where you put in the sparse update

We can call to chat more about this

@odunbar
Copy link
Collaborator

odunbar commented Nov 8, 2021

Note I got this to run by first

conda install -c conda-forge cvxopt

and then in the project base

julia --project
]
add CVXOPT

then in the example directory

julia --project loss_minimization_sparse.jl

@ilopezgp
Copy link
Contributor

ilopezgp commented Nov 10, 2021

Is there a way to implement the Sparse EKI without PyCall? Python dependencies are a headache, and should be avoided if we can. This might be useful: https://jump.dev/Convex.jl/stable/

@jinlong83
Copy link
Contributor Author

Is there a way to implement the Sparse EKI without PyCall? Python dependencies are a headache, and should be avoided if we can. This might be useful: https://jump.dev/Convex.jl/stable/

I have updated the implementation to avoid PyCall. It seems that Convex.jl is slightly less stable than CVXOPT, so I also updated the default settings of sparse EKI parameters in the example.

Project.toml Outdated Show resolved Hide resolved
@odunbar
Copy link
Collaborator

odunbar commented Nov 18, 2021

I forgot to mention the unit tests. Your code is untested

Could you transfer the sparse example you created, and place it in the file test/EnsembleKalmanProcessModule/runtests.jl (remove all the literate comments etc)

inside you then add some@test statements - which are like fancy asserts. You can use the the EnsembleKalmanInversion testing done in the file as aguide, (and maybe add something to test the sparsity property?)

To checkthe testing offline you go to the base and do

julia --project
using Test
include("test/runtests.jl")

Copy link
Collaborator

@odunbar odunbar left a comment

Choose a reason for hiding this comment

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

Excellent addition. LGTM

Please add a check for the L^1 norm in the sparse eki loop.

If you could create a docs PR soon, that adds a page linking to your paper / what the sparsity means in this context etc. that would be awesome.

src/SparseEnsembleKalmanInversion.jl Outdated Show resolved Hide resolved
src/SparseEnsembleKalmanInversion.jl Outdated Show resolved Hide resolved
src/SparseEnsembleKalmanInversion.jl Outdated Show resolved Hide resolved
src/SparseEnsembleKalmanInversion.jl Outdated Show resolved Hide resolved
src/SparseEnsembleKalmanInversion.jl Outdated Show resolved Hide resolved
src/SparseEnsembleKalmanInversion.jl Outdated Show resolved Hide resolved
Copy link
Contributor

@ilopezgp ilopezgp left a comment

Choose a reason for hiding this comment

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

After my Threads.@threads question has been addressed by someone more experienced with Julia, LGTM.

src/SparseEnsembleKalmanInversion.jl Show resolved Hide resolved
@jakebolewski
Copy link
Contributor

bors r+

@bors
Copy link
Contributor

bors bot commented Dec 21, 2021

Build succeeded:

@bors bors bot merged commit d176e8d into main Dec 21, 2021
@bors bors bot deleted the jw/sparse_eki branch December 21, 2021 20:25
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.

4 participants