[newchem-cpp] initial transcription of cool_multi_time_g#275
Merged
mabruzzo merged 34 commits intoJun 12, 2025
Merged
Conversation
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.
…for transcribing)
`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)
…gHeatingScratchBuf struct.
…nInterpScratchBuf
This is the last commit in a sequence where we were refactoring the local variable declarations. The routine should now work properly with OpenMP, but we can't test that since OpenMP-support was broken in earlier commits to the gen2024 branch (before we began the transcription). More simplifications can certainly be made, but it is worth holding off on that until after we have transcribed more of Grackle.
Previously, I had manually specified the name of the C++ runtime library and had libtool directly invoke the system's linker. However, the default C++ runtime library (assumed during compilation) varies across machine and architecture and it would involve specifying the the library in **every** configuration Makefile. This strategy is better since the C++ compiler knows exactly what it needs. It is also considered a general "best practice" (apparently, it can be important on some architectures for more that just knowing the proper C++ runtime library)
mabruzzo
changed the title
cool_multi_time_g
This was referenced Mar 20, 2025
mabruzzo
changed the title
cool_multi_time_gcool_multi_time_g
mabruzzo
moved this from In Progress
to Awaiting Review
in New Chemistry and C++ Transcription
mabruzzo
added
the
refactor
mabruzzo
force-pushed
the
gen2024-transcribe_cool_multi_time_g
branch
from
02c22b3 to
e54c32f
Compare
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
This was referenced May 17, 2025
brittonsmith
approved these changes
Jun 12, 2025
Contributor
There was a problem hiding this comment.
Quite simple and happy to see this go in. I like the simplification of the new data structures. I had the comments about how we're doing the looping and array indexing, but they are minor. One other general comment. I think we can eventually (if not now) move away from the abbreviated fortran-style file naming convention that gives us cool_multi_time to something a bit more expressive.
Edit: I forgot to say that I haven't reviewed the status reporting PR (#269), so maybe this acceptance is provisional on my approving that.
Collaborator
Author
|
I simply cherry-picked the most recent commit that I added to #269 |
github-project-automation
Bot
moved this from Awaiting Review
to Done
in New Chemistry and C++ Transcription
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 was originally proposed as brittonsmith#19
This PR includes changes from #274 (it should be merged first)
This PR converts
cool_multi_time_gfrom Fortran to C++. Along the way, I have refactored a bunch of local variables into new structs (these may not be perfectly organized right now, but they are probably good enough to move on from right now). It also introduces a lot of machinery for needed for supporting C++ files in Grackle (some of these can be reconsidered in the future).It may be easiest to review this PR commit-by-commit (I performed a sequence of logical transformations to group together local variables).
OpenMP should work properly in the transcribed version of this routine (it is broken in the version of
cool_multi_time_g.Fcurrently in the newchem-cpp branch. With that said, I have not tested it because the OpenMP support is broken in other parts of the newchem-cpp branch -- it will be hard to do that until after we merge in all of the other transcribed routines (in each case, I took steps to try to fix it).While transferring the PR, I added a commit that adopts a more general solution for properly linking the C++ runtime library when using the classic build-system.
Be aware, some future PRs (they have not been ported to this repository yet), brittonsmith#24 and brittonsmith#25, will make some additional modifications to
cool_multi_time_g.