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

Add 0-moment micro to diagnostic EDMFX #2011

Merged
merged 1 commit into from
Aug 29, 2023
Merged

Add 0-moment micro to diagnostic EDMFX #2011

merged 1 commit into from
Aug 29, 2023

Conversation

trontrytel
Copy link
Member

@trontrytel trontrytel commented Aug 22, 2023

This PR couples 0-moment microphysics scheme to diagnostic EDMFX

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.

Looking good. I was hoping we can have something like calculate_qt_tendency and dispatch on the microphysics/precipitation scheme. And for now we can have nothing or 0M, so we can still support e.g. Bomex where we don't have precipitation. And we can add 1M in the future.

src/cache/diagnostic_edmf_precomputed_quantities.jl Outdated Show resolved Hide resolved
src/cache/diagnostic_edmf_precomputed_quantities.jl Outdated Show resolved Hide resolved
src/cache/diagnostic_edmf_precomputed_quantities.jl Outdated Show resolved Hide resolved
src/parameterized_tendencies/microphysics/precipitation.jl Outdated Show resolved Hide resolved
@trontrytel trontrytel force-pushed the aj/diagnostic_zerom branch 2 times, most recently from 389053b to 1460e82 Compare August 24, 2023 21:50
@trontrytel trontrytel changed the title Draft of 0-moment micro with diag EDMFX Add 0-moment micro to diagnostic EDMFX Aug 24, 2023
@trontrytel trontrytel marked this pull request as ready for review August 24, 2023 23:18
@szy21
Copy link
Member

szy21 commented Aug 24, 2023

What's the advantage of caching ᶜS_e_totʲs_helper over caching ᶜS_e_totʲs?

@trontrytel
Copy link
Member Author

What's the advantage of caching ᶜS_e_totʲs_helper over caching ᶜS_e_totʲs?

Initially I thought I would be able to hide the different options via dispatch over compute_precipitation_cache! and would not have to modify the precipitation_tendency! itself. So I was trying to keep to a similar pattern of S_q_tot * rhoa * S_e_tot. But I gave up on that, and now I don't modify the cache and instead have an ugly if/else in the actual tendency.

So from that point of view I can instead cache ᶜS_q_totʲs * ᶜS_e_totʲs_helper . I can make the change if you think it's cleaner this way.

@trontrytel
Copy link
Member Author

trontrytel commented Aug 25, 2023

All the cases work now. From the precipitating ones:

  • TRMM looks very patchy (what a great analysis on my side...)
  • Rico looks non-existent and I had to decrease the time step to make it run

I'll tackle allocations tomorrow, though right now I don't see any obvious culprits

@trontrytel
Copy link
Member Author

@charleskawczynski - I applied your suggestions regarding the way I was summing things in precipitation tendency and with the FT. However it's still allocating. Is that level of allcocs acceptable? Or would you have any more hints for me?

Evaluated: 650640 ≤ 637328

@trontrytel
Copy link
Member Author

FYI: Just adding 3 variables to cache

            ᶜS_q_totʲs = similar(Y.c, NTuple{n, FT}),
            ᶜS_q_tot⁰ = similar(Y.c, FT),
            ᶜS_e_totʲs_helper = similar(Y.c, NTuple{n, FT}),

and then unpacking them:

    (; ᶜS_q_totʲs, ᶜS_q_tot⁰, ᶜS_e_totʲs_helper) = p

Increases allocations by

Evaluated: 657168 ≤ 637328

See this PR: #2031

I'll test now just adding them, without unpacking.

@charleskawczynski
Copy link
Member

Interesting, I wonder if we're running into CliMA/ClimaCore.jl#1015?

@charleskawczynski
Copy link
Member

I think it might be better / more ideal if we could aim to compute more things on the fly, from a minimal number of variables, if possible. Otherwise, it's probably fine to just increase the allocation limit so that you're not held back by this.

@trontrytel
Copy link
Member Author

I think it might be better / more ideal if we could aim to compute more things on the fly, from a minimal number of variables, if possible. Otherwise, it's probably fine to just increase the allocation limit so that you're not held back by this.

Yes, I agree. Unfortunately we have to cache those ones. Otherwise we would have to redo the whole EDMF diagnostic loop to get the SGS contributions.

I'll test if it has something to do with unpacking rather than caching. But I doubt it

@trontrytel
Copy link
Member Author

trontrytel commented Aug 28, 2023

What we should be doing to avoid caching and allocating, is to augment the grid-mean tendencies from within the cache/diagnostic_edmf_precomputed_quantities.jl. That would basically make a lot of the parameterized_tendencies/microphysics/precipitation.jl dispatching obsolete.

Maybe we could go back to it once we delete the old EDMF code? It will be easier to make the changes then.

This PR only increases the allocations by 2%. So I'll go ahead and merge it as is. But let me know what you think

@trontrytel trontrytel force-pushed the aj/diagnostic_zerom branch 3 times, most recently from c52c7f4 to e53b5c4 Compare August 29, 2023 01:02
@trontrytel
Copy link
Member Author

bors r+

@bors
Copy link
Contributor

bors bot commented Aug 29, 2023

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.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 1cbb906 into main Aug 29, 2023
8 of 9 checks passed
@bors bors bot deleted the aj/diagnostic_zerom branch August 29, 2023 04:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Couple 0-moment microphysics with diagnostic explicit EDMF
3 participants