Update NC-fluxes for the SWE

[patrickersing]

This PR rewrites some nonconservative fluxes for the ShallowWaterEquations and removes the experimental flux_nonconservative_ersing_etal.

The current formulation of flux_nonconservative_wintermeyer_etal introduces additional inner contributions at the interface of the form equations.gravity * h_ll * b_ll that need to be canceled with the nonconservative surface flux.

Rewriting the nonconservative term of flux_nonconservative_wintermeyer_etal from equations.gravity * h_ll * b_ll to equations.gravity * h_ll * b_jump is equivalent in the volume and does not introduce these additional contributions at the interface.

This allows us to simplify the nonconservative fluxes and the rewritten flux_nonconservative_wintermeyer_etal can now also be used as a surface flux. In fact it is then identical to the experimental flux_nonconservative_ersing_etal, which is therefore removed.

The tests remain mostly unaffected by this change (which is expected since the fluxes should be equivalent). Only elixir_shallowwater_source_terms_dirichlet.jl needs to be updated, probably because of the extra equations.gravity * h_ll * b_ll that was present at the boundary.

Closes #868

[github-actions]

Review checklist

This checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging.

Purpose and scope

• The PR has a single goal that is clear from the PR title and/or description.
• All code changes represent a single set of modifications that logically belong together.
• No more than 500 lines of code are changed or there is no obvious way to split the PR into multiple PRs.

Code quality

• The code can be understood easily.
• Newly introduced names for variables etc. are self-descriptive and consistent with existing naming conventions.
• There are no redundancies that can be removed by simple modularization/refactoring.
• There are no leftover debug statements or commented code sections.
• The code adheres to our conventions and style guide, and to the Julia guidelines.

Documentation

• New functions and types are documented with a docstring or top-level comment.
• Relevant publications are referenced in docstrings (see example for formatting).
• Inline comments are used to document longer or unusual code sections.
• Comments describe intent ("why?") and not just functionality ("what?").
• If the PR introduces a significant change or new feature, it is documented in NEWS.md with its PR number.

Testing

• The PR passes all tests.
• New or modified lines of code are covered by tests.
• New or modified tests run in less then 10 seconds.

Performance

• There are no type instabilities or memory allocations in performance-critical parts.
• If the PR intent is to improve performance, before/after time measurements are posted in the PR.

Verification

• The correctness of the code was verified using appropriate tests.
• If new equations/methods are added, a convergence test has been run and the results
  are posted in the PR.

Created with ❤️ by the Trixi.jl community.

[patrickersing]

This should also fix the problem mentioned in #868 for the SWE

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.27%. Comparing base (b9ace6d) to head (d2db26f).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2038      +/-   ##
==========================================
- Coverage   96.28%   96.27%   -0.00%     
==========================================
  Files         462      462              
  Lines       37078    37052      -26     
==========================================
- Hits        35697    35671      -26     
  Misses       1381     1381              

