remove old EDMF

[szy21]

Purpose

Goodbye old EDMF
co-authored-by: @trontrytel

Here are several reasons we would like to remove it:

• There are 16 CI runs related to it (for a typical total of ~5 CPU-hours)
• The turbconv_cache is fundamentally different from all the other caches. We will have to fix it when cleaning up the cache.
• We will need to add support for different microphysics.
• We will need to add support for new diagnostics.
• For diagnostic EDMFX, we have stable boundary layer (GABLS) shallow Sc clouds (DYCOMS), convective cases (Bomex, Rico), and even a deep convective case (TRMM). We also have an example where the diagnostic EDMFX runs in an aquaplanet configuration, which is something the old EDMF code could not do properly with moisture.
• We don't think we will go back to the old EDMF because of the filters.

To-do

Content

---

• I have read and checked the items on the review checklist.

[charleskawczynski]

I prefer we do not remove this until the new one is actually working. We put a lot of work into this, and I'm not convinced that we won't need to re-analyze what is going on here-- this is the closest version we have to what we do in the existing dycore, whereas TC.jl still uses a different energy formulation, and has diverged a bit from this implementation.

[trontrytel]

I thought there was no going back to the old code, due to the fact that we couldn't make it work with the implicit solver. But you are right that we never fully understood why it did not work.

What would be the acceptable skill level on the side of either DiagnosticEDMFX or PrognosticEDMFX for us to delete this?

[Sbozzolo]

Given the recent advancements in EDMFX, is this a good time to remove the old EDMF?

This would help in cleaning up ClimaAtmos and the work I am doing with the cache.

[charleskawczynski]

Can we add to the description why doing this is beneficial? As my old radiation professor used to say "go from success to success". This code well reproduces something like 12 different cases-- I'd like to know what we gain by deleting it. I'm assuming it's maintenance somehow, what is problematic to maintain?

[Sbozzolo]

Can we add to the description why doing this is beneficial? As my old radiation professor used to say "go from success to success". This code well reproduces something like 12 different cases-- I'd like to know what we gain by deleting it. I'm assuming it's maintenance somehow, what is problematic to maintain?

There are configurations flags that are only relevant to this module + 16 CI runs (for a typical total of ~5 CPU-hours), and some code that is intermix with the code that doesn't depend on EDMF.

Code like:

ClimaAtmos.jl/src/surface_conditions/surface_conditions.jl

Lines 387 to 398 in 6994c4b

	energy_flux = if atmos.energy_form isa PotentialTemperature
	(; ρ_flux_θ = shf / TD.cp_m(thermo_params, ts) * surface_normal)
	elseif atmos.energy_form isa TotalEnergy
	if atmos.turbconv_model isa TC.EDMFModel
	(;
	ρ_flux_h_tot = (shf + lhf) * surface_normal,
	ρ_flux_θ = shf / TD.cp_m(thermo_params, ts) * surface_normal,
	)
	else
	(; ρ_flux_h_tot = (shf + lhf) * surface_normal)
	end
	end

where one has to follow nested if statements, or like

ClimaAtmos.jl/src/prognostic_equations/implicit/implicit_tendency.jl

Lines 12 to 16 in 6994c4b

	if p.turbconv_model isa TurbulenceConvection.EDMFModel
	implicit_sgs_flux_tendency!(Yₜ, Y, p, t, colidx, p.turbconv_model)
	end
	# NOTE: All ρa tendencies should be applied before calling this function
	pressure_work_tendency!(Yₜ, Y, p, t, colidx, p.atmos.turbconv_model)

which is called only for TC.

The turbconv_cache is fundamentally different from all the other caches

ClimaAtmos.jl/src/dycore_equations_deprecated/sgs_flux_tendencies.jl

Lines 130 to 156 in 6994c4b

	function turbconv_cache(
	Y,
	turbconv_model::TC.EDMFModel,
	atmos,
	param_set,
	parsed_args,
	initial_condition,
	)
	FT = Spaces.undertype(axes(Y.c))
	imex_edmf_turbconv = parsed_args["imex_edmf_turbconv"]
	imex_edmf_gm = parsed_args["imex_edmf_gm"]
	test_consistency = parsed_args["test_edmf_consistency"]
	thermo_params = CAP.thermodynamics_params(param_set)
	edmf = turbconv_model
	@info "EDMFModel: \n$(summary(edmf))"
	cache = (;
	edmf,
	turbconv_model,
	imex_edmf_turbconv,
	imex_edmf_gm,
	test_consistency,
	param_set,
	aux = turbconv_aux(atmos, edmf, Y, FT),
	Y_filtered = similar(Y),
	)
	return (; edmf_cache = cache, turbconv_model)
	end

(It has at least one additional nested layer). The turbconv_cache in general is currently relevant only for the this code. In my cleaned up version of the cache, I couldn't easy clean this cache up. Similarly, when I was cleaning the cache, I'd have to fix stuff into this module.

I am guess I am not familiar with the value that this module produces, so to me it feels more like a liability.

[trontrytel]

I think that the diagnostic EDMFX is pretty advanced. We have stable boundary layer (GABLS) shallow Sc clouds (DYCOMS), convective cases (Bomex, Rico) and even a deep convective case (TRMM). What is more we have an example where the diagnostic EDMFX runs in an aquaplanet configuration, which is something the old EDMF code could not do properly with moisture.

The single column cases are not as stable and pretty as the old code. But they don't rely on filters, so it is to be expected.

We can always keep the old code in a separate branch, in case we want to easily go back to it. But with all the changes to the equations, plus the filters I don't think there is any going back to it.

