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 new non-conservative flux for standard and two-layer shallow water equations #1508

Merged
merged 30 commits into from
Nov 10, 2023

Conversation

patrickersing
Copy link
Contributor

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 previous flux_nonconservative_wintermeyer_etal. However, it can also be used together with flux_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 and flux_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.

@ranocha ranocha mentioned this pull request Jun 5, 2023
10 tasks
@codecov
Copy link

codecov bot commented Jun 5, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (9acaa90) 83.00% compared to head (0a594ec) 80.75%.

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     
Flag Coverage Δ
unittests 80.75% <100.00%> (-2.25%) ⬇️

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

Files Coverage Δ
...1d_dgsem/elixir_shallowwater_twolayer_dam_break.jl 100.00% <ø> (ø)
...gsem/elixir_shallowwater_twolayer_well_balanced.jl 100.00% <ø> (ø)
...gsem/elixir_shallowwater_twolayer_well_balanced.jl 100.00% <ø> (ø)
...2d_dgsem/elixir_shallowwater_twolayer_dam_break.jl 100.00% <ø> (ø)
...gsem/elixir_shallowwater_twolayer_well_balanced.jl 100.00% <ø> (ø)
src/Trixi.jl 43.48% <ø> (ø)
src/equations/shallow_water_1d.jl 100.00% <100.00%> (ø)
src/equations/shallow_water_2d.jl 100.00% <100.00%> (ø)
src/equations/shallow_water_two_layer_1d.jl 98.12% <100.00%> (-0.24%) ⬇️
src/equations/shallow_water_two_layer_2d.jl 98.94% <100.00%> (-0.23%) ⬇️

... and 57 files with indirect coverage changes

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

@patrickersing patrickersing marked this pull request as ready for review June 5, 2023 16:08
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.

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.

src/Trixi.jl Show resolved Hide resolved
src/Trixi.jl Outdated Show resolved Hide resolved
src/equations/shallow_water_1d.jl Show resolved Hide resolved
src/equations/shallow_water_1d.jl Outdated Show resolved Hide resolved
src/equations/shallow_water_two_layer_2d.jl Outdated Show resolved Hide resolved
src/equations/shallow_water_two_layer_2d.jl Outdated Show resolved Hide resolved
src/equations/shallow_water_two_layer_2d.jl Outdated Show resolved Hide resolved
src/equations/shallow_water_two_layer_2d.jl Outdated Show resolved Hide resolved
src/equations/shallow_water_two_layer_2d.jl Outdated Show resolved Hide resolved
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.

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)

@patrickersing
Copy link
Contributor Author

I have added the references in commit 641a592. With this the requested changes should now all be addressed.

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! We just need to play the waiting game for the next round of breaking changes.

@sloede
Copy link
Member

sloede commented Jun 30, 2023

#1310 already has a list of things, so maybe it is time for v0.6 instead of waiting...

@andrewwinters5000
Copy link
Member

#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).

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! This is ready to merge as part of the breaking changes in v0.6

@sloede
Copy link
Member

sloede commented Nov 4, 2023

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 ?

@patrickersing
Copy link
Contributor Author

Sure, I will update the update the PR and let you know when it is ready @sloede.

@sloede
Copy link
Member

sloede commented Nov 9, 2023

@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?

@patrickersing
Copy link
Contributor Author

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

@ranocha
Copy link
Member

ranocha commented Nov 9, 2023

What's the protocol for changing the merge base to the staging branch? Can you do this again once Patrick is finished?

Yes - please ping me when it should be done

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! It is also nice that some notation cleanup was caught by Patrick.

@patrickersing
Copy link
Contributor Author

@andrewwinters5000 thanks for the review!

@ranocha @sloede then the PR is ready to be merged.

@ranocha ranocha changed the base branch from main to v0.6-dev November 10, 2023 06:54
@ranocha ranocha merged commit b1b300e into trixi-framework:v0.6-dev Nov 10, 2023
29 of 33 checks passed
@ranocha ranocha mentioned this pull request Nov 10, 2023
2 tasks
ranocha added a commit that referenced this pull request Nov 11, 2023
…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 <[email protected]>

* fix indentation

* fix indentation II

* Update src/equations/shallow_water_2d.jl

Co-authored-by: Andrew Winters <[email protected]>

* fix energy total function

* Apply formatter

* add docstring references

* remove flux_nonconservative_fjordholm_etal

* apply formatter

* Format examples

---------

Co-authored-by: Andrew Winters <[email protected]>
Co-authored-by: Hendrik Ranocha <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants