HLL MHD Breaking changes for Consistency

[DanielDoehring]

This closes #1545, closes #1561 and closes #1525 .

This contains now the breaking changes for MHD to have a consistent implementation of the wave speed estimates across SWE, CEE, MHD.

[github-actions]

Review checklist

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.

Purpose and scope

• The PR has a single goal that is clear from the PR title and/or description.
• All code changes represent a single set of modifications that logically belong together.
• No more than 500 lines of code are changed or there is no obvious way to split the PR into multiple PRs.

Code quality

• The code can be understood easily.
• Newly introduced names for variables etc. are self-descriptive and consistent with existing naming conventions.
• There are no redundancies that can be removed by simple modularization/refactoring.
• There are no leftover debug statements or commented code sections.
• The code adheres to our conventions and style guide, and to the Julia guidelines.

Documentation

• New functions and types are documented with a docstring or top-level comment.
• Relevant publications are referenced in docstrings (see example for formatting).
• Inline comments are used to document longer or unusual code sections.
• Comments describe intent ("why?") and not just functionality ("what?").
• If the PR introduces a significant change or new feature, it is documented in NEWS.md.

Testing

• The PR passes all tests.
• New or modified lines of code are covered by tests.
• New or modified tests run in less then 10 seconds.

Performance

• There are no type instabilities or memory allocations in performance-critical parts.
• If the PR intent is to improve performance, before/after time measurements are posted in the PR.

Verification

• The correctness of the code was verified using appropriate tests.
• If new equations/methods are added, a convergence test has been run and the results
  are posted in the PR.

Created with ❤️ by the Trixi.jl community.

[ranocha]

Does this close the issues/PRs you linked above? If so, you can use any of the key words such as closes #123 or fixes #123 to let GitHub close them automatically when this is merged.

[ranocha]

Could you please add a section "Changes when updating to v0.6 from v0.5.x" similar to https://github.com/trixi-framework/Trixi.jl/blob/main/NEWS.md#changes-when-updating-to-v05-from-v04x describing the breaking change made in this PR?

Could you also please add some simple tests of the new code (maybe just unit tests)?

A review from @andrewwinters5000 would be nice 🙂

[codecov]

Codecov Report

Attention: 67 lines in your changes are missing coverage. Please review.

Comparison is base (61c80ba) 83.12% compared to head (29bb3d8) 77.68%.

Additional details and impacted files

@@             Coverage Diff              @@
##           v0.6-dev    #1708      +/-   ##
============================================
- Coverage     83.12%   77.68%   -5.43%     
============================================
  Files           424      424              
  Lines         34234    34296      +62     
============================================
- Hits          28454    26642    -1812     
- Misses         5780     7654    +1874     

