Add HLL numerical flux #648

MTCam · 2022-04-27T15:37:11Z

This change set:

adds Harten-Lax-van Leer (HLL) numerical flux routines implemented by @anderson2981,
- https://link.springer.com/chapter/10.1007/978-3-662-03490-3_10?noAccess=true
and adds/refactors numerical flux interfaces supporting user-specified numerical flux routines
- euler.py, inviscid.py, flux.py, viscous.py, boundary.py
adds a suite of tests and extends some existing tests for the new fluxes
- test_bc.py, test_inviscid.py, test_flux.py
adds Toro reference on approximate Riemann solvers and sharpens the documentation on BCs and numerical fluxes
- misc.rst, discretization.rst

majosm review checklist:

Is the scope and purpose of the PR clear?
- The PR should have a description.
- The PR should have a guide if needed (e.g., an ordering).
Is every top-level method and class documented? Are things that should be documented actually so?
Is the interface understandable? (I.e. can someone figure out what stuff does?) Is it well-defined?
Does the implementation do what the docstring claims?
~~Is everything that is implemented covered by tests?~~
- Can't answer this; need @anderson2981 to look
Do you see any immediate risks or performance disadvantages with the design? Example: what do interface normals attach to?

anderson2981 review checklist:

Is the scope and purpose of the PR clear?
- The PR should have a description.
- The PR should have a guide if needed (e.g., an ordering).
Is every top-level method and class documented? Are things that should be documented actually so?
Is the interface understandable? (I.e. can someone figure out what stuff does?) Is it well-defined?
Does the implementation do what the docstring claims?
Is everything that is implemented covered by tests?
Do you see any immediate risks or performance disadvantages with the design? Example: what do interface normals attach to?

majosm

Made a couple of passes, mostly looking at code clarity and documentation. Hopefully @anderson2981 can look more closely at the math.

majosm · 2022-04-28T17:04:49Z

mirgecom/flux.py

+    f_minus_normal
+        Normal component of physical flux interior to (left of) interface
+
+    f_plus_normal
+        Normal component of physical flux exterior to (right of) interface
+
+    q_minus
+        Physical state on interior of interface
+
+    q_plus
+        Physical state on exterior of interface


Maybe use TracePairs instead of separate minus/plus arguments?

Well we wanted this interface to be numerical quantities only with no grudgy data structures down here.

How come? I don't see allowing TracePair as being too egregious... and then you get to use tpair.avg, tpair.diff, etc.

At the time they were written, it allowed just sending numbers or numpy arrays to the low-level flux routines in such a way that we could just copy tests right out of the book. The extra baggage of needing TracePairs data structure to call and test the numerical flux function.

Ah, OK. I guess that's fair.

Unresolving to say: Add a comment justifying the choice to supply different variables.

Took a stab. How's it looking now?

mirgecom/flux.py

mirgecom/inviscid.py

mirgecom/viscous.py

mirgecom/flux.py

mirgecom/inviscid.py

Co-authored-by: Matt Smith <[email protected]>

inducer · 2022-05-02T19:24:08Z

mirgecom/flux.py


+    # choose the correct f contribution based on the wave speeds
+    f_check_minus = \
+        actx.np.greater_equal(s_minus, 0*s_minus)*(0*f_minus_normal + 1.0)


actx.zeros_like(s_minus) or so?

Disclaimer: Doesn't totally work yet: inducer/arraycontext#161

mirgecom/inviscid.py

inducer · 2022-05-09T19:12:12Z

doc/discretization.rst

-$\b{h}^*_e$ is equal to the (interior; - side) pressure contribution of
-$\b{F}^I(\b{Q}_{bc})\cdot\b{n}$
-(since $\b{V}\cdot\b{n} = 0$).
+Adiabtic slip wall


Read to here

inducer · 2022-05-09T19:13:55Z

doc/discretization.rst


+.. image:: figures/ElementBoundary.png


Track down the source file?

https://sphinxcontrib-tikz.readthedocs.io/en/latest/

Track down the source file?

I have it, if it is useful. PPTX

mirgecom/viscous.py

inducer · 2022-05-09T19:28:20Z

mirgecom/viscous.py

+        discr, gas_model, boundaries, interior_state_pairs,
+        domain_boundary_states, grad_cv, interior_grad_cv_pairs,
+        grad_t, interior_grad_t_pairs, quadrature_tag=None,
+        numerical_flux_func=viscous_divergence_flux, time=0.0):


Suggested change

numerical_flux_func=viscous_divergence_flux, time=0.0):

numerical_flux_func=viscous_divergence_flux):

I couldn't figure out how to pass time down through the operator to the boundary without this argument.

I think we can make it a little more obvious that this is just a passthrough to the BCs (e.g. by passing something like a boundary_kwargs argument instead of passing time etc. explicitly), but I suspect this is best saved for another PR.

Can we stash time somewhere in the fluid state? That'd be great? 🤔

inducer · 2022-05-09T19:32:05Z

mirgecom/viscous.py

