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 linear dispersion relation #168

Draft
wants to merge 18 commits into
base: main
Choose a base branch
from
Draft

Conversation

JoshuaLampert
Copy link
Owner

@JoshuaLampert JoshuaLampert commented Dec 30, 2024

This adds a new feature to compute the dispersion relation and wave speed of the different equations. It also adds a new page in the docs explaining the general concept of dispersion, which uses the new LinearDispersionRelation to compare the dispersive behavior of the different equations.
I have three questions:

  • Should we provide built-in support for the full dispersion relation? If yes, I'm not sure about the best option. We could add a dummy equation EulerEquation1D as I do in the docs and dispatch on that. This, however, has the disadvantage that it might lead to the confusion that the Euler equations are supported by DispersiveShallowWater.jl.
  • I'm not sure about the hyperbolic Serre-Green-Naghdi equations. There is a dispersion relation given in Favrie and Gavrilyuk, A rapid numerical method for solving Serre-Green-Naghdi equations describing long free surface gravity waves in eq. (19), but I don't know to understand this. Since $\tau = 1/h$, I initially thought, we obtain a classical dispersion relation by $\tau_0 = 1/h_0$, but this doesn't work out dimension-wise. That this doesn't work can also be seen by eq. (18), which differs from the one in the other two papers mentioned in the code. There is an additional third power in the denominator. I think this is related to the fact that the equations are written in Lagrangian form and not in the classical variables, but I'm not sure. Do you know how this dispersion relation would needs to be translated to obtain the classical version, @ranocha?
  • I wanted to double-check that the linear dispersion relation for the BBM equation written as $\eta_t + \sqrt{gD}\eta_x + \frac{3}{2}\sqrt{\frac{g}{D}}\eta\eta_x - \frac{1}{6}D^2\eta_{xxt} = 0$ gives indeed the same as the one for the BBM-BBM equations, but I get another factor of $5/2$ in front (linearizing with $\eta = h_0 + h'$ and $D = h_0$ gives $h'_t + 5/2 \sqrt{g h_0} h'_x - 1/6 h_0^2h_{xxt} = 0$). I haven't thought about that when implementing the BBM equation. I would expect the same dispersion relation as for the BBM-BBM equations. Do you have an idea how to deal with that, @ranocha?

Closes #131.

@JoshuaLampert JoshuaLampert added enhancement New feature or request documentation Improvements or additions to documentation labels Dec 30, 2024
Copy link

codecov bot commented Dec 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.07%. Comparing base (3a32a5c) to head (fa7ae38).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #168      +/-   ##
==========================================
+ Coverage   98.02%   98.07%   +0.04%     
==========================================
  Files          19       20       +1     
  Lines        1877     1923      +46     
==========================================
+ Hits         1840     1886      +46     
  Misses         37       37              

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

Copy link
Contributor

github-actions bot commented Dec 30, 2024

Benchmark Results

main fa7ae38... main/fa7ae385f14741...
bbm_1d/bbm_1d_basic.jl 13.9 ± 0.41 μs 13.7 ± 0.26 μs 1.01
bbm_1d/bbm_1d_fourier.jl 0.267 ± 0.0032 ms 0.214 ± 0.0011 ms 1.24
bbm_bbm_1d/bbm_bbm_1d_basic_reflecting.jl 0.12 ± 0.0016 ms 0.119 ± 0.0032 ms 1.01
bbm_bbm_1d/bbm_bbm_1d_dg.jl 0.0343 ± 0.00052 ms 0.0341 ± 0.00049 ms 1.01
bbm_bbm_1d/bbm_bbm_1d_relaxation.jl 27.5 ± 0.45 μs 27.7 ± 0.45 μs 0.992
bbm_bbm_1d/bbm_bbm_1d_upwind_relaxation.jl 0.0485 ± 0.00054 ms 0.0485 ± 0.00054 ms 1
hyperbolic_serre_green_naghdi_1d/hyperbolic_serre_green_naghdi_dingemans.jl 4.25 ± 0.017 μs 4.17 ± 0.013 μs 1.02
serre_green_naghdi_1d/serre_green_naghdi_well_balanced.jl 0.202 ± 0.0042 ms 0.198 ± 0.004 ms 1.02
svaerd_kalisch_1d/svaerd_kalisch_1d_dingemans_relaxation.jl 0.148 ± 0.0033 ms 0.144 ± 0.0025 ms 1.03
time_to_load 2.03 ± 0.033 s 2.04 ± 0.013 s 0.994

Benchmark Plots

A plot of the benchmark results have been uploaded as an artifact to the workflow run for this PR.
Benchmark Plot

test/test_unit.jl Outdated Show resolved Hide resolved
docs/src/dispersion.md Outdated Show resolved Hide resolved
docs/src/dispersion.md Outdated Show resolved Hide resolved
docs/src/dispersion.md Outdated Show resolved Hide resolved
docs/src/dispersion.md Outdated Show resolved Hide resolved
docs/src/dispersion.md Outdated Show resolved Hide resolved
src/dispersion_relation.jl Outdated Show resolved Hide resolved
src/dispersion_relation.jl Outdated Show resolved Hide resolved
src/dispersion_relation.jl Outdated Show resolved Hide resolved
src/dispersion_relation.jl Outdated Show resolved Hide resolved
@ranocha
Copy link
Collaborator

ranocha commented Jan 9, 2025

I'm not sure about the hyperbolic Serre-Green-Naghdi equations. There is a dispersion relation given in Favrie and Gavrilyuk, A rapid numerical method for solving Serre-Green-Naghdi equations describing long free surface gravity waves in eq. (19), but I don't know to understand this. Since τ = 1 / h, I initially thought, we obtain a classical dispersion relation by τ_0 = 1 / h_0, but this doesn't work out dimension-wise. That this doesn't work can also be seen by eq. (18), which differs from the one in the other two papers mentioned in the code. There is an additional third power in the denominator. I think this is related to the fact that the equations are written in Lagrangian form and not in the classical variables, but I'm not sure. Do you know how this dispersion relation would needs to be translated to obtain the classical version?

Good question... Maybe it would be best to compute the relation from scratch?

@ranocha
Copy link
Collaborator

ranocha commented Jan 9, 2025

I wanted to double-check that the linear dispersion relation for the BBM equation written as [...] I haven't thought about that when implementing the BBM equation. I would expect the same dispersion relation as for the BBM-BBM equations. Do you have an idea how to deal with that?

Why do you expect the dispersion relations to be the same?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Include linear dispersion relation for all equations
2 participants