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

Fix column integrals for deep atmosphere #2119

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dennisYatunin
Copy link
Member

This PR modifies the column_integral_definite! and column_integral_indefinite! functions for deep atmosphere configurations by weighting the discrete lengths Δz with discrete areas ΔA. Since the values of ΔA are constant within each column when using the shallow atmosphere approximation, this change has no effect for shallow atmosphere configurations, and all current unit tests generate the same results.

The purpose of this change is to fix the errors observed in the deep atmosphere PR's buildkite run, where the vertical integrals for zero-moment microphysics appear to not be conserving water mass.

  • Code follows the style guidelines OR N/A.
  • Unit tests are included OR N/A.
  • Code is exercised in an integration test OR N/A.
  • Documentation has been added/updated OR N/A.

Sets `ϕ_top```{}= \\frac{1}{ΔA(z_{bot})}\\int_{z_{bot}}^{z_{top}}\\,
```ᶜ∂ϕ∂z```(z)\\,ΔA(z)\\,dz +{}```ϕ_bot`, where ``z_{bot}`` and ``z_{top}`` are
the values of `z` at the bottom and top of the domain, and where `ΔA(z)` is the
area differential `J(z)/Δz`, with `J(z)` denoting the metric Jacobian. The input
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
area differential `J(z)/Δz`, with `J(z)` denoting the metric Jacobian. The input
area differential `J(z)/Δz(z)`, with `J(z)` denoting the metric Jacobian. The input

both are a function of z, correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, they are both functions of z. But there were too many zs in this docstring so I've since removed them to improve readability.

@charleskawczynski
Copy link
Member

Nice, thank you, @dennisYatunin!

Can we please add deep atmosphere unit tests in ClimaCore for this function?

@dennisYatunin dennisYatunin force-pushed the dy/deep_column_integrals branch from bc70eab to 7b05841 Compare January 11, 2025 03:29
@dennisYatunin dennisYatunin force-pushed the dy/deep_column_integrals branch from 7b05841 to 05ff05d Compare January 11, 2025 03:29
@dennisYatunin
Copy link
Member Author

@charleskawczynski I have added a unit test to test/Operators/integrals.jl that checks for integral consistency in both shallow and deep configurations. We are now always able to achieve consistency up to several times machine epsilon, including with topography. The consistency is also exact for deep atmospheres with Float64 values, which is even better than I was expecting.

I had to add some keyword arguments to the space constructors in order to ensure that topography and deep atmospheres are initialized correctly. Please take another look when you have some time.

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.

2 participants