Flag	Coverage Δ
unittests	96.27% <100.00%> (-<0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[patrickersing]

The failing downstream test for TrixiShallowWater.jl is expected because flux_nonconservative_ersing_etal.jl is no longer defined in Trixi.jl. To fix this we will first have to merge this PR and then open a new PR in TrixiSW.

[andrewwinters5000]

LGTM! This is a nice simplification of the nonconservative treatment that we can likely carry over into the shallow water package too, e.g., with the Chen & Noelle nonconservative term.

[ranocha]

@patrickersing @andrewwinters5000 Does this indeed fix the problem mentioned in #868 so that we can close that issue?

[ranocha]

The failing downstream test for TrixiShallowWater.jl is expected because flux_nonconservative_ersing_etal.jl is no longer defined in Trixi.jl. To fix this we will first have to merge this PR and then open a new PR in TrixiSW.

I would like to avoid breaking CI of all PRs immediately when merging this PR. Moreover, you should make sure that nothing (non-experimental) breaks for users of TrixiShallowWater.jl. Thus, could you please first prepare a PR for TrixiShallowWater.jl that uses something along the lines of

if isdefined(Trixi, :flux_nonconservative_ersing_etal)
    import Trixi: flux_nonconservative_ersing_etal
end

and change stuff like

https://github.com/trixi-framework/TrixiShallowWater.jl/blob/cbc57dc13d719485d5256cc1301d74c37179eb5f/src/equations/shallow_water_two_layer_2d.jl#L346-L348

accordingly? If you can't find a way to adapt TrixiShallowWater.jl so that it works with this PR and continues to work with current main, could you please first update the compat bounds of TrixiShallowWater.jl so that the next stable release of Trixi.jl is excluded, then update TrixiShallowWater.jl, bump the version in this PR, and check that everything works?

Only if all of these steps do not work, we can go ahead as you suggest - but in that case, the PR to TrixiShallowWater.jl should already be prepared so that we can release everything quickly.

[andrewwinters5000]

@patrickersing @andrewwinters5000 Does this indeed fix the problem mentioned in #868 so that we can close that issue?

That issue can be closed. This partially fixes it, but that old issue has been superseded by #1445

[patrickersing]

The failing downstream test for TrixiShallowWater.jl is expected because flux_nonconservative_ersing_etal.jl is no longer defined in Trixi.jl. To fix this we will first have to merge this PR and then open a new PR in TrixiSW.

I would like to avoid breaking CI of all PRs immediately when merging this PR. Moreover, you should make sure that nothing (non-experimental) breaks for users of TrixiShallowWater.jl. Thus, could you please first prepare a PR for TrixiShallowWater.jl that uses something along the lines of

if isdefined(Trixi, :flux_nonconservative_ersing_etal)
    import Trixi: flux_nonconservative_ersing_etal
end

and change stuff like

https://github.com/trixi-framework/TrixiShallowWater.jl/blob/cbc57dc13d719485d5256cc1301d74c37179eb5f/src/equations/shallow_water_two_layer_2d.jl#L346-L348

accordingly? If you can't find a way to adapt TrixiShallowWater.jl so that it works with this PR and continues to work with current main, could you please first update the compat bounds of TrixiShallowWater.jl so that the next stable release of Trixi.jl is excluded, then update TrixiShallowWater.jl, bump the version in this PR, and check that everything works?

Only if all of these steps do not work, we can go ahead as you suggest - but in that case, the PR to TrixiShallowWater.jl should already be prepared so that we can release everything quickly.

@ranocha I don't see a good way for TrixiShallowWater.jl to work with both the current main and this PR. The main Problem is that we not only import flux_nonconservative_ersing_etal, but also extend the Trixi.jl method.

Instead, I would go with your second suggestion and update the compat bounds for TrixiSW, for which I prepared PR trixi-framework/TrixiShallowWater.jl#52.

[patrickersing]

I update the compat bounds in TrixiSW to the latest Trixi release, so merging won't break TrixiSW. However, now the downstream test fails, because of a version conflict:
ERROR: LoadError: empty intersection between [email protected] and project compatibility 0.7-0.8.6.

I would suggest, that I prepare a PR for TrixiSW.jl test it locally and then when both PRs are ready we can continue to merge this?

[ranocha]

I update the compat bounds in TrixiSW to the latest Trixi release, so merging won't break TrixiSW. However, now the downstream test fails, because of a version conflict: ERROR: LoadError: empty intersection between [email protected] and project compatibility 0.7-0.8.6.

I would suggest, that I prepare a PR for TrixiSW.jl test it locally and then when both PRs are ready we can continue to merge this?

Sounds good to me 👍

[patrickersing]

@ranocha @andrewwinters5000 The respective TrixiSW-PR trixi-framework/TrixiShallowWater.jl#53 is now reviewed, so from my end this would be ready to merge.

[ranocha]

Thanks!