-
Notifications
You must be signed in to change notification settings - Fork 19
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
change edmfx advection #2121
change edmfx advection #2121
Conversation
907a638
to
8cf09db
Compare
@tapios , @dennisYatunin - Could you take a look. Below I'm showing results from the edmfx advection test. The first plot shows results for the
TODO - next step: check TODO - next next step: Add the weight for updraft and environment to output. Plot it and see how it looks in problematic areas. ClimaAtmos.jl/src/utils/variable_manipulations.jl Lines 85 to 92 in 5f855e5
|
Here is another clue. I'm plotting the difference between the grid mean You can see that the cache based version has a big spike where we have the problematic secondary peak in Updraft computed from the state: ( Updraft computed from the cache: ( |
Here is how the weight ClimaAtmos.jl/src/utils/variable_manipulations.jl Lines 64 to 71 in 0d2ce17
|
Looks like state and cache are out of sync. May this get better with @charleskawczynski's refactoring of the precomputed quantities to postcomputed quantities? |
Maybe. It's more likely from that ρaq_tot is not equal to ρa * q_tot from the regularization I think. |
Can we run this test with just the limiters on entrainment/detrainment I sketched, without any additional regularization? |
Agreed, the cache is currently in sync, the only difference we should see from moving precomputed to post computed quantities is that we should be able to reduce the number of calls by 1 (technically + a fraction due to reducing in callbacks), while keeping the state and cache in sync. |
This test is only with advection. There is no entrainment/detrainment here |
So entrainment=detrainment=0? |
Yes. The test was meant to look only at advection issues. No other tendencies are applied. I think that the issue is in the regularization function. I'll try to track it today |
Sounds good. Maybe the test is more meaningful with small but nonzero entrainment, detrainment, with the limiter functions on them, to ensure that area fraction remains bounded and updraft properties relax to grid means. |
b345002
to
c988043
Compare
Sounds good to me. |
bors r+ |
2121: change edmfx advection r=trontrytel a=trontrytel Closes #2106 I wasn't sure if we also want to change the tke advection? 2141: Update dependencies r=charleskawczynski a=charleskawczynski This PR upgrades the dependencies, including ClimaTimeSteppers. The new version fuses some operations, resulting in precision error differences. Co-authored-by: Anna Jaruga <ajaruga@caltech.edu> Co-authored-by: Charles Kawczynski <kawczynski.charles@gmail.com>
Build failed (retrying...): |
bors r+ |
Already running a review |
Build succeeded! The publicly hosted instance of bors-ng is deprecated and will go away soon. If you want to self-host your own instance, instructions are here. If you want to switch to GitHub's built-in merge queue, visit their help page. |
Below are results with/without using grid mean density
I'm going to merge those settings as the default for prognostic edmfx and move on to the next test case (GABLS). Using grid mean density in upwind reconstructions: Using updraft density in upwind reconstructions: Target vertical grid: |
Closes #2106
I wasn't sure if we also want to change the tke advection?