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

EKI documentation #21

Merged
merged 3 commits into from
May 4, 2021
Merged

EKI documentation #21

merged 3 commits into from
May 4, 2021

Conversation

ilopezgp
Copy link
Contributor

@ilopezgp ilopezgp commented Apr 16, 2021

This PR adds documentation on the Ensemble Kalman Inversion process, as specified by the documentation guidelines #15. I have also fixed a couple of inconsistencies in the docs regarding the name of the existing modules.

@odunbar
Copy link
Collaborator

odunbar commented Apr 20, 2021

Hi Ignacio - looks good so far!

I think we should write down the update that takes place within the update_ensemble, in particular the where the additive noise comes in. This is quite important for working with deterministic/non-deterministic forward model, (There will be an upcoming PR about this).

Also the "rule of thumb" for ensemble size - I think this should be removed. Perhaps you could add a warning box just saying that if the ensemble size < parameter dimension, one requires regulariziation techniques (which are not yet implemented).

@ilopezgp
Copy link
Contributor Author

Hi Ignacio - looks good so far!

I think we should write down the update that takes place within the update_ensemble, in particular the where the additive noise comes in. This is quite important for working with deterministic/non-deterministic forward model, (There will be an upcoming PR about this).

Also the "rule of thumb" for ensemble size - I think this should be removed. Perhaps you could add a warning box just saying that if the ensemble size < parameter dimension, one requires regulariziation techniques (which are not yet implemented).

Adding the update definition is a good idea, I will do that. I would like to keep a rule of thumb for people starting out with the tools. If this package finds relatively spread use, we cannot expect everyone to read the papers thoroughly before using it. Are there better rules of thumb that you can think of? I think the limit ensemble size = parameter dimension is already not too performant.

@odunbar
Copy link
Collaborator

odunbar commented Apr 20, 2021

Are there better rules of thumb that you can think of?

I'm not sure there is one, it's problem specific. The only thing that's independent is that you at least 1 for each dimension or you get the degeneracy issues to solve. As you said in practice we usually want "some" in each dimension though. Ok let's leave this for now, I appreciate that some people will have no intuition for ensemble size.

@odunbar
Copy link
Collaborator

odunbar commented Apr 27, 2021

Thanks! this looks great! Some comments in order of reading the page.

  1. State that the ensemble method is "finished" where the ensemble acheives sufficient consensus/collapse and we define the solution to be the mean value of the final iteration.

  2. Subheading: creating the EKI Object

  3. The prior object really is required in our method, it felt like you implied it was optional?

  4. The pseudo-code for creating the ensemble: state ParameterDistributionStorage is included to define the prior and parameter transformations (could link to the existing docs page?)

  5. I'd remove the note about the Inversion() not having input arguments

  6. The pseudo-code for the update: It would great if you could also put in the intermediary transform_unconstrained_to_constrained(..) line that's required when we have a constraint at the moment - I have already written this out actually in issue Returning constrained parameters #24, just put # if working with parameter constraints after it - in reality we shoud add it all the time as it will be the identity map if no constraints are defined.

  7. G = H \circ \Psi \circ T_{u \mapsto c}

@ilopezgp ilopezgp marked this pull request as ready for review April 29, 2021 02:33
@odunbar
Copy link
Collaborator

odunbar commented Apr 30, 2021

Almost there!

  1. Can you remove the sigma in the final box, as only the mean yields the solution here
  2. Can you add that G = H \circ \Psi \circ T_{u \mapsto c}, in the theory section
  3. Can you change the pseudocode later on to reflect this. e.g one idea might be to not call the model we apply G but rather call it H_circ_Psi, and if you like, rather than physical_params you could call it T\theta or something!

After that it looks good to me, @bielim if you can also be consistent with Ignacios notation there too that would be great!

@ilopezgp
Copy link
Contributor Author

ilopezgp commented May 3, 2021

Almost there!

  1. Can you remove the sigma in the final box, as only the mean yields the solution here
  2. Can you add that G = H \circ \Psi \circ T_{u \mapsto c}, in the theory section
  3. Can you change the pseudocode later on to reflect this. e.g one idea might be to not call the model we apply G but rather call it H_circ_Psi, and if you like, rather than physical_params you could call it T\theta or something!

After that it looks good to me, @bielim if you can also be consistent with Ignacios notation there too that would be great!

Sounds good, I made the executive decision of defining \phi as T(\theta) for clarity of explanation. Hope that's ok.

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.

LGTM!

It occurs to me in the future we should have a separate "The inverse problem" section, so forward model discussion does not have to be in the EKI page. But I am happy with this at the moment,

Please merge it in

@ilopezgp
Copy link
Contributor Author

ilopezgp commented May 4, 2021

LGTM!

It occurs to me in the future we should have a separate "The inverse problem" section, so forward model discussion does not have to be in the EKI page. But I am happy with this at the moment,

Please merge it in

I completely agree. I think we can restructure it in the future.

@ilopezgp
Copy link
Contributor Author

ilopezgp commented May 4, 2021

bors r+

bors bot added a commit that referenced this pull request May 4, 2021
21: EKI documentation r=ilopezgp a=ilopezgp

This PR adds documentation on the Ensemble Kalman Inversion process, as specified by the documentation guidelines #15. I have also fixed a couple of inconsistencies in the docs regarding the name of the existing modules.

Co-authored-by: Ignacio <[email protected]>
Co-authored-by: ilopezgp <[email protected]>
@bors
Copy link
Contributor

bors bot commented May 4, 2021

Build failed:

@ilopezgp
Copy link
Contributor Author

ilopezgp commented May 4, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented May 4, 2021

Build succeeded:

@bors bors bot merged commit 76c6cde into main May 4, 2021
@bors bors bot deleted the il/docs branch May 4, 2021 20:45
bors bot added a commit that referenced this pull request May 5, 2021
22: [WIP] Add documentation for ensemble Kalman sampler r=bielim a=bielim

This PR adds documentation for the ensemble Kalman sampler and fills a part of the documentation needs identified in #15.

Notes:

- [x] I couldn't figure out how to render greek letters and math equations in the github markdown. If anyone knows how to do this, please let me know.
- [x] I was aiming for a documentation that is similar to what @ilopezgp wrote for ensemble Kalman inversion (PR #21), as I think ideally the documentation should look "all of a piece" in the end. There are currently some differences in the notation between these two PRs, e.g. in the notation for the dimension of parameter and data space. These are just details, but I think it would be good to establish conventions for these kinds of things, also with regard to issue #13.

Co-authored-by: Melanie <[email protected]>
Co-authored-by: odunbar <[email protected]>
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.

2 participants