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

Update limiters #1494

Open
charleskawczynski opened this issue Oct 10, 2023 · 7 comments
Open

Update limiters #1494

charleskawczynski opened this issue Oct 10, 2023 · 7 comments
Assignees
Labels
enhancement New feature or request SDI Software Development Issue

Comments

@charleskawczynski
Copy link
Member

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:

  • Low frequency sin wave in the interior domain
  • High frequency sin wave (sawtooth) in the interior domain
  • Low frequency sin wave on the boundary
  • High frequency sin wave (sawtooth) on the boundary

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

@charleskawczynski charleskawczynski added the enhancement New feature or request label Oct 10, 2023
@charleskawczynski charleskawczynski self-assigned this Oct 10, 2023
@charleskawczynski charleskawczynski added the SDI Software Development Issue label Oct 10, 2023
@tapios
Copy link
Contributor

tapios commented Oct 11, 2023

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
Copy link
Member Author

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
Copy link
Contributor

tapios commented Oct 16, 2023

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

@charleskawczynski
Copy link
Member Author

charleskawczynski commented Oct 16, 2023

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
Copy link
Contributor

tapios commented Oct 16, 2023

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
Copy link
Member Author

charleskawczynski commented Oct 19, 2023

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:

  • Make our existing horizontal QuasiMonotone limiters GPU compatible (Add GPU support for horizontal limiters #1493)
  • Implement horizontal water borrowing limiters
  • vertical van Leer limiter in the vertical
    • Add unit tests
    • Report the maximum dt across a matrix of moist Held-Suarez examples with both vertical (van Leer) and horizontal limiters

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
Copy link
Contributor

tapios commented Oct 19, 2023

There's a bit more, @charleskawczynski:

  • A central result here should be a tested default configuration of timestepper (ideally SSP, but it can be ARIS if SSP is too slow) and timestep that depends on resolution (like 1/\Delta x).
  • We will very likely still need to implement water borrowing. One reason is that sources/sinks are currently not appropriately limited (though we should work on that separately).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request SDI Software Development Issue
Projects
None yet
Development

No branches or pull requests

3 participants