Shuffling contents of Grid3D::Update_Hydro_Grid
#352
Merged
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
I think this is a very sensible organizational change. Plus, this is very helpful for some stuff I'm doing on a local branch (it would to upstream this sooner rather than later to limit merge conflicts)
1. consolidate the ``ifdef CHEMISTRY_GPU`` and ``ifdef COOLING_GRACKLE`` regions 2. Adjust Timer calls to match profiling from before last commit. This involves a somewhat ugly hack related to timing Grackle (it seems unavoidable without modifying the timing_functions - I actually have some longer term ideas to refactor some of that code).
|
This all looks good to me. In practice, I wouldn't worry too much about the timing - we can just add another timer for the timestep calculation, which would be more intuitive, but I appreciate you keeping it consistent for now while we are still running scaling tests. |
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.
This PR shifts some of the contents of from
Grid3D::Update_GridintoGrid3D::Update_Hydro_Grid.Overall, I think that this is a useful organizational change. I also need to do something like this on a local branch and it would be convenient to get this merged upstream as soon as we can since this modification seems like it could easily introduce lots of merge-conflicts.
I would recommend that the reviewer review the changes commit-by-commit:
The 1st commit literally moves everything after the Hydro-Integrator execution out of
Grid3D::Update_Gridinto the body ofGrid3D::Update_Hydro_Grid(after the spot where it callsGrid3D::Update_Grid). I think this should be very non-controversial.The 2nd commit renames
Grid3D::Update_GridtoGrid3D::Execute_Hydro_Integrator, which is a more descriptive & self-explanatory name, (I'm obviously happy to name it something else).The 3rd commit moves Slow-Cell-Averaging & timestep-calculation to after the invocation of Grackle. They were already performed after other forms of cooling/chemistry; this is now more consistent and "correct". This is the only structural change to the control flow.
Hydrotiming-region (this includes theHydro_Integratortiming-region). This is addressed in the next commitThe 4th commit includes slow-cell-averaging & timestep-calculation back inside of the
Hydrotiming-region. The timing-regions are now consistent with the current version of dev.ifdefregions down to 2.To summarize: commit 1 is the most important change & commit 3 makes the only change to the control-flow.
If you take issue with any of the changes, I'm happy to update the PR to just include the first commit (or first 2 commits)