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 multilayer shallow water equations in 2D #40

Merged
merged 18 commits into from
Apr 30, 2024

Conversation

patrickersing
Copy link
Contributor

This PR extends the multilayer shallow water equations introduced in #30 to 2D.
The PR introduces the new equations file and basic tests for well-balancedness, entropy-conservation and convergence on TreeMesh and UnstructuredMesh

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

codecov bot commented Apr 11, 2024

Codecov Report

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

Project coverage is 99.26%. Comparing base (b928469) to head (de6251f).

Files Patch % Lines
src/equations/shallow_water_multilayer_2d.jl 99.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #40      +/-   ##
==========================================
+ Coverage   99.22%   99.26%   +0.04%     
==========================================
  Files          45       50       +5     
  Lines        1680     2054     +374     
==========================================
+ Hits         1667     2039     +372     
- Misses         13       15       +2     

☔ 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 April 11, 2024 15:08
@patrickersing patrickersing reopened this Apr 11, 2024
@patrickersing
Copy link
Contributor Author

Here are some convergence results on TreeMesh2D.
Overall the convergence order is a bit worse than expected.

polydeg = 3, EC flux:
Screenshot from 2024-04-11 19-00-05

polydeg=3, flux_lax_friedrichs
Screenshot from 2024-04-11 18-58-03

polydeg=4, EC flux:
Screenshot from 2024-04-11 19-02-15

polydeg=4, flux_lax_friedrichs:
Screenshot from 2024-04-11 18-56-33

polydeg=5, EC flux:
Screenshot from 2024-04-11 19-06-09

polydeg=5, flux_lax_friedrichs`:
Screenshot from 2024-04-11 18-54-13

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.

Great work @patrickersing ! It is nice that the 2D version of the multilayer equations came together quite easily.

@andrewwinters5000
Copy link
Member

Here are some convergence results on TreeMesh2D. Overall the convergence order is a bit worse than expected.

I agree that some of the convergence values are strange. The polydeg=5 look pretty okay, but the polydeg=3or 4 are a bit odd. Can you try running the ES convergence with flux_hll? Sometimes the flux_lax_friedrichs give dodgy results compare to the more "sophisticated" Riemann solvers.

@patrickersing
Copy link
Contributor Author

Here are the convergence results using FluxHLL. The results look a bit more reasonable, but are still a bit strange.
polydeg=3
polydeg_3
polydeg=4
polydeg_4
polydeg=5
polydeg_5

@andrewwinters5000
Copy link
Member

Here are the convergence results using FluxHLL. The results look a bit more reasonable, but are still a bit strange.

Agreed, these are still a bit odd. The final EOC for the odd polynomial degrees is now better (around polydeg+2) and the even is now worse (around polydeg) than the expected polydeg + 1 for an ES scheme. Would it be possible to run one more refinement level for each of the polydegs to see if this weird trend continues? Or if the EOCs "settle" in some sense.

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 already looks quite good. Just a suggestion about the explicit inclusion o fthe multiplication symbols. This would likely need done in 1D as well.

patrickersing and others added 4 commits April 25, 2024 10:45
@patrickersing
Copy link
Contributor Author

Here are the convergence results using FluxHLL. The results look a bit more reasonable, but are still a bit strange.

Agreed, these are still a bit odd. The final EOC for the odd polynomial degrees is now better (around polydeg+2) and the even is now worse (around polydeg) than the expected polydeg + 1 for an ES scheme. Would it be possible to run one more refinement level for each of the polydegs to see if this weird trend continues? Or if the EOCs "settle" in some sense.

I followed your suggestion and set the gravity_constant=1.1 to obtain a MMS in supersonic flow conditions. Now the convergence rates look pretty good, which suggests that the weird behavior was caused by the transonic MMS.
The results below are obtained with FluxHLL for t=0.2 and a slightly modified MMS with g=1.1.

polydeg = 3:
Screenshot from 2024-04-26 10-46-01

polydeg = 4:
Screenshot from 2024-04-26 10-48-16

polydeg = 5:
image

Since this fixes the weird convergence, I would suggest to update the MMS for both ShallowWaterMultiLayer1D and ShallowWaterMultiLayer2D.

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! I just left a few clean-up comments / questions. One thing we should keep in mind is that the docstrings will need updating once the multilayer preprint is finished.

src/equations/shallow_water_multilayer_1d.jl Show resolved Hide resolved
src/equations/shallow_water_multilayer_1d.jl Outdated Show resolved Hide resolved
src/equations/shallow_water_multilayer_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.

LGTM! We just have to hope to get an Intel chipset for the macOS tests due to the ARM issues.

@patrickersing
Copy link
Contributor Author

I think the reason why tests are suddenly failing is that macOS-latest now defaults to macOS-14-arm64 instead of macOS-12. This also explains why even with multiple tries I never get Intel runners.
So I would suggest to fix the version to macOS-12 for now to merge the PR (assuming this works) and tackle the problem in a dedicated PR.

@andrewwinters5000
Copy link
Member

I think the reason why tests are suddenly failing is that macOS-latest now defaults to macOS-14-arm64 instead of macOS-12. This also explains why even with multiple tries I never get Intel runners. So I would suggest to fix the version to macOS-12 for now to merge the PR (assuming this works) and tackle the problem in a dedicated PR.

Sounds reasonable. We can adjust down to macOS-12 and hopefully grab the Intel runners to fix this problem. We can then open a corresponding issue regarding the CI on Mac-ARM and address it later.

@patrickersing
Copy link
Contributor Author

With the recent CI change in #42 the issues with macOS have been fixed, so we should be good to merge.

@andrewwinters5000 andrewwinters5000 merged commit 2c9afa2 into trixi-framework:main Apr 30, 2024
19 checks passed
@patrickersing patrickersing deleted the 2d_mlswe branch April 30, 2024 10:40
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.

2 participants