-
Notifications
You must be signed in to change notification settings - Fork 99
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
Two-layer shallow water equations #1297
Conversation
Updated MMS to depend on time. Equation file restructured to match 1d-SWE.
- Switched equation order to (h1,h1v1,h2,h2v2) - upper layer = 1; lower layer = 2
Edit code format and comments
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.
I am still working on my full review. But these two suggestions I believe will fix the broken tests due to the breaking changes introduced with Trixi v0.5. Is that right @ranocha ?
Yes, looks correct 👍 |
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.
Overall very nice work! This is a nice new set of equations to expand the capabilities of Trixi. A lot of my comments / suggestions are for cleanup of the elixirs. There are some naming conventions for the new equations that we need to decide on as well.
examples/tree_1d_dgsem/elixir_shallowwater_twolayer_convergence.jl
Outdated
Show resolved
Hide resolved
examples/tree_1d_dgsem/elixir_shallowwater_twolayer_dam_break.jl
Outdated
Show resolved
Hide resolved
examples/tree_1d_dgsem/elixir_shallowwater_twolayer_dam_break.jl
Outdated
Show resolved
Hide resolved
examples/tree_1d_dgsem/elixir_shallowwater_twolayer_dam_break.jl
Outdated
Show resolved
Hide resolved
examples/tree_1d_dgsem/elixir_shallowwater_twolayer_dam_break.jl
Outdated
Show resolved
Hide resolved
@andrewwinters5000 Please feel free to request a review from me once your comments are 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.
Nice work overall! All my original comments were addressed. I left some additional comments, most of which are just spacing adjustments.
examples/tree_1d_dgsem/elixir_shallowwater_twolayer_dam_break.jl
Outdated
Show resolved
Hide resolved
examples/tree_1d_dgsem/elixir_shallowwater_twolayer_well_balanced.jl
Outdated
Show resolved
Hide resolved
examples/tree_1d_dgsem/elixir_shallowwater_twolayer_well_balanced.jl
Outdated
Show resolved
Hide resolved
examples/tree_2d_dgsem/elixir_shallowwater_twolayer_well_balanced.jl
Outdated
Show resolved
Hide resolved
examples/unstructured_2d_dgsem/elixir_shallowwater_twolayer_dam_break.jl
Show resolved
Hide resolved
examples/tree_2d_dgsem/elixir_shallowwater_twolayer_well_balanced.jl
Outdated
Show resolved
Hide resolved
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! I just noted two small typos in the docstrings that need corrected. Also, @patrickersing you should add yourself to the AUTHORS.md
list of contributors.
Does this also work for P4estMesh2D? Or is it "in general yes, but we haven't tried it yet"? |
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.
I did a not-in-super-depth review of this PR and mostly it LGTM. I've left a few comments, most of them on consistency and most of them thought as suggestions. I did not review the 1D part, but anything in 2D that affects a similar place in 1D should be considered for a change as well...
examples/tree_1d_dgsem/elixir_shallowwater_twolayer_dam_break.jl
Outdated
Show resolved
Hide resolved
Does this also work for P4estMesh2D? Or is it "in general yes, but we haven't tried it yet"? Please let me know if I should review something more in depth or have a look at something in particular! |
It could easily be adapted to work with the P4estMesh2D file format. This is not a problem. The main issue is that the AMR as it is currently implemented will not be well-balanced for these equations (neither for the original shallow water equations). The mortar projections would need to be done in a special way to allow for AMR meshes. This has been on my TODO list for sometime but I would propose it get separated to a different PR. |
Yes, that absolutely makes sense to separate from the current PR. Looking forward to review well-balanaced AMR :-) |
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, thanks!
Codecov Report
@@ Coverage Diff @@
## main #1297 +/- ##
===========================================
+ Coverage 82.75% 95.97% +13.23%
===========================================
Files 344 351 +7
Lines 28515 29117 +602
===========================================
+ Hits 23596 27945 +4349
+ Misses 4919 1172 -3747
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 93 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
This adds an implementation for the two-layer shallow water equations with entropy conservative and entropy stable fluxes in 1D and 2D. The changes include: