Hll 2 wave improvements #1545

DanielDoehring · 2023-06-21T09:24:50Z

This addresses #1525 by providing implementations for the standard 2-wave HLL solver for SWE, MHD, CEE as well as the improved 2-wave HLL solver by Einfeldt using Roe averages for SWE.
For completeness, the naive 2-wave HLL solver is added also for MHD.

The motivation for this pull request is

Increase consistency for SWE, MHD, CEE
Provide the standard 2-Wave HLL as the default when calling flux_hll as one would expect

For illustration, the solution of the Sedov Blast wave is compared for the previously default min_max_speed_naive to the new default min_max_speed and flux_hllc.

min_max_speed_naive

min_max_speed

flux_hllc

codecov · 2023-06-21T14:50:57Z

Codecov Report

Merging #1545 (f741306) into main (13b26a3) will decrease coverage by 0.03%.
The diff coverage is 93.75%.

@@            Coverage Diff             @@
##             main    #1545      +/-   ##
==========================================
- Coverage   96.04%   96.02%   -0.03%     
==========================================
  Files         368      369       +1     
  Lines       31049    31394     +345     
==========================================
+ Hits        29821    30144     +323     
- Misses       1228     1250      +22

Flag	Coverage Δ
unittests	`96.02% <93.75%> (-0.03%)`	⬇️

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

