CliMA / ClimaCore.jl

CliMA model dycore
https://clima.github.io/ClimaCore.jl/dev
Apache License 2.0
87 stars 9 forks source link

Update limiters #1494

Open charleskawczynski opened 1 year ago

charleskawczynski commented 1 year ago

Our limiters need to be updated in several aspects:

For unit tests, I would like to also add some simple analysis. For each limiter, we should document the following cases:

Documenting these four cases across a frequency range really characterizes what the limiter is doing. Adding this to the docs would be helpful, and give us confidence about how the limiters are impacting our simulations.

We could go a step further and let advection or diffusion operate on the perturbation of the limited state (perturbed_state = (state - limited_state)), and that would help us get and idea of how we're impacting the phase space, but this is more subjective since different terms can absorb / balance this impact, so it's really more model-dependent.

cc @tapios

tapios commented 1 year ago

Looks good. A standard test for limiters are step waves in tracers advected with constant velocity. If we do not already have that, we shoud add it as a test.

If the third-order flux limited scheme continues to cause stability problems, it would also be good to implement van Leer's second order TVD limiter. This may be more comparable with the oscillatory second-order scheme we otherwise use.

charleskawczynski commented 1 year ago

Sounds good, I'll eventually add those tests if we don't have them. @tapios, can you point me to (paper) references on limiters (or papers that outline limiters in some way)? I'd like to collect them here. So far, I've found:

tapios commented 1 year ago

Thanks, @charleskawczynski. The Guba et al. paper you have describes the horizontal limiter have. In the vertical, there are more choices.

charleskawczynski commented 1 year ago

I think I'm starting to get a better understanding of this.

The FCT algorithm / Van Leer limiters look very different from the Guba one in terms of implementation (one operates on the state, the other on fluxes), also, one is on the vertical (as a FD operator) and the other is in the horizontal. Is the scope of this effort on updating the horizontal limiters, vertical limiters, or both? (or to introduce a 3D limiter?) Also, if we want more than one added, how many and which implementations do we want (the "review of FCT algorithms" has a zoo of FCT-style limiters)

tapios commented 1 year ago

You do not need to do anything to the horizontal limiters (Guba); please just run the standard tests on them (not sure what's implemented, perhaps shallow water flow with a tracer) to make sure that tracers stay positive in the absence of sources/sinks and vertical fluxes.

The main questions concern the vertical limiters, and perhaps their interaction with the horizontal. The only addition may be van Leer in the vertical.

charleskawczynski commented 1 year ago

please just run the standard tests on them (not sure what's implemented, perhaps shallow water flow with a tracer) to make sure that tracers stay positive in the absence of sources/sinks and vertical fluxes.

AFAICT, these tests are already exhaustively run in the ClimaTimeSteppers CI, here. Based on the figures, those limiters seem to be working correctly. So, it seems to me that we want two things out of this issue:

Furthermore, based on what your saying, @tapios, there is no need for water borrowing. In which case, @simonbyrne, can we remove water borrowing from the OKR?

While we grind down on the details of what exactly is needed, I'll continue with trying to get

merged into ClimaCore.

tapios commented 1 year ago

There's a bit more, @charleskawczynski: