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

Add failsafe EKI with sample_succ_gauss handler. #112

Merged
merged 1 commit into from
Mar 3, 2022
Merged

Add failsafe EKI with sample_succ_gauss handler. #112

merged 1 commit into from
Mar 3, 2022

Conversation

ilopezgp
Copy link
Contributor

@ilopezgp ilopezgp commented Feb 25, 2022

  • Adds a failure handler struct to all EnsembleKalmanProcess objects.
  • Implements the IgnoreFailures failure handler for all EnsembleKalmanProcesses, which maintains the former behavior of the algorithms (i.e, do nothing upon failure).
  • Implements the SampleSuccGauss failure handler for the Inversion process, which advances the failed ensemble by sampling from an empirical gaussian formed by the successful ensemble.
  • Failure may be defined by the user by passing the indices of failed particles to update_ensemble!. If nothing is passed, failures are taken to be those forward model evaluations that contain NaNs.
  • Adds tests of new failure handlers.

In addition:

  • Update equations are encapsulated outside of update_ensemble!, and into functions ..._update.
  • Transposes in the Sampler are encapsulated to the failsafe_update call. This allows to work with data as columns in the Sampler's version of update_ensemble!.

@ilopezgp ilopezgp force-pushed the failsafe branch 2 times, most recently from 36edd79 to 13ea9db Compare February 25, 2022 07:49
@ilopezgp ilopezgp marked this pull request as ready for review February 25, 2022 07:49
src/EnsembleKalmanInversion.jl Outdated Show resolved Hide resolved
src/EnsembleKalmanInversion.jl Outdated Show resolved Hide resolved
@ilopezgp
Copy link
Contributor Author

ilopezgp commented Mar 1, 2022

This works for me locally, any idea what the error means? @charleskawczynski @jakebolewski

@charleskawczynski
Copy link
Member

I'm getting a seg fault, I can try taking a closer look tomorrow

@ilopezgp
Copy link
Contributor Author

ilopezgp commented Mar 1, 2022

I'm getting a seg fault, I can try taking a closer look tomorrow

Thanks! That would be helpful. Are you getting seg fault locally? All the tests run for me locally (MacOS), but fail here

@charleskawczynski
Copy link
Member

Are you getting seg fault locally?

Yep

@charleskawczynski
Copy link
Member

I think this is an upstream issue. I'm still not sure from where, but it might be productive to try using more recent versions to confirm if that's the case.

@ilopezgp
Copy link
Contributor Author

ilopezgp commented Mar 1, 2022

I think this is an upstream issue. I'm still not sure from where, but it might be productive to try using more recent versions to confirm if that's the case.

Upstream from what packages? We should be up to date with all packages since the CompatHelper has not recommended any new PR.

@charleskawczynski
Copy link
Member

Upstream from what packages? We should be up to date with all packages since the CompatHelper has not recommended any new PR.

Our compat entries allow for the latest version of SCS.jl, but we're not using the latest version of SCS.jl (the resolver gives us 0.9.something).

@ilopezgp
Copy link
Contributor Author

ilopezgp commented Mar 2, 2022

Upstream from what packages? We should be up to date with all packages since the CompatHelper has not recommended any new PR.

Our compat entries allow for the latest version of SCS.jl, but we're not using the latest version of SCS.jl (the resolver gives us 0.9.something).

Thanks @charleskawczynski !! I think we merged a compat change that did not test the changes from SCS. I tested SCS 0.9 and it leads to segfaults. SCS 1.0 cannot be used yet because of compat issues with Convex.jl, although I think they will be updating that package to v0.15 very soon. Version 0.8 works.

@ilopezgp ilopezgp force-pushed the failsafe branch 2 times, most recently from c983972 to 958aa26 Compare March 2, 2022 21:14
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.

Thanks this looks great! i think all my comments are now addressed.

For the sake of codecov - could you add something to test the warnings that are thrown when 50% fail, or singular covariance? I think you could maybe see if the following are nonempty:
@test_logs(:warn,) update_ensemble(...) or
@test_logs(:info,) update_ensemble(...)

Then I'm happy to merge

@ilopezgp
Copy link
Contributor Author

ilopezgp commented Mar 2, 2022

Thanks this looks great! i think all my comments are now addressed.

For the sake of codecov - could you add something to test the warnings that are thrown when 50% fail, or singular covariance? I think you could maybe see if the following are nonempty: @test_logs(:warn,) update_ensemble(...) or @test_logs(:info,) update_ensemble(...)

Then I'm happy to merge

Sounds good. I will add these as unit tests of the respective functions to be a bit cleaner.

@ilopezgp
Copy link
Contributor Author

ilopezgp commented Mar 2, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 3, 2022

Build succeeded:

@bors bors bot merged commit 4da2e84 into main Mar 3, 2022
@bors bors bot deleted the failsafe branch March 3, 2022 00:12
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.

3 participants