Skip to content

Comments

Add comparison with experimental data to docs#238

Merged
JoshuaLampert merged 18 commits intomainfrom
experimental-data-docs
Aug 28, 2025
Merged

Add comparison with experimental data to docs#238
JoshuaLampert merged 18 commits intomainfrom
experimental-data-docs

Conversation

@JoshuaLampert
Copy link
Member

No description provided.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 26, 2025

Benchmark Results (Julia v1.10)

Time benchmarks
main 0de2069... main / 0de2069...
bbm_1d/bbm_1d_basic.jl - rhs!: 14 ± 0.51 μs 14 ± 0.44 μs 1 ± 0.048
bbm_1d/bbm_1d_fourier.jl - rhs!: 0.223 ± 0.31 ms 0.224 ± 0.31 ms 0.995 ± 2
bbm_bbm_1d/bbm_bbm_1d_basic_reflecting.jl - rhs!: 0.0815 ± 0.00047 ms 0.0805 ± 0.00042 ms 1.01 ± 0.0079
bbm_bbm_1d/bbm_bbm_1d_dg.jl - rhs!: 0.0345 ± 0.00051 ms 0.0347 ± 0.00089 ms 0.995 ± 0.03
bbm_bbm_1d/bbm_bbm_1d_relaxation.jl - rhs!: 27.2 ± 0.42 μs 27.4 ± 0.45 μs 0.994 ± 0.022
bbm_bbm_1d/bbm_bbm_1d_upwind_relaxation.jl - rhs!: 0.0485 ± 0.00053 ms 0.0486 ± 0.00056 ms 0.999 ± 0.016
hyperbolic_serre_green_naghdi_1d/hyperbolic_serre_green_naghdi_dingemans.jl - rhs!: 4.23 ± 0.031 μs 4.28 ± 0.039 μs 0.988 ± 0.012
kdv_1d/kdv_1d_basic.jl - rhs!: 1.47 ± 0.02 μs 1.4 ± 0.011 μs 1.05 ± 0.016
kdv_1d/kdv_1d_implicit.jl - rhs!: 1.44 ± 0.011 μs 1.4 ± 0.011 μs 1.03 ± 0.011
serre_green_naghdi_1d/serre_green_naghdi_well_balanced.jl - rhs!: 0.201 ± 0.0089 ms 0.2 ± 0.0089 ms 1 ± 0.063
svaerd_kalisch_1d/svaerd_kalisch_1d_dingemans_relaxation.jl - rhs!: 0.148 ± 0.0049 ms 0.149 ± 0.0054 ms 0.997 ± 0.049
time_to_load 1.93 ± 0.019 s 1.9 ± 0.02 s 1.01 ± 0.015
Memory benchmarks
main 0de2069... main / 0de2069...
bbm_1d/bbm_1d_basic.jl - rhs!: 1 allocs: 4.12 kB 1 allocs: 4.12 kB 1
bbm_1d/bbm_1d_fourier.jl - rhs!: 1 allocs: 4.12 kB 1 allocs: 4.12 kB 1
bbm_bbm_1d/bbm_bbm_1d_basic_reflecting.jl - rhs!: 5 allocs: 1.17 kB 5 allocs: 1.17 kB 1
bbm_bbm_1d/bbm_bbm_1d_dg.jl - rhs!: 10 allocs: 8.62 kB 10 allocs: 8.62 kB 1
bbm_bbm_1d/bbm_bbm_1d_relaxation.jl - rhs!: 2 allocs: 8.25 kB 2 allocs: 8.25 kB 1
bbm_bbm_1d/bbm_bbm_1d_upwind_relaxation.jl - rhs!: 2 allocs: 8.25 kB 2 allocs: 8.25 kB 1
hyperbolic_serre_green_naghdi_1d/hyperbolic_serre_green_naghdi_dingemans.jl - rhs!: 0 allocs: 0 B 0 allocs: 0 B
kdv_1d/kdv_1d_basic.jl - rhs!: 0 allocs: 0 B 0 allocs: 0 B
kdv_1d/kdv_1d_implicit.jl - rhs!: 0 allocs: 0 B 0 allocs: 0 B
serre_green_naghdi_1d/serre_green_naghdi_well_balanced.jl - rhs!: 0.075 k allocs: 0.66 MB 0.075 k allocs: 0.66 MB 1
svaerd_kalisch_1d/svaerd_kalisch_1d_dingemans_relaxation.jl - rhs!: 0.042 k allocs: 0.315 MB 0.042 k allocs: 0.315 MB 1
time_to_load 0.153 k allocs: 14.5 kB 0.153 k allocs: 14.5 kB 1