Impacted Files	Coverage Δ
examples/dgmulti_2d/elixir_euler_bilinear.jl	`100.00% <ø> (ø)`
examples/dgmulti_2d/elixir_euler_curved.jl	`100.00% <ø> (ø)`
examples/dgmulti_2d/elixir_euler_weakform.jl	`100.00% <ø> (ø)`
examples/dgmulti_3d/elixir_euler_curved.jl	`100.00% <ø> (ø)`
examples/dgmulti_3d/elixir_euler_weakform.jl	`100.00% <ø> (ø)`
...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% <ø> (ø)`
... and 12 more

JoshuaLampert · 2023-06-21T14:53:21Z

I haven't thought this through, but I'm just curious. Would something like the following work? Since min_max_speed (and also max_abs_speeds ?) only depend on the eigenvalues of the flux-jacobian of the equations we could define a general function like

function min_max_speed(u_ll, u_rr, orientation::Integer, equations::AbstractEquations)
    λ_ll = eigenvalues(u_ll, orientation, equations)
    λ_rr = eigenvalues(u_rr, orientation, equations)
    λ_min = min(λ_ll)
    λ_max = max(λ_rr)
    return λ_min, λ_max
end

and define for each equation a function returning the eigenvalues, e.g.

function eigenvalues(u, orientation::Integer, equations::CompressibleEulerEquations1D)
    rho, v1, p = cons2prim(u, equations)

    c = sqrt(equations.gamma * p / rho)
    return v1 - c1, v1, v1 + c1
end

We probably need analogous definitions for min_max_speed(u_ll, u_rr, normal_direction::AbstractVector, equations::AbstractEquations) and eigenvalues(u, normal_direction::AbstractVector, equations::CompressibleEulerEquations1D).
For me, it would be more natural to provide the eigenvalues of a system than to provide a function min_max_speed when one wants to add a new equation since the eigenvalues are more equation-specific and, as far as I understand, at least for the "normal" equations like Euler or SWE, min_max_speed is generic. Maybe even some other functions that only depend on the eigenvalues could be defined generically? Or would this approach be too general and would not work for more sophisticated equations (IIRC @andrewwinters5000 mentioned that the eigenvalues are not known for the two-layer SWE for example)? For me, ideally, it should be necessary for the definition of an equation to provide the minimal set of functions conisting of flux, eigenvalues and maybe some conversion functions like prim2cons and cons2prim if needed. This should be enough to, at least have a working equation with the basic numerical fluxes like flux_lax_friedrichs or flux_hll, shouldn't it? If you have some more sophisticated estimates on the minimal and maximal speeds like Roe averages or if you have a more sophisticated definition for the HLL flux (like for SWE to avoid spurious oscillations in the bottom topography) you can of course provide them for the specific equation, but at least you have some general fallback.

DanielDoehring · 2023-06-21T16:16:46Z

So for the classical HLL 2-Wave solver you would have

function min_max_speed(u_ll, u_rr, orientation::Integer, equations::AbstractEquations)
    λ_ll = eigenvalues(u_ll, orientation, equations)
    λ_rr = eigenvalues(u_rr, orientation, equations)
    λ_min = min(min(λ_ll), min(λ_rr))
    λ_max = max(max(λ_ll), max((λ_rr))
    return λ_min, λ_max
end

which is probably more expensive than the current implementations.
While I can see the elegance in the proposed implementation (especially for SWE and CEE) it is questionable whether this is well suited for e.g. MHD which comes (even in the 1D case) with 8 eigenvalues of which you in principle need only 2.

JoshuaLampert · 2023-06-21T16:19:10Z

So for the classical HLL 2-Wave solver you would have

Yes.

Performance-wise you are probably right. Of course, I am totally fine if we don't implement this because of performance issues. I was just mainly curious if this would work in general.

This reverts commit 047a5e7.

ranocha

Thanks for your effort! This is quite a lot of stuff. It is helpful to break big changes like this down to several smaller PRs to speed up the review process.

src/equations/compressible_euler_1d.jl

src/equations/numerical_fluxes.jl

ranocha

Thanks a lot! This is already in a good shape. However, it contains some breaking changes along with other implementations that can be useful right now. Thus, I would like to ask you to split this into two PRs.

One PR can implement the purely new functionality. This will be not breaking and already a nice addition.
Another PR can implement the breaking changes of the behavior for MHD.

When I drafted the interface of FluxHLL originally in another package in 217 or so, my version of what we call min_max_speed_naive in Trixi.jl was basically what you understand as min_max_speed_davis. I agree with you that this is probably one of the first things people may expect since it easy and already quite good. Thus, I wonder whether we really need the sometimes unexpected version of min_max_speed_naive in Trixi.jl or whether we should replace it by minmax_speed_davis in the long run. Any thoughts about this, @trixi-framework/principal-developers?

examples/structured_1d_dgsem/elixir_euler_sedov_hll_Davis.jl

src/equations/compressible_euler_2d.jl

src/equations/compressible_euler_3d.jl

ranocha · 2023-07-05T14:19:38Z

src/equations/ideal_glm_mhd_1d.jl

 """
-    min_max_speed_naive(u_ll, u_rr, orientation, equations::IdealGlmMhdEquations1D)
+    min_max_speed_einfeldt(u_ll, u_rr, orientation::Integer, equations::IdealGlmMhdEquations1D)

 Calculate minimum and maximum wave speeds for HLL-type fluxes as in
 - Li (2005)
  An HLLC Riemann solver for magneto-hydrodynamics
-  [DOI: 10.1016/j.jcp.2004.08.020](https://doi.org/10.1016/j.jcp.2004.08.020)
+  [DOI: 10.1016/j.jcp.2004.08.020](https://doi.org/10.1016/j.jcp.2004.08.020).
 """
-@inline function min_max_speed_naive(u_ll, u_rr, orientation::Integer,
-                                     equations::IdealGlmMhdEquations1D)
+@inline function min_max_speed_einfeldt(u_ll, u_rr, orientation::Integer,
+                                        equations::IdealGlmMhdEquations1D)
    rho_ll, rho_v1_ll, _ = u_ll
    rho_rr, rho_v1_rr, _ = u_rr


This is the part that I consider to be breaking since it changes the behavior we have so far quite a bit.

ranocha · 2023-07-05T14:21:47Z

src/equations/ideal_glm_mhd_1d.jl

+# Calculate estimates for minimum and maximum wave speeds for HLL-type fluxes
+@inline function min_max_speed_naive(u_ll, u_rr, orientation::Integer,
+                                     equations::IdealGlmMhdEquations1D)
+    rho_ll, rho_v1_ll, _ = u_ll
+    rho_rr, rho_v1_rr, _ = u_rr
+
+    # Calculate primitive variables
+    v1_ll = rho_v1_ll / rho_ll
+    v1_rr = rho_v1_rr / rho_rr
+
+    λ_min = v1_ll - calc_fast_wavespeed(u_ll, orientation, equations)
+    λ_max = v1_rr + calc_fast_wavespeed(u_rr, orientation, equations)
+
+    return λ_min, λ_max
+end


I wonder whether we really need this.

Reason I added this is to have flux_hll working out of the box also for MHD.
But in principle, I agree with you that the naive estimate has probably no real advantage over the Davis estimate.

ranocha · 2023-07-05T14:23:55Z

src/equations/linearized_euler_2d.jl

+# Calculate estimate for minimum and maximum wave speeds for HLL-type fluxes
+@inline function min_max_speed_naive(u_ll, u_rr, orientation::Integer,
+                                     equations::LinearizedEulerEquations2D)
+    min_max_speed_davis(u_ll, u_rr, orientation, equations)
+end
+
+@inline function min_max_speed_naive(u_ll, u_rr, normal_direction::AbstractVector,
+                                     equations::LinearizedEulerEquations2D)
+    min_max_speed_davis(u_ll, u_rr, normal_direction, equations)
+end


That's what I would consider as default, too. Thus, I wonder whether we really need the current version of min_max_speed_naive or whether we should basically deprecate it in favor of min_max_speed_davis.

src/equations/shallow_water_1d.jl

src/equations/shallow_water_2d.jl

test/test_structured_1d.jl

Co-authored-by: Hendrik Ranocha <[email protected]>

…rixi.jl into HLL_2_Wave_Improvements

DanielDoehring · 2023-07-06T13:50:27Z

Thanks a lot! This is already in a good shape. However, it contains some breaking changes along with other implementations that can be useful right now. Thus, I would like to ask you to split this into two PRs.
1. One PR can implement the purely new functionality. This will be not breaking and already a nice addition.

2. Another PR can implement the breaking changes of the behavior for MHD.
When I drafted the interface of FluxHLL originally in another package in 217 or so, my version of what we call min_max_speed_naive in Trixi.jl was basically what you understand as min_max_speed_davis. I agree with you that this is probably one of the first things people may expect since it easy and already quite good. Thus, I wonder whether we really need the sometimes unexpected version of min_max_speed_naive in Trixi.jl or whether we should replace it by minmax_speed_davis in the long run. Any thoughts about this, @trixi-framework/principal-developers?

I extraced the (hopefully) non-breaking part into #1561 and converted this to draft until we decide how we settle the naive vs davis debate.

sloede · 2023-11-04T06:21:04Z

With the upcoming breaking release of v0.6 coming up, is this PR still considered to be merged?

DanielDoehring · 2023-11-04T08:49:47Z

@sloede The main motivation is still there: The naming of wave-speeds is not consistent among equations, most notably for MHD things are different from SWE and CEE.

sloede · 2023-11-04T09:53:37Z

OK. But is this PR in a shape that can be made ready to merge within the next, say, week?

DanielDoehring · 2023-11-04T10:56:36Z

Probably its easier to start over from main branch, but this should be possible within the next week.

sloede · 2023-11-04T12:00:16Z

OK. I'm not saying it has to be done though. But now would be the time for breaking changes 😄

ranocha · 2023-11-06T07:37:22Z

Yes, that would be nice 👍

DanielDoehring · 2023-11-06T08:20:26Z

The new PR is #1708

DanielDoehring added 12 commits June 20, 2023 18:17

Add Classical and Naive HLL 2 Wave solver to classic Hyperbolic PDEs

c6882c7

Format Code

0b74c59

HLLE wave speeds for SWE

0288814

Fix typos

1145f9f

Update tests for HLL

3ce5b3b

Unit test 1D MHD HLL, HLLE

2364017

Add example for classical HLL 2 wave

6c64eba

remove plots

9de6fac

Use lowercase for flux

b10b54d

Use einfeldt for mhd

2c14c35

Use hlle for mhd tets

85403b1

Missing comma causes failing tests

f068fd0

DanielDoehring added 5 commits June 22, 2023 16:04

Correct bug in SWE 2D Roe eigval comp, unit tests

d184578

format

047a5e7

Revert "format"

3e5c402

This reverts commit 047a5e7.

format equations

ce72be0

Add unit tests for HLL naive

f7b2b1e

ranocha requested changes Jun 25, 2023

View reviewed changes

src/equations/compressible_euler_1d.jl Outdated Show resolved Hide resolved

src/equations/numerical_fluxes.jl Outdated Show resolved Hide resolved

src/equations/numerical_fluxes.jl Outdated Show resolved Hide resolved

ranocha added the breaking label Jun 25, 2023

DanielDoehring added 2 commits July 3, 2023 09:58

Revert default hll flux

f1e7f5b

Rename min_max_speed to min_max_speed_davis and reduce documentation

5a03dfa

DanielDoehring requested a review from ranocha July 5, 2023 07:38

Merge branch 'main' into HLL_2_Wave_Improvements

36e9c5e

ranocha requested changes Jul 5, 2023

View reviewed changes

DanielDoehring and others added 2 commits July 6, 2023 10:59

Update src/equations/shallow_water_1d.jl: Comments

7cfc22c

Co-authored-by: Hendrik Ranocha <[email protected]>

Add published resource for Roe averages for SWE

ef6d070

DanielDoehring added 2 commits July 6, 2023 11:37

Merge branch 'HLL_2_Wave_Improvements' of github.com:DanielDoehring/T…

c079f2c

…rixi.jl into HLL_2_Wave_Improvements

Add tests for rotation

f741306

DanielDoehring mentioned this pull request Jul 6, 2023

Hll 2 wave improvements non breaking #1561

Merged

DanielDoehring marked this pull request as draft July 6, 2023 12:43

ranocha mentioned this pull request Jul 14, 2023

Release v0.6 #1310

Closed

10 tasks

This was referenced Sep 22, 2023

Implement the Davis signal speeds in HLL solver for compressible Euler equations, #180

Closed

Estimates of wave speeds #134

Open

DanielDoehring mentioned this pull request Oct 27, 2023

HLLE CEE 2D3D NonCartesian Meshes #1692

Merged

DanielDoehring mentioned this pull request Nov 6, 2023

HLL MHD Breaking changes for Consistency #1708

Merged

ranocha mentioned this pull request Nov 10, 2023

Release v0.7 #1726

Closed

8 tasks

DanielDoehring closed this Nov 10, 2023

DanielDoehring deleted the HLL_2_Wave_Improvements branch November 10, 2023 15:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hll 2 wave improvements #1545

Hll 2 wave improvements #1545

DanielDoehring commented Jun 21, 2023

codecov bot commented Jun 21, 2023 •

edited

Loading

JoshuaLampert commented Jun 21, 2023 •

edited

Loading

DanielDoehring commented Jun 21, 2023

JoshuaLampert commented Jun 21, 2023 •

edited

Loading

ranocha left a comment

ranocha left a comment

ranocha Jul 5, 2023

ranocha Jul 5, 2023

DanielDoehring Jul 6, 2023

ranocha Jul 5, 2023

DanielDoehring commented Jul 6, 2023

sloede commented Nov 4, 2023

DanielDoehring commented Nov 4, 2023

sloede commented Nov 4, 2023

DanielDoehring commented Nov 4, 2023 •

edited

Loading

sloede commented Nov 4, 2023

ranocha commented Nov 6, 2023

DanielDoehring commented Nov 6, 2023

Hll 2 wave improvements #1545

Hll 2 wave improvements #1545

Conversation

DanielDoehring commented Jun 21, 2023

codecov bot commented Jun 21, 2023 • edited Loading

Codecov Report

JoshuaLampert commented Jun 21, 2023 • edited Loading

DanielDoehring commented Jun 21, 2023

JoshuaLampert commented Jun 21, 2023 • edited Loading

ranocha left a comment

Choose a reason for hiding this comment

ranocha left a comment

Choose a reason for hiding this comment

ranocha Jul 5, 2023

Choose a reason for hiding this comment

ranocha Jul 5, 2023

Choose a reason for hiding this comment

DanielDoehring Jul 6, 2023

Choose a reason for hiding this comment

ranocha Jul 5, 2023

Choose a reason for hiding this comment

DanielDoehring commented Jul 6, 2023

sloede commented Nov 4, 2023

DanielDoehring commented Nov 4, 2023

sloede commented Nov 4, 2023

DanielDoehring commented Nov 4, 2023 • edited Loading

sloede commented Nov 4, 2023

ranocha commented Nov 6, 2023

DanielDoehring commented Nov 6, 2023

codecov bot commented Jun 21, 2023 •

edited

Loading

JoshuaLampert commented Jun 21, 2023 •

edited

Loading

JoshuaLampert commented Jun 21, 2023 •

edited

Loading

DanielDoehring commented Nov 4, 2023 •

edited

Loading