-
Notifications
You must be signed in to change notification settings - Fork 27
Fix vertical_transport with vanleer_limiter on GPU #4104
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
base: main
Are you sure you want to change the base?
Conversation
0189827 to
7b3b78f
Compare
|
Is it a fix to this issue: #4046? The error looks different? |
|
Should we maybe change one of the GPU CI cases to run with |
7b3b78f to
1ee7863
Compare
Yes, do you have one in mind? We need to use itime with vanleer to test this. |
|
I don't know where itime is used or what it is? Any GPU spherical prognostic edmf 1M simulation should do. Maybe we can add one to the CI or switch one of the 0M ones. It will probably not run for too long right now, but at least we will be testing if its compiling |
ITime is another way of representing time in a simulation. @ph-kev wrote it so he can correct me but I think the most important advantage of itime over a float64 is to prevent floating point errors, particularly in long simulations. The issue here was that I think we can just set |
|
Sound good. Not sure if we have a
|
|
I think this fixes the diagnostic + 1M + vanleer. It probably also fixes prognostic + 1M + vanleer. The problem with testing prognostic + 1M on gpu on ci is that it runs out of parameter memory on the central gpu. |
Purpose
Closes #4046
Previously, we encountered an InvalidIRError from the van Leer limiter on CUDA when using itime. This PR fixes the issue by converting
dtto anFTbefore passing it toᶠlin_vanleer.We currently convert
FT(dt)at a lot of call sites, perhaps we should follow up on this PR and push these conversions into the corresponding methods.No automated tests catch this, perhaps we should use itime more widely.
Reproducer:
Content