Closed patrickersing closed 3 months ago
This checklist is meant to assist creators of PRs (to let them know what reviewers will typically look for) and reviewers (to guide them in a structured review process). Items do not need to be checked explicitly for a PR to be eligible for merging.
NEWS.md
.Created with :heart: by the Trixi.jl community.
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 96.30%. Comparing base (
0f60ebc
) to head (1a89afe
).
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
The modified wave speed affects the downstream tests for Trixi2Vtk.jl
using elixir_shallowwater_ec.jl
on UnstructuredMesh2D
. I think we just need to update the respective reference files in Trixi2Vtk_reference_files.
The modified wave speed affects the downstream tests for
Trixi2Vtk.jl
usingelixir_shallowwater_ec.jl
onUnstructuredMesh2D
. I think we just need to update the respective reference files in Trixi2Vtk_reference_files.
Yes, that is indeed the case. We will need a new reference file.
Good spot!
I am a bit embarrassed that I did not spot this when adding more refined wave speed estimates, as it is for the 1D case only 10 lines below :O
https://github.com/trixi-framework/Trixi.jl/blame/main/src/equations/shallow_water_1d.jl
I am even more embarrassed because I introduced this bug in my initial implementation of the 2D shallow water equations. I have no idea how/why I wrote the function this way. The 1D bug just propagated my initial mistake.
The modified wave speed affects the downstream tests for
Trixi2Vtk.jl
usingelixir_shallowwater_ec.jl
onUnstructuredMesh2D
. I think we just need to update the respective reference files in Trixi2Vtk_reference_files.Yes, that is indeed the case. We will need a new reference file.
Could you please prepare a PR for that? I would like to see green CI before merging this PR.
Do we also need to create new releases for the changes to take effect across packages?
Yes. I just started the registration process for Trixi2Vtk.jl. It should be released in half an hour or so. Please update this PR later so that the new version is used in CI. Afterwards, we can merge this PR and release a new version of Trixi.jl so that CI for Trixi2Vtk.jl works again.
Yes. I just started the registration process for Trixi2Vtk.jl.
The new version merged, so I merged main
into this PR to trigger the CI again.
The wave speed in
max_abs_speeds
for the SWE computes the wave celerity asc = equations.gravity * sqrt(h)
instead ofc = sqrt(equations.gravity * h)
. This PR fixes the computation and adjusts the test values accordingly. In some elixirs thecfl
had to be set back tocfl = 1.0
, because they became unstable when using the new wave speed.