Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove normal_direction_ll in nonconservative terms #2049

Open
patrickersing opened this issue Aug 23, 2024 · 10 comments · May be fixed by #2062
Open

Remove normal_direction_ll in nonconservative terms #2049

patrickersing opened this issue Aug 23, 2024 · 10 comments · May be fixed by #2062

Comments

@patrickersing
Copy link
Contributor

The nonconservative terms can currently depend on both normal_direction_ll and normal_direction_average.
After merging #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
Copy link
Contributor

jlchan commented Aug 23, 2024

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
Copy link
Member

andrewwinters5000 commented Aug 26, 2024

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
Copy link
Contributor Author

@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
Copy link
Contributor

amrueda commented Aug 26, 2024

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
Copy link
Member

andrewwinters5000 commented Aug 26, 2024

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
Copy link
Contributor Author

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 #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
Copy link
Contributor Author

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
Copy link
Contributor

amrueda commented Aug 28, 2024

Here is a first implementation of the subcell shock-capturing for non-conservative systems using the structured mesh in 2D #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
Copy link
Contributor Author

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
Copy link
Contributor

amrueda commented Sep 4, 2024

Thanks, @patrickersing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants