search

cholla-hydro / cholla

A GPU-based hydro code
https://github.com/cholla-hydro/cholla/wiki
MIT License
60 stars 32 forks source link

Shuffling contents of ``Grid3D::Update_Hydro_Grid`` #352

Closed mabruzzo closed 8 months ago

mabruzzo commented 8 months ago

This PR shifts some of the contents of from Grid3D::Update_Grid into Grid3D::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:

  1. The 1st commit literally moves everything after the Hydro-Integrator execution out of Grid3D::Update_Grid into the body of Grid3D::Update_Hydro_Grid (after the spot where it calls Grid3D::Update_Grid). I think this should be very non-controversial.
  2. The 2nd commit renames Grid3D::Update_Grid to Grid3D::Execute_Hydro_Integrator, which is a more descriptive & self-explanatory name, (I'm obviously happy to name it something else).

  3. 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.

    • A side effect is that slow-cell-averaging & timestep-calculation are no longer part of the Hydro timing-region (this includes the Hydro_Integrator timing-region). This is addressed in the next commit
  4. The 4th commit includes slow-cell-averaging & timestep-calculation back inside of the Hydro timing-region. The timing-regions are now consistent with the current version of dev.
    • I needed a somewhat hacky solution to do this - a more robust solution involves modifying the timing-functionality. I think this may be a little beyond the scope of this PR (I do have some ideas for refactoring this)
    • It also consolidates 4 ifdef regions 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)

evaneschneider commented 8 months ago

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.