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

Use Roe-averaged state in HLLE to compute wave speeds. #93

Merged
merged 3 commits into from
Jan 30, 2024

Conversation

forrestglines
Copy link
Contributor

Changes HLLE to compute wave speeds from the L/R/Roe-averaged states like Athena++ does for adiabatic hydro.

Currently this only changes HLLE for hydro. I'm still exploring whether this change affects disks in a private branch for curvilinear coordinates.

@pgrete
Copy link
Contributor

pgrete commented Jan 23, 2024

Any results on the curvilinear coords (or in other words, should we go ahead and merge this)?

@forrestglines
Copy link
Contributor Author

@par-hermes format

@forrestglines
Copy link
Contributor Author

Any results on the curvilinear coords (or in other words, should we go ahead and merge this)?

We should go ahead and merge this. Although I don't have any comparisons on hand for Cartesian (it passes tests at least), these changes improved the stability of stratified disks in cylindrical and spherical coordinates.

Changing the hydro HLLE also won't affect the INCITE runs since for MHD we use HLLD and hydro we use HLLC.

@forrestglines forrestglines requested a review from pgrete January 27, 2024 23:13
Copy link
Contributor

@pgrete pgrete left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@@ -50,7 +50,7 @@ struct Riemann<Fluid::euler, RiemannSolver::hlle> {
Real gm1 = gamma - 1.0;
Real igm1 = 1.0 / gm1;
parthenon::par_for_inner(member, il, iu, [&](const int i) {
Real wli[(NHYDRO)], wri[(NHYDRO)];
Real wli[(NHYDRO)], wri[(NHYDRO)], wroe[(NHYDRO)];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're only using three out of five variable in wroe[(NHYDRO)] so from a performance point of view (kernel size) might be worth to just use three plain variables.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make a note of this for the flux optimizations I'm doing. Register pressure might be a big bottle neck on the flux kernel performance. This fix would be useful, although I suspect the compile is already optimizing it out.

@forrestglines forrestglines merged commit 522b94a into main Jan 30, 2024
4 checks passed
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.

2 participants