[trontrytel]

ON a side note, I will be removing the 1-moment microphysics support from the old EDMF code. Because I only want to write cache for the new one

[szy21]

Adding to the discussion above, we will need to add diagnostics for the old EDMF when we move to the new diagnostics module, if we still want to run the cases in ci.

I can summarize the above points in the description later.

[charleskawczynski]

What is more we have an example where the diagnostic EDMFX runs in an aquaplanet configuration, which is something the old EDMF code could not do properly with moisture.

Where is the EDMFX aquaplanet run?

[charleskawczynski]

I only see the old edmf version in an aquaplanet run: https://buildkite.com/clima/climaatmos-ci/builds/14135#018b5e08-37c9-408b-a6a0-18ad68b2cffe/142-166.

[szy21]

I only see the old edmf version in an aquaplanet run: https://buildkite.com/clima/climaatmos-ci/builds/14135#018b5e08-37c9-408b-a6a0-18ad68b2cffe/142-166.

Oops, I accidentally removed it here: https://github.com/CliMA/ClimaAtmos.jl/pull/2254/files#diff-a3991ebf1475eb82acab13946ffc1eca02b43917f6d5bec3898f45c8b0b9bd53L652. I will add it back now. This one runs but it was not very stable, but I guess the old one is not stable either.

[szy21]

I added it back here: #2272

[szy21]

Alright I summarized the reasons we would like to remove it in the description. Thanks all for the discussion! Please feel free to add and/or modify.

[charleskawczynski]

Thank you!

[charleskawczynski]

Since I contributed a lot in this space, I figure it's appropriate that I comment on the points, and add some additional remarks:

• There are 16 CI runs related to it (for a typical total of ~5 CPU-hours)

I don't think that this is much of an issue in terms of maintenance, it's probably a lot less than, e.g., the long runs, and none of those jobs are slowing CI.

• The turbconv_cache is fundamentally different from all the other caches. We will have to fix it when cleaning up the cache.

This is indeed unfortunate, and while do-able, refactoring this would further complicate the atmos model, perhaps unnecessarily.

• We will need to add support for different microphysics.

I think this is the main pain point. Maintaining this code in the presence of breaking downstream changes is quite painful, and I think this is the primary reason to drop support if we believe other avenues are more promising

• We will need to add support for new diagnostics.

We already have some diagnostics for the old TC code, and sunsetting this part of the code doesn't necessarily require more diagnostics (unless we wanted to build on it more)

• For diagnostic EDMFX, we have stable boundary layer (GABLS) shallow Sc clouds (DYCOMS), convective cases (Bomex, Rico), and even a deep convective case (TRMM). We also have an example where the diagnostic EDMFX runs in an aquaplanet configuration, which is something the old EDMF code could not do properly with moisture.

This is a good point, and although this is not a direct motivation to remove other working code, I prefer we not slow down progress on this front if there are other barriers.

• We don't think we will go back to the old EDMF because of the filters.

Our dry dycore does already mutate the state in set_precomputed_quantities!, so I don't think it's a huge issue that we also do that in affect_filter!. However, we do mutate the first cell center, and fixing that would require reformulating the BCs, which is not a trivial fix. So, I agree with this point.

Summary

Overall, I agree that maintaining this code is a blocking burden for new developments. It's served its purpose in the interim, and I think we're ready to remove.

Sorry for the friction I've put on this, I just wanted to really make sure that this is the right move because reviving removed code is much more difficult to do than maintaining.

[charleskawczynski]

I can help remove other pieces that may also be tied to this, once we get the main chunk out.

[szy21]

Thanks for the comments Charlie! I will work on cleaning up this PR today.

[szy21]

@trontrytel This PR is ready I think

[trontrytel]

@trontrytel This PR is ready I think

Thank you so much!

[szy21]

@charleskawczynski The buildkite passes but ci fails: https://github.com/CliMA/ClimaAtmos.jl/actions/runs/6645940451/job/18058325471?pr=2163. Could you help me check what the problem is?

[Sbozzolo]

Maybe:

Some package(s) in `deps` of ClimaAtmos [b2c96348-7fb7-4fe0-8da9-78d88439e717] are not loaded during via `using ClimaAtmos`.
    * Distributions [31c24e10-a181-5473-b8eb-7969acd0382f]
    * TerminalLoggers [5d786b92-1e48-4d6f-9151-6b4477ca9bed]
    * LambertW [984bce1d-4616-540c-a9ee-88d1112d94c9]

[szy21]

Should I just remove those dependencies then?

[Sbozzolo]

As far I can see, they were only imported in TurbulenceConvection.jl, so yes.

[szy21]

Removing dependencies fixes the test, thanks @Sbozzolo! I'm going to merge this now.

[szy21]

bors r+

[Sbozzolo]

Can you please also remove:

• the config options that are no longer relevant (update all the default files)
• the arguments in the various functions that are no longer used

[szy21]

bors r-

[bors]

Canceled.

[szy21]

I think I have removed most configs and function arguments that are only used in the old EDMF. There are two configs I didn't remove, turbconv_case and debugging_tc, as they are used in the dycore consistency test, which is currently failing. I'm not quite sure what to do, maybe @charleskawczynski could help?

[charleskawczynski]

We can probably just remove that, too. It would be nice to have, but it’s currently failing, and fixing it is a bit complicated.

[szy21]

Sounds good, I'll remove it then.

[szy21]

bors r+

[bors]

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.

• buildkite/climaatmos-ci
• ci 1.9 - macOS-latest
• ci 1.9 - ubuntu-latest
• docbuild
• format