@JoshuaLampert
Copy link
Member Author

@ranocha, what would you suggest where to put the experimental data? In the main repo like now, in a gist, which is downloaded, or something else?

@JoshuaLampert JoshuaLampert added the documentation Improvements or additions to documentation label Aug 26, 2025
@JoshuaLampert JoshuaLampert marked this pull request as ready for review August 26, 2025 13:51
@JoshuaLampert JoshuaLampert mentioned this pull request Aug 25, 2025
6 tasks
@JoshuaLampert JoshuaLampert added the enhancement New feature or request label Aug 26, 2025
@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@ranocha
Copy link
Member

ranocha commented Aug 26, 2025

Why are the results for the BBM-BBM equations so bad? The plot in https://numericalmathematics.github.io/DispersiveShallowWater.jl/previews/PR238/dingemans/#Visualization-of-Temporal-Evolution does not look good for BBM-BBM. If I remember correctly, we had reasonable results for BBM-BBM in your paper, didn't we?

@JoshuaLampert
Copy link
Member Author

JoshuaLampert commented Aug 26, 2025

Why are the results for the BBM-BBM equations so bad? The plot in numericalmathematics.github.io/DispersiveShallowWater.jl/previews/PR238/dingemans#Visualization-of-Temporal-Evolution does not look good for BBM-BBM. If I remember correctly, we had reasonable results for BBM-BBM in your paper, didn't we?

In the paper we used N = 512 and accuracy_order = 4 for the plot comparing the different equations with the snapshots at different times and an upwind solver with N = 1024 and accuracy_order = 6 for the comparison with experimental data at the wave gauges. Both look better than the current central version with N = 1024 and accuracy_order = 6. But I wanted to use the higher resolution in the docs to have nice pictures especially for Svärd-Kalisch and I didn't want to use upwind operators because we cannot use them for the hyperbolic SGN equations.

@coveralls
Copy link
Collaborator

coveralls commented Aug 26, 2025

Pull Request Test Coverage Report for Build 17289102680

Details

  • 8 of 8 (100.0%) changed or added relevant lines in 2 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.006%) to 98.301%

Totals Coverage Status
Change from base Build 17229674130: 0.006%
Covered Lines: 2314
Relevant Lines: 2354

💛 - Coveralls

@ranocha
Copy link
Member

ranocha commented Aug 27, 2025

Why are the results for the BBM-BBM equations so bad? The plot in numericalmathematics.github.io/DispersiveShallowWater.jl/previews/PR238/dingemans#Visualization-of-Temporal-Evolution does not look good for BBM-BBM. If I remember correctly, we had reasonable results for BBM-BBM in your paper, didn't we?

In the paper we used N = 512 and accuracy_order = 4 for the plot comparing the different equations with the snapshots at different times and an upwind solver with N = 1024 and accuracy_order = 6 for the comparison with experimental data at the wave gauges. Both look better than the current central version with N = 1024 and accuracy_order = 6. But I wanted to use the higher resolution in the docs to have nice pictures especially for Svärd-Kalisch and I didn't want to use upwind operators because we cannot use them for the hyperbolic SGN equations.

I see. On the other hand, we now know that the plain wide-stencil discretizations of the second derivative have issues for solving the elliptic systems. I think we should modify the current version:

  • Simple but less satisfying: Mention these issues in the docs to explain why the BBM-BBM results are not so good
  • A bit more involved: Use upwind operators for the elliptic terms or try to use a higher resolution, and explain why we did so. Ideally, this would improve the BBM-BBM results.