Flag	Coverage Δ
unittests	77.68% <6.94%> (-5.43%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
...t_3d_dgsem/elixir_mhd_alfven_wave_nonconforming.jl	100.00% <ø> (ø)
...ples/structured_3d_dgsem/elixir_mhd_alfven_wave.jl	100.00% <ø> (ø)
...ples/tree_1d_dgsem/elixir_mhd_briowu_shock_tube.jl	100.00% <ø> (ø)
...es/tree_1d_dgsem/elixir_mhd_ryujones_shock_tube.jl	100.00% <ø> (ø)
...s/tree_1d_dgsem/elixir_mhd_shu_osher_shock_tube.jl	100.00% <ø> (ø)
src/equations/ideal_glm_mhd_1d.jl	93.50% <11.11%> (-6.50%)	⬇️
src/equations/ideal_glm_mhd_2d.jl	88.01% <6.90%> (-10.78%)	⬇️
src/equations/ideal_glm_mhd_3d.jl	87.70% <5.88%> (-12.30%)	⬇️

... and 49 files with indirect coverage changes

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

[ranocha]

Thanks! I changed the base branch to the development branch @sloede has created for v0.6. When @andrewwinters5000 agrees, we can merge this PR.

[andrewwinters5000]

Thanks for addressing this! I think some comments need updated that I marked in my review comments. Once these are changed this should be good to go.

[ranocha]

The unit test failures at https://github.com/trixi-framework/Trixi.jl/actions/runs/6784509254/job/18440947485?pr=1708#step:7:1220 look like they should be further investigated?

[DanielDoehring]

The unit test failures at https://github.com/trixi-framework/Trixi.jl/actions/runs/6784509254/job/18440947485?pr=1708#step:7:1220 look like they should be further investigated?

Yes, there was a bug in the 2D version of the hlle flux which must somehow slipped through - for 3D, the correct version was already implemented:

Trixi.jl/src/equations/compressible_euler_2d.jl

Line 1354 in 276dc3c

v_roe_mag = (v1_roe * normal_direction[1])^2 + (v2_roe * normal_direction[2])^2

vs

Trixi.jl/src/equations/compressible_euler_3d.jl

Line 1431 in 276dc3c

v_roe_mag = v1_roe^2 + v2_roe^2 + v3_roe^2

I will open a seperate PR for this.

[ranocha]

Thanks!

[sloede]

@DanielDoehring it would be great if you can resolve the conflicts here and then get another review from @andrewwinters5000. Afterwards, I believe, we can merge this to the staging branch

[DanielDoehring]

Maybe before doing so merge #1720 and #1719 into main, then main into this to have some more tests pass

[sloede]

Maybe before doing so merge #1720 and #1719 into main, then main into this to have some more tests pass

Ah, I'll leave this up to @ranocha who has the magic touch re git merging :-) But from where I stand, I believe it might be easier to merge #1720 and #1719 into here and then this PR into the staging branch, but I might be mistaken...

[ranocha]

Maybe before doing so merge #1720 and #1719 into main, then main into this to have some more tests pass

@DanielDoehring I have updated the v0.6 development branch with your PRs merged into main. Please resolve the conflicts.

[DanielDoehring]

Maybe before doing so merge #1720 and #1719 into main, then main into this to have some more tests pass

@DanielDoehring I have updated the v0.6 development branch with your PRs merged into main. Please resolve the conflicts.

Thanks a lot!

[ranocha]

The test failure looks real to me:

1D non-periodic DGMultiMesh: Error During Test at /home/runner/work/Trixi.jl/Trixi.jl/test/test_unit.jl:1318
  Got exception outside of a @test
  MethodError: no method matching Trixi.DGMultiMesh(::Trixi.DGMulti{1, NodesAndModes.Line, StartUpDG.Polynomial{StartUpDG.DefaultPolynomialType}, Trixi.SurfaceIntegralWeakForm{typeof(Trixi.flux_central)}, Trixi.VolumeIntegralFluxDifferencing{typeof(Trixi.flux_central)}, Nothing, StartUpDG.RefElemData{1, NodesAndModes.Line, StartUpDG.Polynomial{StartUpDG.DefaultPolynomialType}, Int64, Tuple{Int64, Int64}, Tuple{Vector{Float64}}, Tuple{Vector{Float64}}, Tuple{Vector{Float64}}, Tuple{Vector{Float64}}, Tuple{Vector{Float64}}, Vector{Int64}, Matrix{Float64}, Matrix{Float64}, Matrix{Float64}, Matrix{Float64}, Matrix{Float64}, Tuple{Matrix{Float64}}, Matrix{Float64}, Matrix{Float64}, Matrix{Float64}, Vector{Float64}, Vector{Float64}}}; cells_per_dimension::Tuple{Int64}, periodicity::Bool)
  
  Closest candidates are:
    Trixi.DGMultiMesh(::Trixi.DGMulti{NDIMS, ElemType, ApproxType, SurfaceIntegral, VolumeIntegral, Mortar, <:StartUpDG.RefElemData{NDIMS, ElemType, ApproxType}} where {ElemType, ApproxType, SurfaceIntegral, VolumeIntegral, Mortar}, ::String; periodicity) where NDIMS got unsupported keyword argument "cells_per_dimension"
     @ Trixi ~/work/Trixi.jl/Trixi.jl/src/solvers/dgmulti/types.jl:302
    Trixi.DGMultiMesh(::Trixi.DGMulti{NDIMS, ElemType, ApproxType, SurfaceIntegral, VolumeIntegral, Mortar, <:StartUpDG.RefElemData{NDIMS, ElemType, ApproxType}} where {ElemType, ApproxType, SurfaceIntegral, VolumeIntegral, Mortar}, ::Any; coordinates_min, coordinates_max, is_on_boundary, periodicity, kwargs...) where NDIMS
     @ Trixi ~/work/Trixi.jl/Trixi.jl/src/solvers/dgmulti/types.jl:239
    Trixi.DGMultiMesh(::Trixi.DGMulti{NDIMS, ElemType, ApproxType, SurfaceIntegral, VolumeIntegral, Mortar, <:StartUpDG.RefElemData{NDIMS, ElemType, ApproxType}} where {ElemType, ApproxType, SurfaceIntegral, VolumeIntegral, Mortar}, ::Any, ::AbstractArray; is_on_boundary, periodicity, kwargs...) where NDIMS
     @ Trixi ~/work/Trixi.jl/Trixi.jl/src/solvers/dgmulti/types.jl:195
    ...

see https://github.com/trixi-framework/Trixi.jl/actions/runs/6822105630/job/18553580689?pr=1708#step:7:1241

Did some merging go wrong? CC @jlchan

[ranocha]

Aha, these CI failures occurred already in #1709. Could you please have a look at it, @jlchan?

[andrewwinters5000]

LGTM! This can merge into the staging branch.

[jlchan]

Aha, these CI failures occurred already in #1709. Could you please have a look at it, @jlchan?

Yep. @DanielDoehring can I fix it in dev and have you merge the change in here?

[jlchan]

@ranocha @DanielDoehring CI should be fixed in #1728

[DanielDoehring]

I merged your branch directly into this, hope that this works

[ranocha]

I'll merge this now. Please do not merge commits from other PRs into PRs, that's just causing trouble in the long run.