Add specialized limiter function for AMR coarsen and refine by andrewwinters5000 · Pull Request #164 · trixi-framework/TrixiShallowWater.jl

andrewwinters5000 · 2026-07-06T18:39:39Z

This adds specialized instances of PositivityPreservingLimiterShallowWater for use with the AMR coarsen and refine strategy now used in Trixi as #2396 there has been merged to main. This should fix the failing P4estMesh tests that have AMR and wet/dry.

Moreover, this removes the "band-aid" specialized AMR routines that hard-coded a limiter call that were introduced in #97. As that functionality was marked as experimental, this shouldn't be a breaking change.

codecov · 2026-07-06T18:42:57Z

Codecov Report

❌ Patch coverage is 96.58120% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 99.07%. Comparing base (8981e2f) to head (9217d43).

Files with missing lines	Patch %	Lines
...c/callbacks_stage/positivity_shallow_water_dg2d.jl	96.43%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #164      +/-   ##
==========================================
+ Coverage   97.47%   99.07%   +1.60%     
==========================================
  Files         105      104       -1     
  Lines        5565     5576      +11     
==========================================
+ Hits         5424     5524     +100     
+ Misses        141       52      -89

☔ View full report in Codecov by Harness.
📢 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.

…eed positivity

ranocha · 2026-07-07T10:45:53Z

Just to get it right: merging trixi-framework/Trixi.jl#2396 broke something in TrixiShallowWater.jl, since it used non-public API, correct? Can you add one of the elixirs affected by this to the downstream tests of TrixiShallowWater.jl that Trixi.jl executes for new PRs, please?

ranocha

Thanks! This might be worth a NEWS.md entry. I will review the changes in more details.

ranocha

Thanks, this looks good overall. You also need to require a newer version of Trixi.jl (in Project.toml) including the feature to use limiters for AMR.

ranocha · 2026-07-07T10:57:27Z

+        # We compute the value directly with the mean values, as we assume that
+        # Jensen's inequality holds (e.g. pressure for compressible Euler equations).
+        value_mean = variable(u_mean, equations)
+        theta = (value_mean - threshold) / (value_mean - value_min)


TrixiShallowWater.jl limits only the water height, correct? Thus, it would be better to write something like

Suggested change

# We compute the value directly with the mean values, as we assume that

# Jensen's inequality holds (e.g. pressure for compressible Euler equations).

value_mean = variable(u_mean, equations)

theta = (value_mean - threshold) / (value_mean - value_min)

# We compute the value directly with the mean values. In the general

# case, this requires Jensen's inequality, e.g., for the pressure for

# the compressible Euler equations. However, we only need linear

# variables such as the `waterheight` here, where this is satisfied.

value_mean = variable(u_mean, equations)

theta = (value_mean - threshold) / (value_mean - value_min)

ranocha · 2026-07-07T11:03:36Z

+                u_node = get_node_vars(u, equations, dg, i, j, new_element_id)
+
+                # Cut off velocity in case that the water height is smaller than the threshold
+
+                h_node, h_v1_node, h_v2_node, b_node = u_node
+                h_mean, h_v1_mean, h_v2_mean, _ = u_mean # b_mean is not used as it must not be overwritten
+
+                if h_node <= threshold
+                    h_v1_node = zero(eltype(u))
+                    h_v2_node = zero(eltype(u))
+                    h_v1_mean = zero(eltype(u))
+                    h_v2_mean = zero(eltype(u))
+                end
+
+                u_node = SVector(h_node, h_v1_node, h_v2_node, b_node)
+                u_mean = SVector(h_mean, h_v1_mean, h_v2_mean, b_node)
+
+                # When velocities are cut off, the only averaged value is the water height,
+                # because the velocities are set to zero and this value is passed.
+                # Otherwise, the velocities are averaged, as well.
+                # Note that the auxiliary bottom topography variable `b` is never limited.
+                set_node_vars!(u, theta * u_node + (1 - theta) * u_mean,
+                               equations, dg, i, j, new_element_id)


It looks like there is some duplication of code around inner blocks like this one. Did you check whether it could make sense to put parts like this one into the own (@inlined) functions?

That's a good point. I can double check if there are places where introducing a helper function is warranted.

If you @inline them, there will be no performance penalty.

ranocha · 2026-07-07T11:06:06Z

+# Version used by the AMR callback
+function (limiter!::PositivityPreservingLimiterShallowWater)(u, mesh, equations, solver,
+                                                             cache, args...)
+    limiter_shallow_water!(u, limiter!.variables, mesh, equations,
+                           solver, cache, args...)
+end


