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

Add wet/dry functionality for the Multilayer SWE in 1D #38

Merged

Conversation

patrickersing
Copy link
Contributor

This PR adds wet/dry functionality to the ShallowWaterMultiLayerEquations1D.

The new functionality includes support for shock-capturing, a positivity-limiter and the new hydrostatic_reconstruction_ersing_etal as well as corresponding tests and examples.

The general strategy follows the implementation of the ShallowWaterEquationsWetDry1D. However, in contrast to the ShallowWaterEquationsWetDry1D the positivity-limiter is applied layerwise and only acts on the water heights h (the momentum remains unchanged) and a velocity desingularization is applied to avoid numerical problems near dry states. The specific implementation of the positivity-limiter is still experimental and might change.

The new hydrostatic reconstruction can be used to create an entropy-stable and well-balanced scheme if the surface flux is set to

surface_flux = (FluxHydrostaticReconstruction(FluxPlusDissipation(flux_ersing_etal, DissipationLocalLaxFriedrichs()), hydrostatic_reconstruction_ersing_etal),
                FluxHydrostaticReconstruction(flux_nonconservative_ersing_etal, hydrostatic_reconstruction_ersing_etal))

@patrickersing patrickersing added the enhancement New feature or request label Apr 9, 2024
Copy link

codecov bot commented Apr 10, 2024

Codecov Report

Attention: Patch coverage is 98.94180% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 99.23%. Comparing base (2c9afa2) to head (dcb6abf).

Files Patch % Lines
...c/callbacks_stage/positivity_shallow_water_dg1d.jl 95.34% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #38      +/-   ##
==========================================
- Coverage   99.26%   99.23%   -0.04%     
==========================================
  Files          50       54       +4     
  Lines        2054     2228     +174     
==========================================
+ Hits         2039     2211     +172     
- Misses         15       17       +2     

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

Copy link
Member

@andrewwinters5000 andrewwinters5000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work @patrickersing ! Having positivity across multiple layers is a great new feature. I left a few comments and opened a discuss about having different threshold quantities across the different variants of the wet/dry capabilities.

@andrewwinters5000
Copy link
Member

It seems that the Mac CI tests are failing due to too strict tolerances. We should keep an eye on this because it would be very annoying (and concerning) if we need to soften the tolerances on so many tests.

@patrickersing
Copy link
Contributor Author

It seems that the Mac CI tests are failing due to too strict tolerances. We should keep an eye on this because it would be very annoying (and concerning) if we need to soften the tolerances on so many tests

I have softened the tolerances in 2857753 and the CI now passes (at least in the recent run). I do not really see another alternative to fix the tests.
I also think the tolerance changes are reasonable. Only elixir_shallowwater_multilayer_beach.jl required a pretty high tolerance atol=1e-5, but this is in line with the tolerance atol=1e-7, set in elixir_shallowwater_beach.jl. The higher value is probably necessary, since the test runs until t=0.25 instead of t=0.05.

Copy link
Member

@andrewwinters5000 andrewwinters5000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good overall @patrickersing ! The only hanging question is related to the (mysterious) reason why lines that should be covered are reported as uncovered.

src/equations/shallow_water_multilayer_1d.jl Show resolved Hide resolved
src/equations/shallow_water_multilayer_1d.jl Show resolved Hide resolved
src/equations/shallow_water_multilayer_1d.jl Outdated Show resolved Hide resolved
Co-authored-by: Andrew Winters <[email protected]>
@ranocha
Copy link
Member

ranocha commented Apr 27, 2024

I am not sure why these lines are not reported as being covered. Any ideas @ranocha or @sloede ?

Because we never call real(equations) in a standard setup? I guess you could get also get rid of this if you don't use it

@patrickersing
Copy link
Contributor Author

I am not sure why these lines are not reported as being covered. Any ideas @ranocha or @sloede ?

Because we never call real(equations) in a standard setup? I guess you could get also get rid of this if you don't use it

@ranocha These lines are definitely called during testing and the respective function calls also show up as covered, so we expect coverage. The same problem also appears for a similar function nlayers(equations).

@patrickersing patrickersing marked this pull request as ready for review April 29, 2024 12:09
Copy link
Member

@andrewwinters5000 andrewwinters5000 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! The small coverage drop is due to spurious lines that should be reported as covered but are not as discussed above.

@andrewwinters5000 andrewwinters5000 merged commit 583a372 into trixi-framework:main Apr 30, 2024
16 of 19 checks passed
@patrickersing patrickersing deleted the es_reconstruction_1d branch April 30, 2024 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants