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

Clarifies notation and units in Ensemble Kalman Inversion documentation #47

Merged
merged 8 commits into from
Sep 3, 2021

Conversation

navidcy
Copy link
Contributor

@navidcy navidcy commented Sep 2, 2021

Attempts to resolve #46.

plus converts $..$ to ..
@navidcy navidcy added the documentation Improvements or additions to documentation label Sep 2, 2021
@navidcy
Copy link
Contributor Author

navidcy commented Sep 2, 2021

@glwagner and @adelinehillier, how does it read now? Feel free to edit at your liking.

@navidcy navidcy requested a review from odunbar September 2, 2021 02:22
@glwagner
Copy link
Member

glwagner commented Sep 2, 2021

@navidcy do we have docs previews?

@glwagner
Copy link
Member

glwagner commented Sep 2, 2021

Thanks @navidcy, I appreciate the extra verbosity.

@navidcy
Copy link
Contributor Author

navidcy commented Sep 2, 2021

@glwagner
Copy link
Member

glwagner commented Sep 2, 2021

Is Gamma_y a scalar variance or a covariance matrix (eg curly N is the multivariate Gaussian)... ?

@navidcy
Copy link
Contributor Author

navidcy commented Sep 2, 2021

Is Gamma_y a scalar variance or a covariance matrix (eg curly N is the multivariate Gaussian)... ?

Hm, didn't think of that.
Still Γ is positive definite and thus the inverse square root makes sense, right?

@glwagner
Copy link
Member

glwagner commented Sep 2, 2021

From Iglesias et al (2013):

image

I think this means \Gamma_y is a matrix in general (allowing each observation to have a different variance and also cross-correlations between observations), so maybe the manipulations you've made in the update equation aren't universally valid?

@navidcy
Copy link
Contributor Author

navidcy commented Sep 2, 2021

Even if it's a matrix, it should be a positive definite one. The inverse square root is then, nothing else, from the eigen decomposition of Γ but with the inverse square root of the eigenvalues of Γ in the diagonal matrix. Right?

@navidcy
Copy link
Contributor Author

navidcy commented Sep 2, 2021

But perhaps it's simpler if we go back to Γ^{-1}. :)

@glwagner
Copy link
Member

glwagner commented Sep 2, 2021

Even if it's a matrix, it should be a positive definite one. The inverse square root is then, nothing else, from the eigen decomposition of Γ but with the inverse square root of the eigenvalues of Γ in the diagonal matrix. Right?

Ah, I see.

But perhaps it's simpler if we go back to Γ^{-1}. :)

Your call.

@navidcy
Copy link
Contributor Author

navidcy commented Sep 2, 2021

Even if it's a matrix, it should be a positive definite one. The inverse square root is then, nothing else, from the eigen decomposition of Γ but with the inverse square root of the eigenvalues of Γ in the diagonal matrix. Right?

Ah, I see.

Well, now that I'm thinking about it, same questions arise if Γ is a matrix and we write Γ^{-1}. Does the inverse exist and is it well-defined? So I vote for keeping the symmetric version and adding a clarification, I'll do that. :)

@glwagner
Copy link
Member

glwagner commented Sep 2, 2021

Even if it's a matrix, it should be a positive definite one. The inverse square root is then, nothing else, from the eigen decomposition of Γ but with the inverse square root of the eigenvalues of Γ in the diagonal matrix. Right?

Ah, I see.

Well, now that I'm thinking about it, same questions arise if Γ is a matrix and we write Γ^{-1}. Does the inverse exist and is it well-defined? So I vote for keeping the symmetric version and adding a clarification, I'll do that. :)

Okay. Also need to clarify that Gamma_y is a covariance matrix rather than variance and N is the multivariate Gaussian distribution. It could be nice to remark on the meaning of Gamma_y too (represents uncertainty, including correlated uncertainty, in the observations?)

@navidcy
Copy link
Contributor Author

navidcy commented Sep 2, 2021

@glwagner have a look at 46b0969 and feel free to rephrase to make it even clearer! :)

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! A few small comments.

RE notation - we have a glossary which links notation in code docs and the application examples. We have tried to be consistent here, so changing these will require its own PR. as things will need to change everywhere.

RE the formulas, the notation we use matches papers that describe the algorithms as well as how they are implemented, so i think we should leave these.

docs/src/ensemble_kalman_inversion.md Outdated Show resolved Hide resolved
docs/src/ensemble_kalman_inversion.md Outdated Show resolved Hide resolved
docs/src/ensemble_kalman_inversion.md Outdated Show resolved Hide resolved
docs/src/ensemble_kalman_inversion.md Show resolved Hide resolved
docs/src/ensemble_kalman_inversion.md Outdated Show resolved Hide resolved
@navidcy
Copy link
Contributor Author

navidcy commented Sep 2, 2021

RE notation - we have a glossary which links notation in code docs and the application examples. We have tried to be consistent here, so changing these will require its own PR. as things will need to change everywhere.

Perhaps raise this as an issue (if it's not already there)?

@navidcy navidcy requested a review from odunbar September 2, 2021 23:05
@navidcy
Copy link
Contributor Author

navidcy commented Sep 2, 2021

@navidcy
Copy link
Contributor Author

navidcy commented Sep 3, 2021

@odunbar I did everything, do you want to have another look?

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.

I'm happy except for the typo, LGTM, thanks Navid!

PS you can view the docs by going the the bottom of the thread, and click Details in the documenter/deploy — Documentation build succeeded section

@navidcy
Copy link
Contributor Author

navidcy commented Sep 3, 2021

bors r+

@bors
Copy link
Contributor

bors bot commented Sep 3, 2021

Build succeeded:

@bors bors bot merged commit c975abd into main Sep 3, 2021
@navidcy navidcy deleted the ncc/EKI-docs-clarifications branch September 3, 2021 23:05
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.

Clarify notation and units in Ensemble Kalman Inversion documentation?
3 participants