trixi-framework / Trixi.jl

Trixi.jl: Adaptive high-order numerical simulations of conservation laws in Julia
https://trixi-framework.github.io/Trixi.jl
MIT License
537 stars 109 forks source link

Remove `normal_direction_ll` in nonconservative terms #2049

Closed patrickersing closed 3 weeks ago

patrickersing commented 2 months ago

The nonconservative terms can currently depend on both normal_direction_ll and normal_direction_average. After merging https://github.com/trixi-framework/Trixi.jl/pull/2038 the nonconservative terms for the SWE now only depend on normal_direction_average.

The only other place where normal_direction_ll occurs is the flux_nonconservative_powell in ideal_glm_mhd. I am curious if we could also replace normal_direction_ll here with the averaged one. For entropy conservation it shouldn't matter which normal direction we use (see https://doi.org/10.1016/j.jcp.2018.06.027, C.15-C.17). Running unstructured_2d_dgsem/elixir_mhd_ec.jl also confirms entropy conservation using normal_direction_average. Are there any other considerations why we should use normal_direction_ll instead of normal_direction_average?

If we only needed to pass normal_direction_average, we could unify the function signature of conservative and nonconservative fluxes and could then also use the same boundary condition, which could fix #1445.

jlchan commented 2 months ago

If I remember correctly, wasn't the reason we needed normal_direction_ll due to non-conforming/adaptive meshes? IIRC @andrewwinters5000 mentioned this when I was writing DGMulti solvers.

andrewwinters5000 commented 2 months ago

I do not remember exactly why we needed the normal_direction_ll and normal_direction_average flexibility. I think it might be a vestigial implementation that is a holdover from the old way of selecting the nonconservative terms using the mode which could have been :weak, :whole or :inner. This strategy was removed in #657 when the nonconservative terms were revamped.

We can do some testing for EC and/or EOC for a mesh with hanging nodes and the two available strategies discussed by Patrick above. The main thing I am not sure of is if the sub-cell shock capturing that @amrueda and @bennibolm still requires this flexibilty for the nonconservative to have the _ll and _average normal direction available.

patrickersing commented 2 months ago

@andrewwinters5000 I can run some tests for this. I expect that for nonconforming the normal direction does not make a difference, since we pass the same normal directions at interfaces / mortars (https://github.com/trixi-framework/Trixi.jl/blob/main/src/solvers/dgsem_p4est/dg_2d.jl#L557)

amrueda commented 2 months ago

The main thing I am not sure of is if the sub-cell shock capturing that @amrueda and @bennibolm still requires this flexibilty for the nonconservative to have the _ll and _average normal direction available.

The subcell shock-capturing methods require the _ll and _average normal directions available only if the non-conservative terms are dependent on these directions. Specifically, the methods introduced in #1670 (currently applicable only to tree meshes) necessitate the separate evaluation of the local (_ll) and symmetric components of the non-conservative terms. In the case of the flux_nonconservative_powell method, the flux-differencing formula remains valid if the metric term is shifted from the local (_ll) component to the symmetric part of the GLM non-conservative term.

The proofs for EC/ES would remain valid after converting normal_direction_ll to normal_direction_average. However, I am not sure of what is the impact of this change on the robustness of the schemes. I have already observed that some forms of the non-conservative terms are more robust than others. For example, while implementing #1670, I found that although the Trixi-standard form and the local*symmetric form of the non-conservative terms are algebraically equivalent for conforming meshes, the former is more robust in some tests with non-conforming meshes.

We can do some testing for EC and/or EOC for a mesh with hanging nodes and the two available strategies discussed by Patrick above.

EC/ES tests with non-conforming meshes require EC/ES mortars. Do we already have EC/ES mortars in Trixi?

andrewwinters5000 commented 2 months ago

EC/ES tests with non-conforming meshes require EC/ES mortars. Do we already have EC/ES mortars in Trixi?

Aha, right. No the EC/ES mortars are not available in main. There is an old (now closed) PR from Michael that implemented them #247 , but this would need updated significantly due to subsequent restructuring of Trixi's compute kernels.

patrickersing commented 2 months ago

The subcell shock-capturing methods require the _ll and _average normal directions available only if the non-conservative terms are dependent on these directions. Specifically, the methods introduced in #1670 (currently applicable only to tree meshes) necessitate the separate evaluation of the local (_ll) and symmetric components of the non-conservative terms. In the case of the flux_nonconservative_powell method, the flux-differencing formula remains valid if the metric term is shifted from the local (_ll) component to the symmetric part of the GLM non-conservative term.

It's good to know that it wouldn't interfere with the subcell shock-capturing. Since the implementation uses different functions for symmetric and local parts, you could probably also still use different normal directions and just pass them directly from the kernel.

I have already observed that some forms of the non-conservative terms are more robust than others. For example, while implementing https://github.com/trixi-framework/Trixi.jl/pull/1670, I found that although the Trixi-standard form and the local*symmetric form of the non-conservative terms are algebraically equivalent for conforming meshes, the former is more robust in some tests with non-conforming meshes.

There might be some effect on the volume term from changing the normal direction, but for nonconforming we pass the same normal direction for normal_direction_average and normal_direction_ll, so this would not change anything in regards to nonconforming meshes.

patrickersing commented 2 months ago

Following from our discussion, changing to normal_direction_average should be possible. Currently, the main concern is that the subcell shock-capturing would become less efficient if the normal direction is evaluated in the symmetric part. So we need to investigate how much this change affects performance or alternatively if there is a way to still pass different normal directions in the subcell shock-capturing.

amrueda commented 2 months ago

Here is a first implementation of the subcell shock-capturing for non-conservative systems using the structured mesh in 2D https://github.com/trixi-framework/Trixi.jl/pull/2051. If we change the normal directions to normal_direction_average, we will need 3 evaluations of the non-conservative flux in 2D (1 for Powell and 2 for GLM) and 4 evaluations in 3D (1 for Powell and 3 for GLM), instead of 2. I am not sure how much this will affect performance, but it would be good to investigate it.

patrickersing commented 2 months ago

In the Trixi meeting we discussed, that we could split the problem in two steps:

  1. Remove the normal_direction_ll for the standard fluxes (subcell-shock capturing remains unchanged)
  2. Test performance differences for the subcell shock-capturing and decide if we want to change the normal here as well.

After implementing step one the implementations with/without subcell shock-capturing will be slightly different (normal_direction_ll vs. normal_direction_average) so we should make sure to mention this in the docstring.

amrueda commented 1 month ago

Thanks, @patrickersing!