Rainy Euler equations with implicit condensation

[benegee]

Part of #95

I suggest merging only the Euler equations with rain and implicit condensation in a first pass. I added just enough tests to achieve good coverage.

[codecov]

Codecov Report

❌ Patch coverage is 96.10092% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.19%. Comparing base (b55387b) to head (311af3c).

Files with missing lines	Patch %	Lines
src/equations/compressible_rainy_euler_2d.jl	96.33%	15 Missing ⚠️
src/callbacks_stage/nonlinear_solve_dg2d.jl	90.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #100      +/-   ##
==========================================
+ Coverage   91.46%   92.19%   +0.73%     
==========================================
  Files          22       25       +3     
  Lines        2331     2767     +436     
==========================================
+ Hits         2132     2551     +419     
- Misses        199      216      +17     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[benegee]

Unfortunately the errors for macos and ubuntu differ by more than the default tolerance. Interestingly, this only happens for the elixir involving a parabolic part.

@tristanmontoya observed discrepancies here as well trixi-framework/Trixi.jl#2431 but for other parts of the code.

Here as well: #94 (comment)

[tristanmontoya]

I still have to go through this in more detail, but I am fine with this PR as a first step. Ultimately, I want to make sure we implement the different model options in an extensible way, as there are so many choices that we need to handle (e.g. gravitational potential term, choice of prognostic variables, condensation model).

[benegee]

Ultimately, I want to make sure we implement the different model options in an extensible way, as there are so many choices that we need to handle (e.g. gravitational potential term, choice of prognostic variables, condensation model).

That is my goal as well! The intention of this PR rather is to make sure that @FnHck's implementation makes it into our initial release.

[benegee]

For the time being I just increased the tolerance to make the macos tests pass.

Could some mac user maybe check if this is just an issue with the github runners?

[tristanmontoya]

Nice work with this, thanks @benegee and @FnHck! I added some comments, which should be largely taken as suggestions. Most of the proposed changes involve moving code that is not problem specific out of the problem-specific elixirs.

[benegee]

Thanks a lot @tristanmontoya for going through this.
I will address the pending items.

[benegee]

Yes, very good point!

[benegee]

Same here!

[benegee]

This is maybe something to discuss.

I think it is not Trixi.jl style. However, negative values, also elsewhere e.g. vapor density, are really critical here. The tests that run in CI should not throw any of them, but if you were to work on these examples again, a warning (maybe not an error) could be helpful.

[benegee]

I tried to address all points raised during review, and included unit and type stability tests as done in #117 .

I also tried to apply our Float32 convention everywhere, as well as switching to our global constants like EARTH_GRAVITATIONAL_ACCELERATION. The latter causes CI to fail because we get small deviations from the reference values. In particular, I confirmed that the shallow water tests and alike finish successfully with g = 9.80616 while the moisture and potential temperature tests rather use g = 9.81. I think the concrete value is not important here, but it would make sense to unify the used constants. For the time being, both values would be fine for me. What do you think @amrueda @tristanmontoya?
(In the long run we should come up with a more flexible global constants set. Something that at least supports delivering values with a predefined floating point type).

[tristanmontoya]

I tried to address all points raised during review, and included unit and type stability tests as done in #117 .

I also tried to apply our Float32 convention everywhere, as well as switching to our global constants like EARTH_GRAVITATIONAL_ACCELERATION. The latter causes CI to fail because we get small deviations from the reference values. In particular, I confirmed that the shallow water tests and alike finish successfully with g = 9.80616 while the moisture and potential temperature tests rather use g = 9.81. I think the concrete value is not important here, but it would make sense to unify the used constants. For the time being, both values would be fine for me. What do you think @amrueda @tristanmontoya? (In the long run we should come up with a more flexible global constants set. Something that at least supports delivering values with a predefined floating point type).

Can we just modify the reference values so CI passes with the unified value of the gravitational acceleration? I'm not sure I understand correctly. The values I used like EARTH_GRAVITATIONAL_ACCELERATION come from the Williamson et al. test suite for shallow water, which is standard in that context.

[benegee]

Can we just modify the reference values so CI passes with the unified value of the gravitational acceleration? I'm not sure I understand correctly. The values I used like EARTH_GRAVITATIONAL_ACCELERATION come from the Williamson et al. test suite for shallow water, which is standard in that context.

All good! I had just been playing around with the constant to see which tests fail. We have to either adjust the values for the shallow water test, or for the Euler tests. I am fine with adjusting the Euler test cases.

But now we are at it, what will be our ultimate standard choice in the future? Williamson? Maybe this one: https://physics.nist.gov/cgi-bin/cuu/Value?gn
(I have no opinion, but it would be nice to have to adjust the reference values only once. 😉 )

[tristanmontoya]

Can we just modify the reference values so CI passes with the unified value of the gravitational acceleration? I'm not sure I understand correctly. The values I used like EARTH_GRAVITATIONAL_ACCELERATION come from the Williamson et al. test suite for shallow water, which is standard in that context.

All good! I had just been playing around with the constant to see which tests fail. We have to either adjust the values for the shallow water test, or for the Euler tests. I am fine with adjusting the Euler test cases.

But now we are at it, what will be our ultimate standard choice in the future? Williamson? Maybe this one: https://physics.nist.gov/cgi-bin/cuu/Value?gn (I have no opinion, but it would be nice to have to adjust the reference values only once. 😉 )

I would keep them as the Williamson values for no other reason than it being what was used in my paper. If DCMIP or some other more recent project has standardized values, we could use that instead. Are the reference values the only reason for the failing tests, @benegee?

[tristanmontoya]

I would be careful with using integer values like this (and elsewhere) rather than using explicit Float32 and/or convert and promote. Could this lead to unwanted integer division if these variables come into contact with other integers? I typically would do something like 1f4 or 10000.0f0 instead of 100000 to avoid this. This is just a question, maybe I am incorrect here, but wanted to be careful.

[tristanmontoya]

Aside from the above comments about constants and integer values, I'd say this is good to merge.