Move SWEs to TrixiShallowWater.jl

[patrickersing]

Now that TrixiShallowWater.jl is released, we would like to move the shallow water equations from Trixi.jl into TrixiShallowWater.jl. The aim of this PR is to test how such a separation would affect the coverage and testing in Trixi.jl and it should provide a basis for discussion on how to structure the separation and which functionality should remain in Trixi.jl. To coordinate the move the corresponding PR (trixi-framework/TrixiShallowWater.jl#96) has been created in TrixiShallowWater.jl .

Right now it is planned to move the following equations:

• ShallowWaterEquations1D
• ShallowWaterEquations2D
• ShallowWaterEquationsQuasi1D

The functions below are currently exported by Trixi.jl and used by both TrixiShallowWater.jl and TrixiAtmo.jl. so we should discuss whether each subpackage would define their own version or if we want to keep empty versions of those functions in Trixi.jl:

• flux_wintermeyer_etal
• flux_nonconservative_wintermeyer_etal
• flux_fjordholm_etal
• flux_nonconservative_fjordholm_etal
• lake_at_rest_error
• waterheight
• waterheight_pressure
• AbstractShallowWaterEquations

TODO:

• Update Trixi2Vtk.jl test that uses elixir_shallowwater_ec.jl (https://github.com/trixi-framework/Trixi.jl/actions/runs/14736003027/job/41362120222?pr=2379#step:9:1284)
  • Fixed with merged PRs in Trixi2Vtk_reference_files and Trixi2Vtk
• Create news item
• Fix coverage drop due to removed tests:
  • Elixir that uses DissipationLocalLaxFriedrichs
  • Elixir with boundary conditions for some nonconservative equation (Tree1D & Tree2D)
  • Elixir with BCs for some nonconservative equation on UnstructuredMesh2D
  • Some dgmulti elixir to test hadamard_sum!

Closes #2357

[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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.80%. Comparing base (2fc0a47) to head (270e8f7).
Report is 1 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2379       +/-   ##
===========================================
+ Coverage   85.73%   96.80%   +11.06%     
===========================================
  Files         512      494       -18     
  Lines       41815    40825      -990     
===========================================
+ Hits        35850    39518     +3668     
+ Misses       5965     1307     -4658     

Flag	Coverage Δ
unittests	96.80% <100.00%> (+11.06%)	⬆️

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.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[ranocha]

The functions below are currently exported by Trixi.jl and used by both TrixiShallowWater.jl and TrixiAtmo.jl. so we should discuss whether each subpackage would define their own version or if we want to keep empty versions of those functions in Trixi.jl:

• flux_wintermeyer_etal
• flux_nonconservative_wintermeyer_etal
• flux_fjordholm_etal
• flux_nonconservative_fjordholm_etal
• lake_at_rest_error
• waterheight
• waterheight_pressure
• AbstractShallowWaterEquations

I prefer defining them in a central place and adding specialized methods in our sub-packages. Otherwise, it will be annoying to work with different sub-packages in the same project.

[patrickersing]

I prefer defining them in a central place and adding specialized methods in our sub-packages. Otherwise, it will be annoying to work with different sub-packages in the same project.

That sounds good to me. If we want to avoid problems from duplicate function names in the sub-packages it would also be good to add a test like this to both TrixiShallowWater and TrixiAtmo:

        @testset "Namespace conflicts" begin
            # Test for namespace conflicts between TrixiShallowWater.jl and TrixiAtmo.jl
            for name in names(TrixiAtmo)
                @test !(name in names(TrixiShallowWater))
            end
        end

[patrickersing]

The function lake_at_rest_error it currently not used by TrixiAtmo.jl, do you anticipate that you will make use of the function in the future or should it live inside TrixiShallowWater.jl for now?

[patrickersing]

The function lake_at_rest_error it currently not used by TrixiAtmo.jl, do you anticipate that you will make use of the function in the future or should it live inside TrixiShallowWater.jl for now?

@amrueda, @tristanmontoya, @benegee ?

[tristanmontoya]

@patrickersing I haven't used lake_at_rest_error for anything, because I initially didn't actually understand what it was used for (I just tested well balancedness for my methods by initializing a lake-at-rest state and checking the standard linf errors and residuals).

However, I recently looked at what you did in your hydrostatic reconstruction paper to test well balancedness (taking a perturbed state and seeing if the scheme dissipates it to recover a lake at rest) and I can see how lake_at_rest_error would be useful for that. Now that I understand what the function is for, I actually think it would be nice to have it in TrixiAtmo.jl so that we can run those kinds of tests as well.

Edit: to directly answer your question, I would prefer if it stays in Trixi.jl for this PR as I want to extend it in TrixiAtmo.jl. But it's not super important, as I can just make it atmosphere_at_rest_error over there if needed.

[patrickersing]

@patrickersing I haven't used lake_at_rest_error for anything, because I initially didn't actually understand what it was used for (I just tested well balancedness for my methods by initializing a lake-at-rest state and checking the standard linf errors and residuals).

However, I recently looked at what you did in your hydrostatic reconstruction paper to test well balancedness (taking a perturbed state and seeing if the scheme dissipates it to recover a lake at rest) and I can see how lake_at_rest_error would be useful for that. Now that I understand what the function is for, I actually think it would be nice to have it in TrixiAtmo.jl so that we can run those kinds of tests as well.

Edit: to directly answer your question, I would prefer if it stays in Trixi.jl for this PR as I want to extend it in TrixiAtmo.jl. But it's not super important, as I can just make it atmosphere_at_rest_error over there if needed.

Okay, then we can just keep the function in Trixi.jl.

[andrewwinters5000]

The removal looks good with appropriate changes to the README and a NEWS item. The empty functions waterheight, waterheight_pressure, lake_at_rest_error live in equations.jl and the empty Wintermeyer or Fjordholm based fluxes live in numerical_fluxes.jl which makes sense. The new tests that exercise the Dirichilet and nonconservative terms via MHD onion elixirs (for TreeMesh2D and UnstructuredMesh2D) or a Quasi 1D Euler run (for TreeMesh1D).

[DanielDoehring]

I have only questions about whether or not to have docstrings (of the admittedly still exported) functions. I would feel like having the documentation closer to the implementation.

[andrewwinters5000]

This is good to go from my side. The Downstream should pass again once the corresponding TrixiShallowWater PR is merged and released.

[patrickersing]

@andrewwinters5000 thanks for the review! Yes, that the downstream testing fails is okay, since we manually test compatibility in the respective TrixiShallowWater.jl PR.

@trixi-framework/principal-developers Can someone have a second look over this?

[ranocha]

Thanks a lot! I just have a few minor formatting comments. Could you please check them, @patrickersing, and apply them in TrixiShallowWater.jl accordingly?

[ranocha]

Thanks