-
Notifications
You must be signed in to change notification settings - Fork 18
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
Adding sinusoid example #228
Conversation
bors try |
tryBuild succeeded: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a great start!
I have made a few stylistic comments but there are a couple of substance too: Mainly there are 3 things to do
- replace
get_u_final
withget_\phi_final
(see my comment). - Add sections "setting up the problem" and "applying the algorithm" as a lot of the code here is just setup
- Could you add 2 gifs of this example to the index page? One showing the collapse in the constrained parameter-parameter space, and another showing the model ensembles. 1 frame = 1 iteration. I don't think much setup is required here, just a title saying "learning the amplitude and shift of a sine curve"
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey! just a couple more notes:
d1cf67c
to
f40942f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I have one single concern left, which is that people will say the ensemble 'misses the truth', without knowing this is due to the bias of a single data sample.
Yes, what should we do about this? Mention it? I think reducing the observation error would make it converge even faster, which makes the animation less interesting. |
f40942f
to
3f86781
Compare
3f86781
to
9ebf66b
Compare
bors r+ |
Build succeeded: |
235: Sinusoid example in paper.md r=odunbar a=odunbar <!--- THESE LINES ARE COMMENTED --> ## Purpose <!--- One sentence to describe the purpose of this PR, refer to any linked issues: #14 -- this will link to issue 14 Closes #2 -- this will automatically close issue 2 on PR merge --> Adds the sinusoid example #228 into the paper ## Content <!--- specific tasks that are currently complete - Solution implemented --> Latest pdf: [paper.pdf](https://github.com/CliMA/EnsembleKalmanProcesses.jl/files/10061313/paper.pdf) <!--- Review checklist I have: - followed the codebase contribution guide: https://clima.github.io/ClimateMachine.jl/latest/Contributing/ - followed the style guide: https://clima.github.io/ClimateMachine.jl/latest/DevDocs/CodeStyle/ - followed the documentation policy: https://github.com/CliMA/policies/wiki/Documentation-Policy - checked that this PR does not duplicate an open PR. In the Content, I have included - relevant unit tests, and integration tests, - appropriate docstrings on all functions, structs, and modules, and included relevant documentation. --> ---- - [x] I have read and checked the items on the review checklist. Co-authored-by: odunbar <[email protected]>
Closes #183