-
Notifications
You must be signed in to change notification settings - Fork 5
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
Update Manifests to dycore paper: Land 15.2, Thermodynamics 12.8, Atmos 27.6 #994
Conversation
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.
We can merge it after fixing the buildkite errors.
Never does it run smooth... I might have the time on Saturday to look into it. But not sure if I'll be able to finish everything before the CI starts. In many places I see
Would we be ok to bump the limit or is it something we neet o look into |
Compared to the previous build there seems to be a ~50% increase in the water conservation error and an order of magnitude increase in the energy conservation error for many cases, so I think we should spend some time looking into it. I can take a look when I have some time. |
Maybe @juliasloan25 can help take a look too. |
Maybe we can try updating atmos and land separately to see which one breaks the conservation test? |
I reverted only Atmos to the previous version, and all the conservation checks are passing: https://buildkite.com/clima/climacoupler-ci/builds/4717#_ one of the hierarchy cases is failing in saturation_adjustment though |
Thank you. The failing case is probably because ClimaAtmos v0.27.5 is not compatible with Thermodynamics 0.12.8. The only thing I can think about between the two ClimaAtmos version that may affect conservation is this PR: CliMA/ClimaAtmos.jl#3260. Could you or Anna try reverting it? You don't need to revert the whole thing, just the change in src/solver/type_getters.jl, and you may need to add dz_top back to the default config. If this is causing issues, it means the conservation test is sensitive to the grid configuration, and we can take a look at why, but at least we know that it is not a physical problem. |
Sounds good! For the purpose of testing I'll add the reverting changes on top of this PR. Hopefully it fixes the conservation checks. If yes, we might want to rewrite the tests to be less sensitive to the grid, somehow |
68129ec
to
36c15f5
Compare
Seems like it was indeed the change in grid stretching in Atmos that affects the conservation checks. (Ignore the tests that fail because of the package I'll look into the tests and try to understand why the values depend so much on grid spacing. |
Great! I'm ok with changing the tolerance then. I think the test might be highly dependent on the magnitude of source and sink terms, which can be sensitive to vertical resolution during the spin-up. It would be useful to understand what is exactly in the conservation test. |
OK - I'll spend some time thinking about it tomorrow, but try to merge things regardless of whether I have any good ideas on how to improve the tests |
The conservation test uses our ConservationChecker module (here) to compute the total water and energy of each component model individually, then compares the totals to determine conservation. Here are the atmos calculations for energy and water. They also use the I'm not sure why the calculations are grid-dependent. It looks like when we do the calculations for atmos, we just access the total energy and water stored in the simulation object, which hopefully aren't grid-dependent. I'm happy to discuss more in person if that would be helpful |
The ClimaAtmos conservation tests are fine when changing the grid, but I think there is some difference here. Let's find a time to go through the details of the conservation check. For now we can move on with this PR I think. And we can update ClimaLand to v0.15.2 now, which will fix the nightly AMIP. |
Btw I compared the current build with the main build, and in quite a few cases energy conservation error is still larger by a factor of 5-10, so the grid configuration seems to affect mostly the water conservation. The energy conservation error is much smaller than the water conservation error though, so the test doesn't fail. |
That sounds good - I opened an issue to keep track of this #1000 |
36c15f5
to
7d8008d
Compare
7d8008d
to
f0958c0
Compare
Testing with dycore paper equations