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 documentation for ensemble Kalman sampler #22

Merged
merged 10 commits into from
May 5, 2021
Merged

Conversation

bielim
Copy link
Contributor

@bielim bielim commented Apr 19, 2021

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

Notes:

  • 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.
  • I was aiming for a documentation that is similar to what @ilopezgp wrote for ensemble Kalman inversion (PR EKI documentation #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 Consistency in variable names #13.

@bielim bielim added the documentation Improvements or additions to documentation label Apr 19, 2021
@bielim bielim requested a review from odunbar April 19, 2021 22:58
@bielim bielim self-assigned this Apr 19, 2021
@odunbar
Copy link
Collaborator

odunbar commented Apr 20, 2021

I had a look at the Oceaninanigans docs raw files and they use double `` latex math `` to close the a latex expression. It doesn't render latex in markdown, but the documenter renders it in html.

The full line equations can be done with either ```math <newline> \begin{equation} abc \end{equation} <newline> ``` or you can use proper latex environments

Links:

  1. (docs page) https://clima.github.io/OceananigansDocumentation/stable/physics/navier_stokes_and_tracer_conservation/
  2. (un-rendered markdown) https://github.com/CliMA/Oceananigans.jl/blob/master/docs/src/physics
  3. (raw file) https://raw.githubusercontent.com/CliMA/Oceananigans.jl/master/docs/src/physics/navier_stokes_and_tracer_conservation.md

Regarding notation, if you could come to consensus between yourself and @ilopezgp, I am happy to pitch in here too, I would argue for consistency with the EKI/CES papers, as these will be the go-to references and I believe are self consistent

@odunbar
Copy link
Collaborator

odunbar commented Apr 26, 2021

Hi Melanie - You need to add your doc page into the Make.jl file, just add something like

"Ensemble Kalman Sampler" => "ensemble_kalman_sampler.md"

into the dictionary

@bielim
Copy link
Contributor Author

bielim commented Apr 27, 2021

Hi Melanie - You need to add your doc page into the Make.jl file, just add something like

"Ensemble Kalman Sampler" => "ensemble_kalman_sampler.md"

into the dictionary

Done, thank you!

@odunbar
Copy link
Collaborator

odunbar commented Apr 28, 2021

Hi Melanie, some comments as i see them - some similarities with Ignacios EKI PR:

  1. It would be good to provide the update equation for theta, as otherwise why do we provide the formulation and notation for the first section
  2. Your bullet points go 1. 1. 1. (perhaps you are used to rendering of .md files)
  3. I think you can drop the details about the prior (as you don't actually use these later). I would just include the ParameterDistributionStorage module and put e.g # required to create the prior , then start with the line prior = ParameterDistributions(...)
  4. Maybe remove the comment about not needing the forward model, I wouldn't expect any optimization/sampling scheme to need information about the forward model in it's construction
  5. Please add the unconstrained transformation map as seen in e.g the snippet of Returning constrained parameters #24 in the EKI loop code
  6. I like the solution section at the end! (I have asked for something like this in the EKI docs) I would definitely impress that the solution of this method is a Gaussian distribution, and you can extract the mean and covariance with the empirical statistics from the final ensemble. You can then say that the "optimal" parameter with regards to calibration would be given by the mean.

@odunbar
Copy link
Collaborator

odunbar commented May 5, 2021

Hi Melanie - this looks great, notation is unified with Ignacios as far as i can see!

Some typos:

  • Capital \Theta instead of small \theta
  • extra bracket pair in H(\Psi(\phi_n...)))
  • # evaluate forward map comment should be a line up
  • In "Solution" I would drop the "using Statistics" - for a start you don't do this earlier in the script when you use mean and cov before

@bielim
Copy link
Contributor Author

bielim commented May 5, 2021

Hi Melanie - this looks great, notation is unified with Ignacios as far as i can see!

Some typos:

  • Capital \Theta instead of small \theta

I adopted the notation in Cleary et al., where capital \Theta is used to denote the set of all ensemble particles - the docs now clarify this.

  • extra bracket pair in H(\Psi(\phi_n...)))

Fixed, thanks!

  • # evaluate forward map comment should be a line up

Fixed, thanks!

  • In "Solution" I would drop the "using Statistics" - for a start you don't do this earlier in the script when you use mean and cov before

I left it in there because personally I like it when people are explicit about the packages that are used to run the code. The get_mean and get_cov I use in a previous code snippet actually come from ParameterDistributionStorage, which is included as well :-)

@odunbar
Copy link
Collaborator

odunbar commented May 5, 2021

LGTM! Merge away

@bielim
Copy link
Contributor Author

bielim commented May 5, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented May 5, 2021

Build succeeded:

@bors bors bot merged commit 86ee7c9 into main May 5, 2021
@bors bors bot deleted the mb/eks_docs branch May 5, 2021 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants