-
Notifications
You must be signed in to change notification settings - Fork 125
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
Revision #79
Revision #79
Conversation
Btw, AFAICT, there's no need to keep the deterministic and stochastic rk4 separate. I was thus able to simplify the |
Word, sorry I'm dragging my feet on this -- I have all my time going to
submitting my new manuscript ASAP. I'll try to get back to development
in a week or two. Also, once I get my manuscript up, we'll need to chat
about integrating the SIEnKS into DAPPER.
Cheers,
Colin
…On 8/31/21 8:14 AM, Patrick N. Raanes wrote:
Btw, AFAICT, there's no need to keep the deterministic and stochastic
rk4 separate. I was thus able to simplify the |rk4| code. Can you have
a look?
472bcf8
<472bcf8>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#79 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABXIPFXIPEFDEBFWASZ646DT7TWVPANCNFSM5CTCMSZQ>.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
No worries. That's the way it goes. But I do need you to double check the validity of these changes. In the process, you will hopefully pick up some more of DAPPER. |
Edit of the above: But I do need you to double check the validity of these changes when you find the time |
Good news and bad news. Good news, I submitted my manuscript and I had a chance to look over this today. Bad news, the results that I reproduced with my earlier scripts are now broken in the examples provided, I think some of the changes are OK but some things appear to have gotten lost in this. I'm also not entirely sure based on the changes where this is occurring. Let me explain what the old script was reproducing, so that maybe you have a better sense of what this was intended to study. In particular, we were looking at the following configurations: High-precision (unrealistic for most twin experiments due to the extremely fine discretization)
The ensemble filters generated with the RK4 scheme and EM scheme perform roughly as good as each other, without the presence of filter divergence. This shows that it is possible to use the EM scheme with the extremely fine discretization step. Low-precision (but statistically robust when comparing with the high-precision setting and using RK)
The ensemble filters generated by the RK4 scheme perform about as well as in the high-precision setting, but EM experiences filter divergence for low noise settings. On the other hand, in the high noise settings, RK and EM perform about as well as each other, if I recall, having an average RMSE around the 0.5 mark. This demonstrates that the EM scheme is not statistically robust as the RK scheme, but that when model uncertainty dominates the dynamics, we can still get away with a very low precision scheme like EM. This second scenario is roughly visualized with the following plot online (https://gmd.copernicus.org/articles/13/1903/2020/gmd-13-1903-2020-f08-web.png) where it is plotting the difference between the low-precision EM scheme and a higher precision ensemble forecast. |
In commit f9fbb0b, I produced the following with my script for the Euler-Maruyama scheme with h=0.01 and Taylor 0.005
This is closer to what I remember seeing in my own own results, though it's possible there was still a bug here. However, the current results don't look much like this at all. |
The RK scheme produced the following under the same conditions above
|
Under fine integration conditions described above, the RK scheme produced
|
Under the fine conditions described above, the EM scheme produced
|
Digging out my old simulation data from the manuscript, this is what I find for the coarse simulations (note the difference where I did the silly mistake of using the variance rather than std for the obs error):
Again this is very similar to the previous commit, but things seem to be breaking now. |
Hmm, ok, I will have a new look. I remember checking that the first script did not change the results. But for the second script I did not check. |
IMO I had indeed understood this when making the changes. Is there somewhere in the current code that does not seem to conform to this purpose?
I don't see what you're referring to here |
Hmmm. Actually #79 contained one bugfix. The setting And now I found another bug: we forgot that the rename of "Diffusion" to "diffusion" must also be done in the script! 🤕 I then computed results from #77, #79 and c6c1d60, and compared to your "old simulation data"
The correspondence is now remarkably good! I think we can be pretty happy with this. A very close reproduction of your original results! |
I had two axes in the figure that, to remain on the same scale, should have increased in both directions with respect to the standard deviation, rather than standard deviation / variance... Silly on my part, but remarkably silly that the referees and co-authors didn't catch this either... XD Not that this makes the results erroneous, just harder to compare based on the linear and quadratic scales side-by-side. The numerics above do look really good -- as long as the fine simulation isn't so far different in either case (EM / RK) from the coarse RK simulation, then this validates that we can produce a statistically robust twin experiment in the suggested configuration in a way that doesn't bias the outcome away from the extremely fine ground truth. The EM scheme definitely biases this, as evidenced in a variety of places in the original manuscript, but the RK simulation makes for a very good out-of-the-box SDE ensemble integration scheme. I'll make a pull of the current versions and look back over. Also, I'll start digging into this a bit to inspect how we might merge in the SIEnKS method. You are a recommended referee, in part because you're one of a handful of people who can review this without needing to read the long mathematical intro. I think if you aren't a referee, you can basically just jump to the punch line with the numerical benchmarks. In any case, I think you'll like the numerics, but if I rope you into reading the whole manuscript, you'll probably have some good critical feedback... hehehehe |
I see. Well I didn't actually look it up. I have been more concerned with the actual reproduction than with the interpretation. I think I've spent several days total on these two PRs, so hopefully next time will be easier. I leave it to you to verify the fine / high-res case. Congrats on submitting! I don't know if I have time right now for a review. In any case, if I do, I will sign it, just FYI. |
Hahahaha, indeed... This has been a fair amount of work on both of our
sides I think, I hope that the next roll will come more quickly. In any
case, I think there's some fun additional pieces of research to put in
this that will be good for all respective projects involved.
…On 9/5/21 12:05 PM, Patrick N. Raanes wrote:
I see. Well I didn't actually look it up. I have been more concerned
with the actual reproduction than with the interpretation. I think
I've spent several days total on these two PRs, so hopefully next time
will be easier. I leave it to you to verify the fine / high-res case.
Congrats on submitting! I don't know if I have time right now for a
review. In any case, if I do, I will sign it, just FYI.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#79 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABXIPFQN5LZNDONUCJONVJDUAO5OBANCNFSM5CTCMSZQ>.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
@cgrudz After merging the PR (#77), I also went through and re-did the example scripts. I hope you will find them improved in their simplicity. Please go through and see if you understand the new scripts, ensure that they make sense, and that they produce sensible results.
How to checkout PRsUpdate: relevant commit
Please also fill in the TODOs (use Ctrl-F here to find them).