Skip to content

transcribe: solve_rate_cool_g to C++#24

Closed
mabruzzo wants to merge 252 commits into
brittonsmith:gen2024from
mabruzzo:gen2024transcribe/solve_rate_cool_g
Closed

transcribe: solve_rate_cool_g to C++#24
mabruzzo wants to merge 252 commits into
brittonsmith:gen2024from
mabruzzo:gen2024transcribe/solve_rate_cool_g

Conversation

@mabruzzo

Copy link
Copy Markdown

This PR needs to be reviewed after PRs #19 - #21 and #23


High-level Summary

This PR transcribes solve_rate_cool_g from Fortran to C++ (the helper functions within solve_rate_cool_g.F were not transcribed). I think the following table is somewhat instructive about the significance of this PR

# of Lines # of args # of local variables
original subroutine: ~1700 469 265
transcribed function: ~400 7 38

Overview

At its core, this PR is a straight-forward one-to-one from transcription from Fortran to C++. This is plainly visible in the first couple of commits. All of the subsequent commits correspond to efforts to essentially "polish" the code

All of this polishing was primarily motivated by a desire to minimize unnecessary future work and this routine's status as the lowest common ancestor of the call-graphs of every non-trivial Fortran subroutine. In more detail:

  • the arguments of every subroutines further down the call stack are composed of a subset of solve_rate_cool_g's arguments and hundreds of local variables
  • a key feature of the transcription tools is that during transcription from Fortran to C++, it can be directed to replace a set of arguments that are read from a single struct with a single argument referring to the struct and modify all references to the original argument appropriately (this can be done for multiple argument-sets)
  • Consequently, I was strongly incentivized to try to aggregate the local variables into structs based on how they are used in other subroutines.1

Polishing the code also made it easier to understand the general control flow (and how the routines are all inter-related), but that's honestly a secondary motivation

Important

To be clear:

  • I took great effort to perfectly preserve all of the logic. I not only confirmed that each commit was atomic and passed every test, but also made sure not to refactor or reorder any of the logic. All mathematical operations, array modifications, etc. should be perfectly preserved. There are a small handful of locations where control-flow constructs (namely if/else and goto) have superficial changes (the control-flow is preserved) that are described down below.2
  • When looking at the original routine, you should easily be able to locate the corresponding logic in the new function
  • All comments have been preserved (or improved!)

More About Polishing:

  • I aggregated local variables into a series of structs. I spent a lot of time figuring out how to do this in a way that will make transcription of future subroutines a lot easier. This took a significant amount of time
  • I put all calls to untranslated fortran subroutines behind wrapper function.
    • This helped make the purpose of certain local variables a lot more clear and helped me make decisions on how to best aggregate local variables
    • the header for this file also explains how it will also help with the transcription of fortran routines that are called in multiple locations
  • I relocated chunks of logic from within the subcycle loop into helper functions.
    • This made the subcycle loop a lot easier to read (and helped me understand how to better aggregate the local variables).
    • they're named: enforce_max_heatcool_subcycle_dt_, select_chem_scheme_update_masks_, calc_Heq_div_dHeqdt_, & set_subcycle_dt_from_chemistry_scheme_
    • these functions all have descriptive docstrings and there are explanatory comments just before each function-call (the logic has not changed -- this was just a relocation)

Important new types:

  • IndexRange:
  • InternalGrUnits:
    • this aggregates all of the unit variables commonly passed around the Fortran routines.
    • Perhaps more importantly, I have copied all of the duplicated logic related to constructing these values directly into a function that will construct this type directly from an instance of code_units. I was EXTREMELY careful to make sure I preserved the original logic and make sure we used the right version of all of the constants (The value of pi differs between the C and Fortran cases and the default precision various constants differ)
    • we can worry about better reconciling this with code_units in the future. For no, this simplifies a lot of transcription
  • SpeciesRateSolverScratchBuf:
    • the main thing to note here is that this struct aggregates local-variable arrays as well as structs of arrays. This is intended for organizational purposes inside of solve_rate_cool_g (i.e. it provides a clear distinction between local variables required for cooling and variables needed for species chemistry)

Places where logic changed/relocated:

  • convert the goto statement into a break statement
  • the handling of ierr has been tweaked slightly. (the actual logic is all the same, but now we directly return the value)
  • some of the unit-calculation logic has been moved to the functions associated with the new InternalGrUnitstype. But the logic has been perfectly preserved.
  • the outer for-loop (over j,k index-pairs) was converted to use the index_helper machinery used in our other existing C routines (the logic is exactly equivalent and this was necessary to use InternalGrUnits)
  • the logic where we use the radiation field to modify the iteration mask has been relocated to the coupled_rt_modify_itmask_ local function. There was a minor tweak: I moved the statement checked by both inner if-statements to the outer if-statement

Assorted Closing Thoughts

The scope of this PR definitely ballooned, but I think it's worth it. All of the commits were logically ordered and have a somewhat descriptive commit message (the ordering got a little more chaotic near the end as I kept finding things that I forgot to address).

If it would make things easier, I could divide this PR into smaller parts. But you would need to be okay with merging very messy looking code into the branch.

Note

PRs for other transcribed routines should all be a lot simpler to review. As noted earlier, there were unique and strong incentives to do a lot of "polishing" in this PR.


As should be clear, I have verified that all the tests pass! (they do so after every commit)

OpenMP should now work. I still can't haven't tested it yet since it's broken in the untranslated top-level subroutines (since Gen didn't keep the lists of private local variables up to date)


Brief Thoughts for the future:

  • If we are willing to embrace a little more C++ I think we could further improve organization
  • Now that I finally have a complete understanding of how all of the logic works (I've never understood been able to keep track of all of it at once before), I have a good idea of how we should pursue adding GPU support. (I think there's a "right way" to do it in a performance portable manner. At some point soon, I plan to make an Issue explaining how to do it)

Footnotes

  1. The groupings of local variables don't have to be perfect, we can always adjust them later. But, the process of aggregating them will never be easier than it is right now

  2. Additionally, there were a couple spots that had large "if-conditions" where I pre-computed parts of the conditions and temporarily stored them within local variables to simply improve legibility. The logic did not change

mabruzzo and others added 30 commits September 23, 2024 16:53
Unlike other cases where we replaced ``logical`` with ``MASK_TYPE``,
I replaced it with ``integer*8`` for the sake of consistency with the
grackle-project#160 PR (I plan to use the transcription of
interpolators_g.F from that PR in the future).
A handful of c functions had a `[-Wreturn-type]` compiler warning where
a function was supposed to return a value but had at least 1 control
flow branch where we didn't return anything.

Also fixed a spot in `c_local_example` where we were passing a
`chemistry_data**` ptr to `local_free_chemistry_data` rather than a
`chemistry_data*` ptr.
To aide with transcription to C/C++ I made the following tweaks to
`calc_grain_size_increment_1d.F`:
- Within `calc_grain_size_increment_1d`, I broke lines similar to
    `SN_i(nSN) =  1; SN_metal(:,nSN) = metal_loc(:,j,k)`
  into 2 separate lines.
  - Prior to this commit, there were essentially 2 statements on each
    of these lines (the statements are delimited by `;`)
  - The transcription routines can't currently handle cases like this
    (it assumes that there is only 1 statement on each line)
  - I made changes like this in 12 locations.
- I inserted `#include "grackle_fortran_types.def"` into
  `solve_cubic_equation` to declare the ``pi_val`` macro inside of the
  subroutine's parameter declarations.
  - This is a convention followed by nearly every subroutine
    declaration in Fortran and it simplifies the process of
    transcription.
  - A standard Fortran compiler does not technically require this change
    since the header is already included within the body of different
    subroutine earlier in the file and the macro definition carries over
    from there.
A handful of c functions had a `[-Wreturn-type]` compiler warning where
a function was supposed to return a value but had at least 1 control
flow branch where we didn't return anything.

Also fixed a spot in `c_local_example` where we were passing a
`chemistry_data**` ptr to `local_free_chemistry_data` rather than a
`chemistry_data*` ptr.
this parallel clause is used when converting from comoving to proper
this parallel clause is used when converting from proper to comoving
Removed unused local variables from `calc_tdust_3d_g` that followed the
naming scheme used in solve_rate_cool for tracking the mass density for
the mass densities of various metal species used in dust chemistry.

There was logic to convert between proper and comoving units for each of
these variables, but they were not actually used for anything (which is
a good thing since they were never initialized).

These variable declarations were probably blindly copied from somewhere
else and were never removed.
Removed unused local variables from `calc_tdust_3d_g` that followed the
naming scheme used in solve_rate_cool for tracking the mass density for
tracking the mass densities of various metal species used in metal
chemistry.

There was logic to convert between proper and comoving units for each of
these variables, but they were not actually used for anything (which is
a good thing since they were never initialized).

These variable declarations were probably blindly copied from somewhere
else and were never removed.
`make dep` previously didn't work correctly with some of the C++ files
(I think because I changed the suffix of the names of some headers)
brittonsmith and others added 27 commits April 9, 2025 17:17
Originally I tried to fix the problem properly, by having
grackle_macros.h directly include grackle.h. But, this produced
problems related to the fact that some function are redeclared slightly
differently between grackle.h and within the source files (the
difference is OK since it is just related to the appearance of `const`).
A number of pending PRs gradually fix this disagreement throughtout the
codebase. Thus, we are going to hold off on "fixing it properly" until
those other PRs are merged
[newchem-cpp] Reverting minor changes to abundance corrections
[newchem-cpp] 2 minor bugfixes following last week's merge
I a prior merge, I accidently duplicated "initialize_rates.lo" in
``src/clib/Make.config.objects``. This caused obviously introduced some
linking problems with the classic build-system
While it looks like there is a large deviation from the older results,
Britton previously did a lot of work investigating this. It turns out
that the example is in the regime where Grackle keeps overshooting the
equlibrium state. We just aren't at the "same point of overshoot"
…d-standard

Register temporary gold standard
When I changed the type of `end_int` argument of ``interpolate_3Dz_g``
to use a 64-bit integer, I forgot to modify the declaration used within
the InterpolationTest.Interpolate3Dz unit-test
@mabruzzo

Copy link
Copy Markdown
Author

superseded by grackle-project#331

@mabruzzo mabruzzo closed this May 17, 2025
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.

5 participants