Skip to content

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

3 participants