Fix & refector: Incorrect v^2 calc in some pgens, and move Reducers out of Driver class. #188
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Yurlungur
approved these changes
Dec 2, 2023
| @@ -78,6 +80,9 @@ void ProblemGenerator(MeshBlock *pmb, ParameterInput *pin) { | |||
| const Real P = y < 0.25 ? P1 : P0; | |||
| const Real vel = y < 0.25 ? v1 : v0; | |||
|
|
|||
| Real gcov[4][4]; | |||
| geom.SpacetimeMetric(0.0, x, y, 0.0, gcov); | |||
There was a problem hiding this comment.
not that it matters much, but why set z to zero here? Why not use the z from the coords?
| @@ -247,7 +247,7 @@ void ProblemGenerator(MeshBlock *pmb, ParameterInput *pin) { | |||
| } | |||
|
|
|||
| Real vsq = 0.; | |||
| SPACELOOP2(ii, jj) { vsq += v(ivlo + ii, k, j, i) * v(ivlo + jj, k, j, i); } | |||
| SPACELOOP(ii) { vsq += v(ivlo + ii, k, j, i) * v(ivlo + ii, k, j, i); } | |||
There was a problem hiding this comment.
is the metric not needed for this one?
|
My comments are non-blockers, so I'm just going to merge. But may be wroth revisiting them at some point. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The name says it all. When I was trying to test tracers with the 2D advection problem, I eventually found that the
vsqcalculation was erroneous, showing the following pattern (psueudocode):Namely, it was either 1) missing the metric (which would collapse the sum in Minkowski or 2) should've been manually collapsed to just
SPACELOOP(ii). The result wasvsq > 1and unphysical Lorentz factors. This PR fixes that by pulling in the metric (opting for the more general solution). I noticed the same pattern in thekhandlinear_modesproblems.khis fixed the same, whilelinear_modesI replaced the loop with a 1DSPACELOOPto not fight with the existing metric for when using e.g., snake coordinates.There's a (somewhat) unrelated refactor in this PR. I was working on adding a Parthenon-style advection regression test #187 but ran into an issue: the reducer objects for e.g.,
particle_resolutionin the Driver class would have their destructors called after MPI/Kokkos finalize and Phoebus would exit with an error code, which caused the regression testing to exit with an error, too. So, this PR also refactors those Reducer objects as mutable params intoradiationandfluid, where appropriate. I also split off the Driver creation and execution into its own scope. Not necessary with the refactor, but safer and potentially more future-proof. In a separate PR I'll pull in the regression testing (see #187 ).