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

Adapt Svärd-Kalisch equations to Serre-Green-Naghdi equations #146

Merged
merged 19 commits into from
Aug 25, 2024

Conversation

JoshuaLampert
Copy link
Owner

In this PR I try to make the implementation of the Svärd-Kalisch equations follow a more similar style as in the Serre-Green-Naghdi equations. This includes:

  • having a bathymetry_type as additional field. Up to now, only a variable bathymetry is implemented (as we had before). It might be reasonable to also add the possibility to have a bathymetry_flat and then use the higher-order derivative operators for the dispersive terms (this, however, would require to have a third-derivative operator in the solver). I would like to keep this for a separate PR in the future.
  • avoid any allocations in the hyperbolic part. I pre-allocated the necessary data structures in create_cache and overwrite them in rhs! now instead of always allocating them again. Now we have 0 allocations for the hyperbolic part.
  • unify comments and docstrings a bit.
  • reuse solve_system_matrix! from the Serre-Green-Naghdi equations.
  • make some smaller fixes and simplification along the way.

Additionally, I avoided the allocations in energy_total_modified by reusing tmp1. I don't know if this is a good idea on the other hand. This seems dangerous to me since tmp1 will be changed. This also changes the returned value, right? Should I revert this change and allocate a new vector each time again, @ranocha?

@JoshuaLampert
Copy link
Owner Author

I plan to make a similar PR for the BBM-BBM equations in the next days, but this will be breaking because it will unify the two equation systems.

Copy link

codecov bot commented Aug 21, 2024

Codecov Report

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

Project coverage is 97.92%. Comparing base (99583ab) to head (40c19c1).
Report is 2 commits behind head on main.

Files Patch % Lines
src/equations/svaerd_kalisch_1d.jl 98.37% 2 Missing ⚠️
src/equations/equations.jl 94.44% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #146      +/-   ##
==========================================
+ Coverage   97.76%   97.92%   +0.15%     
==========================================
  Files          23       23              
  Lines        1747     1784      +37     
==========================================
+ Hits         1708     1747      +39     
+ Misses         39       37       -2     

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

@JoshuaLampert JoshuaLampert requested a review from ranocha August 21, 2024 20:04
Copy link
Collaborator

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

Nice 👍 I have not checked all changes in detail since I assume everything should be fine when tests pass.

src/equations/equations.jl Outdated Show resolved Hide resolved
src/equations/equations.jl Outdated Show resolved Hide resolved
src/equations/svaerd_kalisch_1d.jl Outdated Show resolved Hide resolved
src/equations/svaerd_kalisch_1d.jl Show resolved Hide resolved
src/equations/svaerd_kalisch_1d.jl 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_1d.jl Outdated Show resolved Hide resolved
src/equations/svaerd_kalisch_1d.jl Outdated Show resolved Hide resolved
@JoshuaLampert JoshuaLampert requested a review from ranocha August 22, 2024 11:17
src/equations/hyperbolic_serre_green_naghdi_1d.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_1d.jl Show resolved Hide resolved
src/semidiscretization.jl Outdated Show resolved Hide resolved
@JoshuaLampert JoshuaLampert requested a review from ranocha August 23, 2024 08:43
src/equations/hyperbolic_serre_green_naghdi_1d.jl Outdated Show resolved Hide resolved
src/equations/serre_green_naghdi_1d.jl Outdated Show resolved Hide resolved
src/equations/svaerd_kalisch_1d.jl Outdated Show resolved Hide resolved
@ranocha
Copy link
Collaborator

ranocha commented Aug 24, 2024

I only have some minor suggestions for improving the documentation. From my point of view, this PR can be merged afterward. Thanks a lot!

@JoshuaLampert JoshuaLampert requested a review from ranocha August 24, 2024 16:31
Copy link
Collaborator

@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

@JoshuaLampert JoshuaLampert merged commit 1f8f932 into main Aug 25, 2024
16 checks passed
@JoshuaLampert JoshuaLampert deleted the bathymetry-type-SK branch August 25, 2024 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants