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 documentation for unscented Kalman inversion #25

Merged
merged 1 commit into from
Jun 21, 2021
Merged

Add documentation for unscented Kalman inversion #25

merged 1 commit into from
Jun 21, 2021

Conversation

Zhengyu-Huang
Copy link
Contributor

@Zhengyu-Huang Zhengyu-Huang commented May 4, 2021

This PR adds documentation for the unscented Kalman sampler

  • Need to check equations
  • Need to check visualization

@odunbar
Copy link
Collaborator

odunbar commented May 12, 2021

Could you add the your UKI file into Make.jl (in the same format as the Ensemble Kalman Inversion file) in the docs/ directory, and push this to the PR so I can see the rendered docs on github?

Cheers

@odunbar
Copy link
Collaborator

odunbar commented May 18, 2021

Thanks Daniel, Some comments as I see them

  1. Paragraph 2 "It is favorable for the following reasons" -> "The UKI algorithm has the following properties". I would describe them as features, as we are not trying to advertise the methods as such, just say what they do (though you can still say why it is an advantage to have a certain property)
  2. "Efficiency: ..." I would maybe say " UKI has a given ensemble size" ,
  3. "No ensemble collapse" should be in the uncertainty quantification bullet, not robustness
  4. "No early stopping criteria" - what is the stopping criteria then? I think in this codebase you still pre-specify it? rather than "iterate until converged with this tolerance"
  5. You state that there is no empirical inflation, but you do have regularization parameters e.g alpha that is used to modify the covariance - in some sense is this not a form of inflation?
  6. At the start of the Algorithm, state what the "free" parameters are (which must be chosen) Then later on change the "Free parameters" section to "Choosing the free parameters"
  7. Unify notation with y, \mathcal{G}, eta, \Gamma_y
  8. In the Algorithm section, from the point of view of the user, could we present it as "Analysis at sigma-ensemble" followed by "Generate new sigma-ensemble" (which includes the prediction step) as it is currently written in the code?
  9. Algorithm section: Remove the paragraph after the "Cholesky factor of C." we discuss the solution later
  10. Free parameters section: Are there actually defaults or just recommended values for the parameters? it would be useful to state the choices as "Set to X as default", then change the default if ... Also would be good to write in the names in the code for the parameters too here e.g \alpha_reg
  11. typo's errror ->error
  12. You need to define Lambda before \Sigma_\omega, as it depends on it.
  13. Is \Sigma_\eta = \Gamma_y?
  14. Updating the ensemble: the code snippet should be the same as that of EKS. Could you also copy - paste the "creating the forward map" section in from EKS too, needed for the parameter transformation stuff. [just for now - this will be addressed in a later PR].
  15. Could you add a "solution" section as in EKS (I think it is likely to be the same). You could also talk about how to determine convergence here

Thanks for all the work! Let me know if you have some questions

@Zhengyu-Huang
Copy link
Contributor Author

Thanks Daniel, Some comments as I see them

  1. Paragraph 2 "It is favorable for the following reasons" -> "The UKI algorithm has the following properties". I would describe them as features, as we are not trying to advertise the methods as such, just say what they do (though you can still say why it is an advantage to have a certain property)
  2. "Efficiency: ..." I would maybe say " UKI has a given ensemble size" ,
  3. "No ensemble collapse" should be in the uncertainty quantification bullet, not robustness
  4. "No early stopping criteria" - what is the stopping criteria then? I think in this codebase you still pre-specify it? rather than "iterate until converged with this tolerance"
  5. You state that there is no empirical inflation, but you do have regularization parameters e.g alpha that is used to modify the covariance - in some sense is this not a form of inflation?
  6. At the start of the Algorithm, state what the "free" parameters are (which must be chosen) Then later on change the "Free parameters" section to "Choosing the free parameters"
  7. Unify notation with y, \mathcal{G}, eta, \Gamma_y
  8. In the Algorithm section, from the point of view of the user, could we present it as "Analysis at sigma-ensemble" followed by "Generate new sigma-ensemble" (which includes the prediction step) as it is currently written in the code?
  9. Algorithm section: Remove the paragraph after the "Cholesky factor of C." we discuss the solution later
  10. Free parameters section: Are there actually defaults or just recommended values for the parameters? it would be useful to state the choices as "Set to X as default", then change the default if ... Also would be good to write in the names in the code for the parameters too here e.g \alpha_reg
  11. typo's errror ->error
  12. You need to define Lambda before \Sigma_\omega, as it depends on it.
  13. Is \Sigma_\eta = \Gamma_y?
  14. Updating the ensemble: the code snippet should be the same as that of EKS. Could you also copy - paste the "creating the forward map" section in from EKS too, needed for the parameter transformation stuff. [just for now - this will be addressed in a later PR].
  15. Could you add a "solution" section as in EKS (I think it is likely to be the same). You could also talk about how to determine convergence here

Thanks for all the work! Let me know if you have some questions

Thanks for these suggestions!

@odunbar
Copy link
Collaborator

odunbar commented Jun 4, 2021

Thanks Daniel!

A couple more points

  1. "Updating the ensemble" section - you refer to eks not uki in the code snippet
  2. I would still prefer a presentation of the algorithm with the Analysis first (before prediction). If you don't want to do this, could you instead add a sentence saying "for implementational reasons, the update_ensemble is performed by computing analysis stage first, followed by a prediction of the next sigma ensemble" into the "Updating the Ensemble" section

Otherwise - looks great, thanks! I'd be happy for you to merge after this.

@Zhengyu-Huang
Copy link
Contributor Author

Hi Ollie, I just update it as you suggested.

@odunbar
Copy link
Collaborator

odunbar commented Jun 18, 2021

Hi Daniel,

just a typo in Updating the ensemble section - could you make update_ensemble into update_ensemble .

Then merge away! Thanks!

update make.jl

uki

update uki docs

update

correct typos
@Zhengyu-Huang
Copy link
Contributor Author

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 21, 2021

Build succeeded:

@bors bors bot merged commit 49bf4d2 into main Jun 21, 2021
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