-
Notifications
You must be signed in to change notification settings - Fork 196
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
Generalize RungeKutta3 and QuasiAdamsBashforth2 for any model #1210
Conversation
…th Models and or fields not found.
…he same time. Will do tests next.
Given some checks were not successful I now see that there are some problems with this PR. I glanced at the errors but I don't prettend to understand what's going on. |
Ah I think the method conflict warnings were because By the time I figured this out I already modified quite a bit so I just pushed my changes, but warnings should be gone now! |
I think the tests almost pass! It's just a few tests in Maybe they're just missing a |
Thanks @ali-ramadhan for finding the problem and fixing it. I will pull the updated code and try the test that you pointed out. Not sure I will be able to figure it out or not though but I will try. |
…MA/Oceananigans.jl into fjp/generalize-runge-kutta-3
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.
I think this is a pretty straightforward change. One benefits is that we don't have to double up the time stepping for tracers, as they are done at the same time as the other fields, and it does allow time stepping for virtually any model.
I am happy that the tests all seem to pass and no more warnings, thanks to @ali-ramadhan
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 great! Less code, more features too.
I think there are some conflicts with the master branch so you might have to git merge master
into this branch and resolve the conflicts locally and push the changes before the PR can be merged.
Sometimes the conflicts are pretty minor in which case you could resolve them directly on GitHub. But in this case GitHub says they're too complex.
Sounds good. Luckily those are files that I created. I was planning on fixing them up tomorrow anyhow, now theres just added incentive. |
Benchmark? The reason we combined the updates for velocities was a perceived performance gain. Probably we were wrong about that, but it'd be good to show it. |
Do I understand that to mean that you want to test the performance of the code before and after the PR? If there are tests that I can run to do this with both versions, I would be happy to try that on my desktop, only CPU, but that probably wouldn't be as nice as trying it on a better computer. |
So there's a script (https://github.com/CliMA/Oceananigans.jl/blob/master/benchmark/benchmark_regression.jl) that benchmarks the current branch against the master branch but it doesn't currently print the results so I'll fix it and run it on this branch on Tartarus. |
We would like to test the performance of the version of Oceananigans on this PR versus Oceananigans#master on the CPU and GPU and for a variety of problem sizes. Maintaining good performance is a top priority of ours. Generally speaking we would like to avoid performance regressions --- even small ones (which accumulated over many PRs could become significant). |
…tore_tendencies.jl.
…ext need to look at benchmarks.
New branch fjp/generalize-runge-kutta-3: I did 2 trials to try and get an idea of the variance we can expect. Sorry if this is too much information. Trial 1:
Trial 2:
Old branch master:
For this one single test (clearly more needs to be done) it seems that on average the speedup is slightly lower and the memory is also slightly lower, compared to master |
New branch fjp/generalize-runge-kutta-3:
Old branch master:
Seems comparable to me but two observations:
|
Thanks for running the benchmarks!
|
I ran the Hard to say whether it's noise, it might be more due to other processes causing small variations in runtime. To me I don't think this PR slows down or speeds up the code, but it simplifies and improves the time stepping code so it should be merged. There's a few more memory allocations now (due to extra kernel launches) but this shouldn't affect performance. System info
Master branch
This branch
|
I think a slow down for small models, speed up for large models makes sense given that this PR splits one relatively large kernel into three smaller ones (two times). Seems like an acceptable trade off to me (and also nearly unnoticeable). Why are the validation experiments failing? |
Ah we can ignore the validation experiments pipeline failure. It's failing because |
@ali-ramadhan Any idea why there are two failures above? Should we fix these before we merge? If it's a bit tricky I'm happy to leave the merging upto you, as you are co-author on this PR. Happy to chat if that would help. |
@francispoulin Ah those two failures are unrelated to this PR (it's the validation experiments pipeline failure I mentioned above) so this should be good to merge if you're happy with the PR. |
Thank you @ali-ramadhan ! That is what I thought but wanted to make sure. I will now go and press the big red button, at long last. |
This generalized the runge_kutta3.jl script to work for any model. Also, it combines the time stepping for the fields, say velocities, and the tracers into one loop. Thanks to @glwagner and @ali-ramadhan !
I do get the following warnings that I would like to eliminate. Any suggestions how i can do that?