Currently, the conclusion favors SK significantly while the results are not only influenced by the model but also the discretization.

@JoshuaLampert
Copy link
Member Author

I switched to upwind solvers for the BBM-BBM and the SK equations in f861bcb. This improves the solution for BBM-BBM a bit, but it is still relatively oscillatory at some places. The second figure should now show the same numerical values as the last two figures from the preprint (SK set 2 in the second to last figure and BBM-BBM in the last). If you agree, I would keep it like this and add a sentence why we use upwind operators for BBM-BBM and Svärd-Kalisch and central for the others. Should we use upwind or central for SGN? If we want to use an upwind operator, should it also have accuracy_order - 1 (like here)? Why was that needed again for SGN?

@ranocha
Copy link
Member

ranocha commented Aug 27, 2025

Thanks! This looks better.

If you agree, I would keep it like this and add a sentence why we use upwind operators for BBM-BBM and Svärd-Kalisch and central for the others.

👍

Should we use upwind or central for SGN?

I would expect upwind to perform at least as good as or better than the central one, so I would check the upwind version.

If we want to use an upwind operator, should it also have accuracy_order - 1 (like here)? Why was that needed again for SGN?

If I remember correctly, the upwind operators are only used for higher-order derivatives. In particular, they will be used symmetrically for the second-derivative terms. Thus, these terms will have an even order of accuracy, i.e., the accuracy will be one order higher than that of the individual upwind operators. The central first-derivative operator induced by the upwind operators has the same property, i.e., setting accuracy_order = 5 for the upwind operators results in a sixth-order accurate scheme in the end. Thus, I would typically choose the fifth-order upwind operators since they have a smaller stencil and will be faster than the sixth-order operators.

We can briefly comment on this in the docs and choose accuracy_order = 5 for all upwind discretizations.

@JoshuaLampert
Copy link
Member Author

I now switched to upwind operators also for the SGN equations and construct the upwind operators with an accuracy_order = 5. I also added a paragraph explaining the use of the upwind operators.

If I remember correctly, the upwind operators are only used for higher-order derivatives.

We do use the .central part of the upwind operator also for first-order derivatives, cf.

and also the directed derivatives, cf.

So now these use first-derivative upwind operators of odd order. Is this problematic?

@JoshuaLampert JoshuaLampert requested a review from ranocha August 27, 2025 14:35
Copy link
Member

@ranocha ranocha left a comment

Choose a reason for hiding this comment

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

Thanks! That's fine since the central operator has also an even order (one order higher, i.e., sixth-order accurate in our case) and the upwind terms are treated symmetrically. For example,

mul!(v_x_upwind, D1_upwind.minus, v)

is used together with

@.. tmp = 0.5 * h^2 * (h * v_x + h_x * v) * v_x_upwind
mul!(p_x, D1_upwind.plus, tmp)

to get a symmetric second-derivative discretization with variable coefficient. The same holds for

mul!(eta_x_upwind, D1.plus, eta)

and

@.. tmp1 = alpha_hat * eta_x_upwind
mul!(alpha_eta_x_x, D1.minus, tmp1)

Co-authored-by: Hendrik Ranocha <ranocha@users.noreply.github.com>
ranocha
ranocha previously approved these changes Aug 28, 2025
@JoshuaLampert
Copy link
Member Author

I added a NEWS entry. Do you mind re-approving, @ranocha?

@JoshuaLampert JoshuaLampert requested a review from ranocha August 28, 2025 07:36
@JoshuaLampert
Copy link
Member Author

Whoops, you were faster than I requesting your review 😅 Thanks!

@JoshuaLampert JoshuaLampert enabled auto-merge (squash) August 28, 2025 07:38
@JoshuaLampert JoshuaLampert merged commit 05e2ac0 into main Aug 28, 2025
11 checks passed
@JoshuaLampert JoshuaLampert deleted the experimental-data-docs branch August 28, 2025 07:55
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.

4 participants