-
Notifications
You must be signed in to change notification settings - Fork 109
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 new non-conservative flux for standard and two-layer shallow water equations #1508
Add new non-conservative flux for standard and two-layer shallow water equations #1508
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1508 +/- ##
==========================================
- Coverage 83.00% 80.75% -2.25%
==========================================
Files 431 431
Lines 34665 34568 -97
==========================================
- Hits 28772 27914 -858
- Misses 5893 6654 +761
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice addition to the battery of shallow water fluxes and non-conservative terms. It simplifies the combination of volume and surface treatments. Some of the suggestions I made might get taken care of with the SciML formatting PR that will merge soon.
examples/tree_1d_dgsem/elixir_shallowwater_twolayer_well_balanced.jl
Outdated
Show resolved
Hide resolved
Change order of exporting statements Co-authored-by: Andrew Winters <andrew.ross.winters@liu.se>
Co-authored-by: Andrew Winters <andrew.ross.winters@liu.se>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything looks good to me! The only remaining thing is to place the following link (or something similar) to a reference to the preprint in the spot marked TODO
For details see
- Patrick Ersing, Andrew R. Winters (2023)
An entropy stable discontinuous Galerkin method for the two-layer
shallow water equations on curvilinear meshes
[arXiv: 2306.12699](https://arxiv.org/abs/2306.12699)
I have added the references in commit 641a592. With this the requested changes should now all be addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! We just need to play the waiting game for the next round of breaking changes.
#1310 already has a list of things, so maybe it is time for v0.6 instead of waiting... |
I was hoping to let the next breaking release coincide with the divorce of the specific shallow water features into TrixiShallowWater.jl. However, that might be a bit ambitious because I have not had a lot of time to play with the wet/dry merging issues (although I hope to have time in the coming weeks). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! This is ready to merge as part of the breaking changes in v0.6
With #1701 we have another breaking PR ready that should be merged soon. Would you mind bringing this up to date such that it is ready to be merged as well @patrickersing @andrewwinters5000 ? |
Sure, I will update the update the PR and let you know when it is ready @sloede. |
@patrickersing What's the status on this PR? @ranocha What's the protocol for changing the merge base to the staging branch? Can you do this again once Patrick is finished? |
@sloede the PR is now again up to date. I just requested another review from @andrewwinters5000, so if that is accepted the PR can be merged. |
Yes - please ping me when it should be done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! It is also nice that some notation cleanup was caught by Patrick.
@andrewwinters5000 thanks for the review! |
…r equations (#1508) * Add flux_nonconservative_ersing_etal * Change formulation of flux_nonconservative_ersing * remove old fluxes (fjordholm, wintermeyer) * Update tests for new flux_ersing_etal * Add flux_nonconservative_ersing for standard SWE * Edit comments, change flux in elixir * fix well-balanced test * Update src/Trixi.jl Change order of exporting statements Co-authored-by: Andrew Winters <andrew.ross.winters@liu.se> * fix indentation * fix indentation II * Update src/equations/shallow_water_2d.jl Co-authored-by: Andrew Winters <andrew.ross.winters@liu.se> * fix energy total function * Apply formatter * add docstring references * remove flux_nonconservative_fjordholm_etal * apply formatter * Format examples --------- Co-authored-by: Andrew Winters <andrew.ross.winters@liu.se> Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>
This PR adds
flux_nonconservative_ersing_etal
to the standard and two-layer versions of the shallow water equations.This new flux is entropy conservative and well-balanced when used with
flux_wintermeyer_etal
and is path-conservative, such that the nonconservative products are properly defined. In the volume it is equivalent to the previousflux_nonconservative_wintermeyer_etal
. However, it can also be used together withflux_wintermeyer_etal
on the surface, so it is not longer necessary to choose different volume and surface fluxes.Therefore the fluxes
flux_fjordholm_etal
,flux_nonconservative_fjordholm_etal
andflux_nonconservative_wintermeyer_etal
are deprecated and have been removed for the two-layer version, which will introduce a breaking change. The PR is not urgent, so merging can wait until other breaking changes occur.