Something else that just came to my mind: What about the 1D case? As far as I see, only the 2D case is changed. However, this allow the 1D case as well.

Currently, the AMR in TrixiShallowWater is only available in 2D and only on P4estMesh. This is because we need special projections to and from the mortars to make sure that the simulation stays well-balanced. I never bothered to implement these projections on TreeMesh. That is why I did not modify the 1D variant of the limiters.

You don't need projections in 1D since there are no mortars, are there?

Ah, a fantastic point! In 1D there are no mortars so it would be possible to do AMR with some modified limiters. I can try out such a run and add it to this PR or do a separate 1D PR, depending on how much additional code needs touched.

Doing it in another PR would be perfectly fine. It would just be good to have this on the agenda, since some parts of the new code here allow 1D while others are specialized for 2D.

It is easy enough to get the AMR up and running on TreeMesh 1D. Although figuring out a good AMR indicator is another task, as you can see in this snapshot, the indicator fires a lot in the dry beach region.

I will do the 1D version in a different PR.

ranocha · 2026-07-07T11:12:17Z

+                if h_node <= threshold
+                    h_v1_node = zero(eltype(u))
+                    h_v2_node = zero(eltype(u))
+                    h_v1_mean = zero(eltype(u))
+                    h_v2_mean = zero(eltype(u))
+                end
+
+                u_node = SVector(h_node, h_v1_node, h_v2_node, b_node)
+                u_mean = SVector(h_mean, h_v1_mean, h_v2_mean, b_node)


This makes me feel a bit unhappy. If I understand it correctly, the mean value u_mean is computed first and then used to update the values in the child cells. However, the mean value of the momentum is set to zero if the nodal value at some point has h_node <= threshold. Thus, a zero momentum will propagate to other nodes, even with h_node > threshold, if they ocme after a dry node. This is likely not what we want, is it?

A solution could be to avoid overwriting u_mean by instead using something like

Suggested change

if h_node <= threshold

h_v1_node = zero(eltype(u))

h_v2_node = zero(eltype(u))

h_v1_mean = zero(eltype(u))

h_v2_mean = zero(eltype(u))

end

u_node = SVector(h_node, h_v1_node, h_v2_node, b_node)

u_mean = SVector(h_mean, h_v1_mean, h_v2_mean, b_node)

if h_node <= threshold

h_v1_node = zero(eltype(u))

h_v2_node = zero(eltype(u))

h_v1_mean = zero(eltype(u))

h_v2_mean = zero(eltype(u))

end

u_node = SVector(h_node, h_v1_node, h_v2_node, b_node)

u_mean_local = SVector(h_mean, h_v1_mean, h_v2_mean, b_node)

and using u_mean_local instead of u_mean below.

This situation should then be explained in a comment around this assignment. The same kind of observation holds for other cases as well.

Ah, yes. That is a possibility the zero momentum value could propagate beyond the node where we apply the cutoff. I can change and test this out.

Using the u_mean_local to ensure that only the node i,j is affected ended up slightly changing some test values of other wet/dry test cases.

andrewwinters5000 · 2026-07-07T18:17:29Z

Just to get it right: merging trixi-framework/Trixi.jl#2396 broke something in TrixiShallowWater.jl, since it used non-public API, correct? Can you add one of the elixirs affected by this to the downstream tests of TrixiShallowWater.jl that Trixi.jl executes for new PRs, please?

Yes, that is what happened. TrixiShallowWater was using a specialized version of coarsen / refine routines that had a "band-aid" hard coded limiter call. With the release of Trixi version 0.16.24 (that is used by the CI testing) the API change caused the TrixiShallowWater tests to then fallback to using the Trixi.coarsen or Trixi.refine routines that do not have the limiter call by default.

The limiting strategy from trixi-framework/Trixi.jl#2396 is the correct approach now that the AMR callback can accept the limiter! as an optional argument and the limiting is done with respect to parent or children mean values.

I will add one of the wet/dry + AMR elixirs to the downstream tests as this is a much tougher test than the fully wet AMR test that is currently in the downstream.

andrewwinters5000 · 2026-07-08T13:44:48Z

I have run into an issue that I am unsure the best way to solve. I added one of the AMR + wet/dry tests to test_upstream.jl as this would be run by Trixi.jl for the Downstream CI runs. However, within TrixiShallowWater.jl we use the test_upstream.jl for the Downgrade CI run. But, when downgrading to Trixi v0.15 the AMRCallback does not have the optional limiter! argument and the downgrade test fails.

Should I add logic in test_upstream.jl to only run the test if Trixi is v0.16.24 or higher?

ranocha

Thanks!

Should I add logic in test_upstream.jl to only run the test if Trixi is v0.16.24 or higher?

No, we need to adapt the compatibility requirement for Trixi.jl anyway.

Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>

andrewwinters5000 · 2026-07-14T10:09:05Z

Thanks!

andrewwinters5000 added 4 commits July 6, 2026 20:17

remove old specialized AMR coarsen and refine routines

6470c0c

add new specialized limiters for the refine and coarsen with AMR

11106eb

update elixirs and tests

185ab19

apply formatter

6bab087

andrewwinters5000 marked this pull request as ready for review July 6, 2026 18:39

andrewwinters5000 requested review from JoshuaLampert and ranocha July 6, 2026 18:42

andrewwinters5000 added 2 commits July 6, 2026 20:57

readjust fully wet AMR test elixir so Downgrade will pass. Does not n…

a33dadf

…eed positivity

adjust final time of one test to fire more code

c2f5409

andrewwinters5000 mentioned this pull request Jul 7, 2026

Replace all instances of OBS with Note #165

Merged

ranocha reviewed Jul 7, 2026

View reviewed changes

Comment thread test/test_p4est_2d.jl

ranocha reviewed Jul 7, 2026

View reviewed changes

ranocha requested changes Jul 7, 2026

View reviewed changes

andrewwinters5000 added 6 commits July 8, 2026 11:14

add NEWS item regarding the new limiter

61026a7

update compat bound on Trixi

a5773ad

add an AMR with positivity limiter to the upstream tests

6ef66b4

introduce helper function for velocity cutoff and update tests

4d394f5

apply formatter

7265239

adjust upstream test values for conical island

a77514d

andrewwinters5000 added 6 commits July 8, 2026 15:50

run formatter

444e44a

add logic to avoid running upstream test during downgrade

6bb3343

attempt to fix upstream test

6fe2368

apply formatter

9915a83

another attempt to fix upstream tests

a99e8dc

yet another attempt to fix upstream tests

625d657

andrewwinters5000 requested a review from ranocha July 8, 2026 20:25

This was referenced Jul 10, 2026

Fix SWME flux function #161

Merged

Bump the all-github-actions group with 3 updates #163

Merged

andrewwinters5000 added 4 commits July 10, 2026 10:57

Merge branch 'main' into amr-limiter

86f5d7c

add equations for dispatching of helper function

cf33131

update comments

6b03b69

fix typo

ebdb67f

ranocha requested changes Jul 11, 2026

View reviewed changes

Comment thread docs/Project.toml Outdated

Comment thread test/Project.toml Outdated

Comment thread Project.toml Outdated

Comment thread test/test_upstream.jl Outdated

Comment thread test/test_upstream.jl Outdated

andrewwinters5000 and others added 3 commits July 14, 2026 09:49

Apply suggestions from code review

2f58b57

Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>

remove unnecessary logic in test_upstream.jl

b34eba7

bump ODE compat bounds needed for downgrade testing with Trixi

9217d43

andrewwinters5000 requested a review from ranocha July 14, 2026 08:14

ranocha approved these changes Jul 14, 2026

View reviewed changes

andrewwinters5000 merged commit f3adf8a into trixi-framework:main Jul 14, 2026
17 of 18 checks passed

andrewwinters5000 deleted the amr-limiter branch July 14, 2026 10:09

andrewwinters5000 mentioned this pull request Jul 14, 2026

Add 1D versions of AMR positivity limiter #167

Merged

andrewwinters5000 mentioned this pull request Aug 5, 2026

Mark AMR specific limiter routines as experimental #177

Merged

Conversation

andrewwinters5000 commented Jul 6, 2026

Uh oh!

codecov Bot commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ranocha commented Jul 7, 2026

Uh oh!

Uh oh!

ranocha left a comment

Choose a reason for hiding this comment

Uh oh!

ranocha left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewwinters5000 Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewwinters5000 commented Jul 7, 2026

Uh oh!

andrewwinters5000 commented Jul 8, 2026

Uh oh!

ranocha left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andrewwinters5000 commented Jul 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Jul 6, 2026 •

edited

Loading

andrewwinters5000 Jul 8, 2026 •

edited

Loading

ranocha left a comment •

edited

Loading