Skip to content
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

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

trontrytel
Copy link
Member

Testing with dycore paper equations

@trontrytel trontrytel requested a review from szy21 October 5, 2024 02:02
Copy link
Member

@szy21 szy21 left a 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.

@trontrytel
Copy link
Member Author

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

[ Info: 0.0030480909747078485
--
  | ERROR: LoadError: AssertionError: rse[end] < 0.003

Would we be ok to bump the limit or is it something we neet o look into

@szy21
Copy link
Member

szy21 commented Oct 5, 2024

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

[ Info: 0.0030480909747078485
--
  | ERROR: LoadError: AssertionError: rse[end] < 0.003

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.

@szy21
Copy link
Member

szy21 commented Oct 5, 2024

Maybe @juliasloan25 can help take a look too.

@szy21
Copy link
Member

szy21 commented Oct 6, 2024

Maybe we can try updating atmos and land separately to see which one breaks the conservation test?

@juliasloan25
Copy link
Member

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

@szy21
Copy link
Member

szy21 commented Oct 7, 2024

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.

@trontrytel
Copy link
Member Author

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

@trontrytel
Copy link
Member Author

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 JLFzf error. For some reason I could not up the dependnecies locally without it. But it breaks things on the CI)

I'll look into the tests and try to understand why the values depend so much on grid spacing.

@szy21
Copy link
Member

szy21 commented Oct 7, 2024

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 JLFzf error. For some reason I could not up the dependnecies locally without it. But it breaks things on the CI)

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.

@trontrytel
Copy link
Member Author

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

@juliasloan25
Copy link
Member

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 get_field functions defined in climaatmos.jl. The function plot_global_conservation is where the RSE is calculated and checked (here).

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

@szy21
Copy link
Member

szy21 commented Oct 8, 2024

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.

@szy21
Copy link
Member

szy21 commented Oct 8, 2024

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.

@juliasloan25
Copy link
Member

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.

That sounds good - I opened an issue to keep track of this #1000

@trontrytel trontrytel changed the title up deps in experiments/ClimaEarth Update Manifests to dycore paper Land, Atmos and Thermo Oct 11, 2024
@trontrytel trontrytel changed the title Update Manifests to dycore paper Land, Atmos and Thermo Update Manifests to dycore paper: Land 15.2, Thermodynamics 12.8, Atmos 27.6 Oct 11, 2024
@juliasloan25 juliasloan25 merged commit 46b321c into main Oct 11, 2024
7 checks passed
@juliasloan25 juliasloan25 deleted the aj/dycore_paper_changes branch October 11, 2024 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants