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

implement Serre-Green-Naghdi equations for flat bathymetry #127

Merged
merged 38 commits into from
Aug 15, 2024

Conversation

ranocha
Copy link
Collaborator

@ranocha ranocha commented Aug 7, 2024

I have implemented the semidiscretization of https://arxiv.org/abs/2408.02665 of the Serre-Green-Naghdi equations for flat bathymetry using central SBP operators. This is the first step and I plan to extend it in the future

  • upwind operators
  • variable bathymetry (mild-slope approximation and full system)
  • hyperbolic approximation

I plan to do this in other PRs to make the individual steps easier to review.

@ranocha ranocha requested a review from JoshuaLampert August 7, 2024 17:51
Copy link

codecov bot commented Aug 7, 2024

Codecov Report

Attention: Patch coverage is 98.07692% with 4 lines in your changes missing coverage. Please review.

Project coverage is 97.00%. Comparing base (6577a58) to head (ad71305).
Report is 1 commits behind head on main.

Files Patch % Lines
src/equations/equations.jl 92.30% 2 Missing ⚠️
src/equations/serre_green_naghdi_flat_1d.jl 98.85% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #127      +/-   ##
==========================================
+ Coverage   96.88%   97.00%   +0.12%     
==========================================
  Files          17       18       +1     
  Lines        1187     1369     +182     
==========================================
+ Hits         1150     1328     +178     
- Misses         37       41       +4     

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

Copy link
Owner

@JoshuaLampert JoshuaLampert left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR! I left some questions, ideas, and comments below. Some more questions that are in a broader context:

  1. What is the plan regarding the full Serre-Green-Naghdi equations and the hyperbolic approximation? Having two separate equations, I guess? To what extend can they share code?
  2. One bigger question I already addressed in some places in the code review: What is the plan regarding the different bathymetries? I would like to have only one equation struct covering all types of bathymetry (in contrast to the current state for the BBM-BBM equations). I think in order to have a smooth generalization for other than constant bathymetries in the future, it already pays of to have a clear plan in mind how this generalization should look like. Especially, should we have another variable in the ODE representing the bathymetry? If we do, then this should already be present in the flat bathymetry case, right?
  3. From the paper I see there are several possible energy-conserving split forms. You only implemented one, right? I am totally fine with that, but I was only wondering whether you have some plans/ideas to be more flexible?
  4. In the paper you also include the possibility to add artificial diffusion? Do you have any plans for integrating this feature in DispersiveShallowWater.jl? If we want to include it, could this be a general (more or less) equations-independent choice and could be switched on/off on the solver side rather than the equation side? This is definitely not something, we need to decide now, but I already wanted to hear your opinion on that.
  5. To have a better impression on the dispersive behaviour of the equations, do you know of an expression for the dispersion relation of the Serre-Green-Naghdi equations (and the hyperbolic approximation). I would be interested in that.
  6. I think it would be good to add a test, which uses the relaxation callback and tests for exact entropy conservation (I think relaxation does not work out of the box with the current setup because entropy needs the whole vector q and therefore needs to be handled differently).

Generally, I think a subsequent PR (or more than one) that applies some of your ideas from this PR to the other equations can be useful. I can do that once this PR is merged.

src/callbacks_step/analysis.jl Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Show resolved Hide resolved
src/semidiscretization.jl Outdated Show resolved Hide resolved
@JoshuaLampert JoshuaLampert added the enhancement New feature or request label Aug 8, 2024
@ranocha ranocha mentioned this pull request Aug 9, 2024
6 tasks
@JoshuaLampert
Copy link
Owner

We discussed the following things (see also comments inside the code):

  1. What is the plan regarding the full Serre-Green-Naghdi equations and the hyperbolic approximation? Having two separate equations, I guess? To what extend can they share code?

Yes, they are split into two equations that both inherit AbstractSerreGreenNaghdiEquations.

  1. One bigger question I already addressed in some places in the code review: What is the plan regarding the different bathymetries? I would like to have only one equation struct covering all types of bathymetry (in contrast to the current state for the BBM-BBM equations). I think in order to have a smooth generalization for other than constant bathymetries in the future, it already pays of to have a clear plan in mind how this generalization should look like. Especially, should we have another variable in the ODE representing the bathymetry? If we do, then this should already be present in the flat bathymetry case, right?

We will have struct BathymetryFlat, BathymetryVariable and BathymetryMildSlope that are used for dispatch, but do not hold any information about the bathymetry. The bathymetry is still kept as variable in the equation and is set in the initial condition (also for the case of a flat bathymetry). This is especially to be able to potentially support time-dependent bathymetries in the future. This means the two BBM-BBM equations and Svärd-Kalisch equations also need to be adapted (see #130).

  1. From the paper I see there are several possible energy-conserving split forms. You only implemented one, right? I am totally fine with that, but I was only wondering whether you have some plans/ideas to be more flexible?

For now, we focus on the one implemented since we see no big advantages of others.

  1. In the paper you also include the possibility to add artificial diffusion? Do you have any plans for integrating this feature in DispersiveShallowWater.jl? If we want to include it, could this be a general (more or less) equations-independent choice and could be switched on/off on the solver side rather than the equation side? This is definitely not something, we need to decide now, but I already wanted to hear your opinion on that.

Artificial dissipation is left for possible future work.

  1. To have a better impression on the dispersive behaviour of the equations, do you know of an expression for the dispersion relation of the Serre-Green-Naghdi equations (and the hyperbolic approximation). I would be interested in that.

A linear dispersion relation for the hyperbolic approximation can be found in Favrie, Gavrilyuk, A rapid numerical method for solving Serre–Green–Naghdi equations describing long free surface gravity waves (2017).

  1. I think it would be good to add a test, which uses the relaxation callback and tests for exact entropy conservation (I think relaxation does not work out of the box with the current setup because entropy needs the whole vector q and therefore needs to be handled differently).

This will be done once the naming convention of the entropy and entropy_modified is settled because the relaxation approach depends on it.

src/equations/equations.jl Outdated Show resolved Hide resolved
src/equations/equations.jl Outdated Show resolved Hide resolved
src/equations/equations.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Outdated Show resolved Hide resolved
test/test_unit.jl Outdated Show resolved Hide resolved
test/test_unit.jl Outdated Show resolved Hide resolved
test/test_unit.jl Outdated Show resolved Hide resolved
test/test_unit.jl Outdated Show resolved Hide resolved
test/test_unit.jl Outdated Show resolved Hide resolved
@ranocha ranocha requested a review from JoshuaLampert August 14, 2024 06:05
@ranocha
Copy link
Collaborator Author

ranocha commented Aug 14, 2024

Thanks for your review, @JoshuaLampert! I think we're close to the last iteration 🙂

Copy link
Owner

@JoshuaLampert JoshuaLampert left a comment

Choose a reason for hiding this comment

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

Thanks a lot @ranocha, also for the additional type stability tests. I think this is a big step forward!
I have one final iteration of minor comments/questions until this is good to merge.

src/equations/equations.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_flat_1d.jl Show resolved Hide resolved
test/test_serre_green_naghdi_1d.jl Outdated Show resolved Hide resolved
src/equations/equations.jl Show resolved Hide resolved
@ranocha ranocha requested a review from JoshuaLampert August 15, 2024 06:09
Copy link
Owner

@JoshuaLampert JoshuaLampert left a comment

Choose a reason for hiding this comment

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

Thanks again.

@JoshuaLampert JoshuaLampert merged commit fba188f into main Aug 15, 2024
14 checks passed
@JoshuaLampert JoshuaLampert deleted the hr/serre_green_naghdi branch August 15, 2024 07:50
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