+def viscous_flux_on_element_boundary(
+        discr, gas_model, boundaries, interior_state_pairs,
+        domain_boundary_states, grad_cv, interior_grad_cv_pairs,
+        grad_t, interior_grad_t_pairs, quadrature_tag=None,


Suggested change

grad_t, interior_grad_t_pairs, quadrature_tag=None,

grad_t, interior_grad_t_pairs, discretization_tag=None,

(here and elsewhere)

Bump to issue? Document to be a discretization tag?

Made an issue, but eager to tackle it here if we can.

majosm

Made another pass through and found some stuff (mostly doc fixes).

doc/discretization.rst

mirgecom/flux.py

mirgecom/viscous.py

Co-authored-by: Matt Smith <[email protected]>

majosm

Github made me split this into 3 suggestions, ugh sorry.

doc/discretization.rst

Co-authored-by: Matt Smith <[email protected]>

majosm

Just had another pass through; looks good on my end. Left a couple of optional thoughts.

majosm · 2022-05-11T21:56:08Z

doc/discretization.rst


+.. image:: figures/ElementBoundary.png


The text renders a little blurry when scaled down to fit in the doc page. Maybe see how it looks without the shadows?

fwiw, we intend to replace this outright with tikz.

majosm · 2022-05-11T21:57:18Z

mirgecom/inviscid.py

+    # calculate left/right wavespeeds
+    actx = state_pair.int.array_context
+    ones = 0.*state_pair.int.mass_density + 1.
+
+    # note for me, treat the interior state as left and the exterior state as right
+    # pressure estimate
+    p_int = state_pair.int.pressure
+    p_ext = state_pair.ext.pressure
+    u_int = np.dot(state_pair.int.velocity, normal)
+    u_ext = np.dot(state_pair.ext.velocity, normal)
+    rho_int = state_pair.int.mass_density
+    rho_ext = state_pair.ext.mass_density
+    c_int = state_pair.int.speed_of_sound
+    c_ext = state_pair.ext.speed_of_sound
+
+    p_star = (0.5*(p_int + p_ext) + (1./8.)*(u_int - u_ext)
+              * (rho_int + rho_ext) * (c_int + c_ext))
+
+    gamma_int = gas_model.eos.gamma(state_pair.int.cv, state_pair.int.temperature)
+    gamma_ext = gas_model.eos.gamma(state_pair.ext.cv, state_pair.ext.temperature)
+
+    q_int = 1 + (gamma_int + 1)/(2*gamma_int)*(p_star/p_int - 1)
+    q_ext = 1 + (gamma_ext + 1)/(2*gamma_ext)*(p_star/p_ext - 1)
+
+    pres_check_int = actx.np.greater(p_star, p_int)
+    pres_check_ext = actx.np.greater(p_star, p_ext)
+
+    q_int_rt = actx.np.sqrt(actx.np.where(pres_check_int, q_int, ones))
+    q_ext_rt = actx.np.sqrt(actx.np.where(pres_check_ext, q_ext, ones))
+
+    # left (internal), and right (external) wave speed estimates
+    # can alternatively use the roe estimated states to find the wave speeds
+    s_minus = u_int - c_int*q_int_rt
+    s_plus = u_ext + c_ext*q_ext_rt
+
+    f_minus_normal = inviscid_flux(state_pair.int)@normal
+    f_plus_normal = inviscid_flux(state_pair.ext)@normal
+
+    q_minus = state_pair.int.cv
+    q_plus = state_pair.ext.cv


Should we be documenting anything here about how the wave speeds are computed? Or should that be obvious to someone with domain knowledge?

These Riemann-solver-based flux routines are well known and documented in detail by Toro, which is linked in the documentation. That said, I don't think it would hurt the pepper in some comments in the implementations where there might be subtle points about our implementation. @anderson2981 wrote this routine - so I suspect he already has comments where he thought he needed them.

those expressions follow almost verbatim from Toro. I'm not sure we need extra comments here.

MTCam · 2022-05-12T21:47:57Z

Anything preventing merge here from your perspective, @inducer?

anderson2981

This passes all the tests I setup initially to verify its implementation from a physics perspective. I don't see any changes that would impeded its proper operation. And the tests are still passing.

MTCam · 2022-05-24T19:23:55Z

Resolved part of #509

Stab wildly at extricating HLL out of NS.

50e1b6d

MTCam force-pushed the add-hll-flux branch from fcc98fc to 50e1b6d Compare April 27, 2022 15:47

MTCam changed the base branch from ns-update to main April 27, 2022 15:48

Test all the flux options in inviscid testing.

cc7cc4c

MTCam mentioned this pull request Apr 27, 2022

Navier-Stokes operator #581

Closed

8 tasks

MTCam added 2 commits April 27, 2022 14:32

Test all the numerical flux funcs in slipwall flux test.

8688b21

Correct some interface mistakes.

5b21d7a

MTCam changed the title ~~Stab wildly at extricating HLL out of NS.~~ Add HLL numerical flux Apr 27, 2022

MTCam marked this pull request as ready for review April 27, 2022 21:15

Accept unknown arguments that are unused.

738c2c8

MTCam requested review from majosm and anderson2981 April 28, 2022 00:14

MTCam added enhancement New feature or request Production Feeder Feeds the production branch labels Apr 28, 2022

Use new flux routine in autoignition example.

4c43e92

majosm reviewed Apr 28, 2022

View reviewed changes

MTCam and others added 12 commits April 28, 2022 15:42

Merge branch 'main' into add-hll-flux

8f13b89

Change doc to match book, switch implementation to look like doc.

4e72608

Remove kwargs, re-use of q_int/ext

1138e69

Sharpen/correct documentation

6f8c52c

Co-authored-by: Matt Smith <[email protected]>

Updates from @majosm review.

1f833cf

Do not run pytest in parallel until #647

589b7c9

Sharpen documentation per @majosm review.

c1d7d2c

Rename numerical flux routines +_facial_

72bc8a5

Merge branch 'main' into add-hll-flux

d799371

Undo production customization

5f4c957

Bug fix symbolic test from #654

785adcd

Merge branch 'main' into add-hll-flux

21f8a9c

inducer reviewed May 2, 2022

View reviewed changes

mirgecom/inviscid.py Outdated Show resolved Hide resolved

Remove kwargs (again), add doc for HLL

5064642

inducer reviewed May 9, 2022

View reviewed changes

Correct misspelling, remove kwargs

2255077

MTCam mentioned this pull request May 9, 2022

Bump to issue? Document to be a discretization tag? #663

Open

majosm reviewed May 9, 2022

View reviewed changes

MTCam and others added 9 commits May 9, 2022 19:54

Apply quicky doc changes from review.

17dec78

Co-authored-by: Matt Smith <[email protected]>

Correct eqn form

06886c5

Merge remote-tracking branch 'origin/add-hll-flux' into save-my-changes

9fed239

Reword

9042e4a

Address review suggestions

e58c306

Fix docs per review comments.

923fbff

More doc updates, fallout from other changes.

6c9675e

Fix typo in eqn doc viscous

4d6359e

Clean up mark up for boundary quantity definitions.

6bb900d

MTCam mentioned this pull request May 10, 2022

Stash time in the fluid state? #664

Open

MTCam added 7 commits May 10, 2022 15:09

Ambiguate return types for pair-type flux funcs.

b9a1aea

Ambiguate return types for pair-type flux funcs.

8f66f87

Ambiguate return types for pair-type flux funcs.

f021349

Remove superflous section of the num flux doc.

2482bb2

Remove dissipative flux options, and convenience wrappers for operators.

d540004

Update docs per removed flux functions

15a6264

Fix formula expn

de7b37a

majosm reviewed May 11, 2022

View reviewed changes

doc/discretization.rst Outdated Show resolved Hide resolved

doc/discretization.rst Outdated Show resolved Hide resolved

doc/discretization.rst Outdated Show resolved Hide resolved

Sharpen docs per review

4e8f68a

Co-authored-by: Matt Smith <[email protected]>

majosm approved these changes May 11, 2022

View reviewed changes

anderson2981 approved these changes May 13, 2022

View reviewed changes

MTCam merged commit 52a6e26 into main May 13, 2022

MTCam deleted the add-hll-flux branch May 13, 2022 16:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add HLL numerical flux #648

Add HLL numerical flux #648

MTCam commented Apr 27, 2022 •

edited by anderson2981

Loading

majosm left a comment

majosm Apr 28, 2022

MTCam Apr 28, 2022

majosm Apr 29, 2022

MTCam Apr 29, 2022

majosm Apr 29, 2022

inducer May 2, 2022

MTCam May 7, 2022

inducer May 2, 2022

inducer May 9, 2022

inducer May 9, 2022

inducer May 9, 2022

MTCam May 9, 2022

inducer May 13, 2022

inducer May 9, 2022

MTCam May 9, 2022

majosm May 9, 2022

MTCam May 9, 2022

inducer May 9, 2022

inducer May 9, 2022

MTCam May 9, 2022

majosm left a comment

majosm left a comment

majosm left a comment

majosm May 11, 2022

MTCam May 12, 2022

MTCam May 13, 2022

majosm May 11, 2022

MTCam May 12, 2022

anderson2981 May 13, 2022

MTCam commented May 12, 2022

anderson2981 left a comment

MTCam commented May 24, 2022

	numerical_flux_func=viscous_divergence_flux, time=0.0):
	numerical_flux_func=viscous_divergence_flux):

	grad_t, interior_grad_t_pairs, quadrature_tag=None,
	grad_t, interior_grad_t_pairs, discretization_tag=None,

Add HLL numerical flux #648

Add HLL numerical flux #648

Conversation

MTCam commented Apr 27, 2022 • edited by anderson2981 Loading

majosm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

majosm left a comment

Choose a reason for hiding this comment

majosm left a comment

Choose a reason for hiding this comment

majosm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MTCam commented May 12, 2022

anderson2981 left a comment

Choose a reason for hiding this comment

MTCam commented May 24, 2022

MTCam commented Apr 27, 2022 •

edited by anderson2981

Loading