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

Low-Mach correction for HLLC solver #80

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

BenWibking
Copy link
Contributor

@BenWibking BenWibking commented Aug 1, 2023

Includes the low-mach correction for the pressure in the Riemann solver. Does not include carbuncle fix. Based on Minoshima & Miyoshi (2021).

This is an updated version of PR #57.

Closes #18.

@BenWibking
Copy link
Contributor Author

@pgrete @forrestglines I'd like to get this merged in, so I can run my precipitator simulations with upstream AthenaPK and Parthenon. Please let me know what you need from me to do this.

@pgrete
Copy link
Contributor

pgrete commented Jan 29, 2024

This is not the full implementation of the LHLLC solver, i.e., it just contains the fix for the pressure at the contact surface and not the shock fix (based on the velocity divergence), isn't it?

@BenWibking BenWibking changed the title WIP: low-Mach HLLC solver WIP: low-Mach correction for HLLC solver Jan 29, 2024
@BenWibking
Copy link
Contributor Author

This is not the full implementation of the LHLLC solver, i.e., it just contains the fix for the pressure at the contact surface and not the shock fix (based on the velocity divergence), isn't it?

Yes, that's right.

@BenWibking BenWibking mentioned this pull request Feb 28, 2024
@BenWibking BenWibking changed the title WIP: low-Mach correction for HLLC solver Low-Mach correction for HLLC solver Aug 30, 2024
@BenWibking BenWibking marked this pull request as ready for review August 30, 2024 18:06
@BenWibking BenWibking requested a review from pgrete August 30, 2024 18:06
@pgrete
Copy link
Contributor

pgrete commented Sep 2, 2024

Given that this boils down to a 5 line change (with regard to the HLLC solver), I'm wondering if there's a smart (constexpr if way?) around this to reduce the code duplication.

Otherwise, it'd be great if

  • the current version is mentioned in the docs (with a note that it includes only one part of the solver described in the paper)
  • there's a Changelog entry

@BenWibking
Copy link
Contributor Author

Given that this boils down to a 5 line change (with regard to the HLLC solver), I'm wondering if there's a smart (constexpr if way?) around this to reduce the code duplication.

The template metaprogramming to do this while keeping it as a runtime parameter is not trivial. For instance, this is the AMReX implementation: https://github.com/AMReX-Codes/amrex/blob/b78849057c0f533466da6b517a59d2eddc99c1a8/Src/Base/AMReX_CTOParallelForImpl.H#L143

I can add it to CHANGELOG + docs.

@pgrete
Copy link
Contributor

pgrete commented Sep 3, 2024

Given that this boils down to a 5 line change (with regard to the HLLC solver), I'm wondering if there's a smart (constexpr if way?) around this to reduce the code duplication.

The template metaprogramming to do this while keeping it as a runtime parameter is not trivial. For instance, this is the AMReX implementation: https://github.com/AMReX-Codes/amrex/blob/b78849057c0f533466da6b517a59d2eddc99c1a8/Src/Base/AMReX_CTOParallelForImpl.H#L143

Interesting. Might be worth to keep that in mind for the futures.

I can add it to CHANGELOG + docs.

Yes, that'd be great.
And then also a tiny test case (I suggest one more here https://github.com/parthenon-hpc-lab/athenapk/blob/main/tst/regression/test_suites/convergence/convergence.py#L37 and one more here https://github.com/parthenon-hpc-lab/athenapk/blob/main/tst/regression/test_suites/riemann_hydro/riemann_hydro.py#L27C28-L27C69 mirroring the "fiducial" VL2/PLM case).

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

Successfully merging this pull request may close these issues.

add low-mach correction to HLLC solver
2 participants