[newchem-cpp] Consistency tweaks: GrainSpeciesCollection and cool_multi_time_g#332
Merged
brittonsmith merged 11 commits intoAug 13, 2025
Conversation
This was done to be more consistent with the general-purpose `SpeciesCollection` type. For concreteness: - `SpeciesCollection` has always used the enumerators defined within `SpLUT` as a compile-time lookup table indices - this commit refactors `GrainSpeciesCollection` so that it uses the the enumerators within the newly-defined `OnlyGrainSpLUT` entity as a compile-time lookup table In the future, this change is going to help simplify the C++ versions of `calc_all_tdust_gasgr_1d_g` and `calc_grain_size_increment_1d`. Currently these routines have multiple instances where the same exact logic is repeated 12 times (once per dust grain species). This change should help us remove this code-duplication and replace it with a for-loop.
…the reduced arg lists.
… solve_rate_cool_g
…ribe/consistency-tweaks
This was referenced May 18, 2025
…ribe/consistency-tweaks
mabruzzo
commented
Jun 12, 2025
Comment on lines
+90
to
+92
| for (int i = idx_range.i_start; i < idx_range.i_stop; i++) { | ||
| itmask[i] = MASK_TRUE; | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Here is an example of "cleaned up indexing" (for the discussion in #275)
mabruzzo
commented
Jun 12, 2025
Comment on lines
+110
to
+114
| for (int i = idx_range.i_start; i < idx_range.i_stop; i++) { | ||
| double energy = std::fmax(p2d[i]/(my_chemistry->Gamma-1.), | ||
| tiny_fortran_val); | ||
| cooltime(i,j,k) = (gr_float)(energy/edot[i]); | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Here is another example of "cleaned up indexing" (for the discussion in #275)
brittonsmith
approved these changes
Aug 13, 2025
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
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#25
To be reviewed after #331 is merged (i.e. the PR where
solve_rate_cool_gwas transcribed)This PR essentially goes back and makes some tweaks to the internal
GrainSpeciesCollectionhelper-type and the C++ version of thecool_multi_time_gfunction, both introduced in PR #275, to achieve consistency with some of choices made in #3311This is actually a fairly small PR. The changes include:
GrainSpeciesCollectionto be more consistent with the similar, but more genericSpeciesCollectiontype. For concreteness:SpeciesCollectiontype, I designed it to use enumerators defined within theSpLUTpseudo-namespace as a compile-time lookup table (mapping species names to an index)GrainSpeciesCollectionso that it uses the the enumerators within the newly-definedOnlyGrainSpLUTpseudo-namespace as a lookup-table.2cool_multi_time_gfunction:InternalGrUnitstype instead of manually tracking theutem,urho&uxyzfloating point values and an instance of thecode_unitsstruct (this also removes some boiler-plate fromlocal_calculate_temperature)grackle_index_helperandIndexRange(the latter helps removes some ambiguities about 0-based and 1-based indexing when calling routines that are not yet transcribed)FORTRAN_NAME({subroutine})occurrences with the nicer, C++ wrapper functions. Recall that these wrappers take shorter argument lists, mostly composed of structs, and internally forward the struct-members on to the underlying routine. This is important for the future process of transcribing a routine likecool1d_multi_g, which is called from multiple other transcribed routines.3cool_multi_time_g. The variables holding variable values now only hold the 0-based index values (rather than 1-based index values that we adjust every time that they are used). This was not strictly essential to this PRsolve_rate_cool_g). This was not strictly essential to this PRFootnotes
To put it another way, PR [newchem-cpp] initial transcription of
cool_multi_time_g#275 was my first attempt at transcribing a top-level function. PR [newchem-cpp] transcribe:solve_rate_cool_gto C++ #331 was my second attempt at doing so. I learned a lot while working on [newchem-cpp] transcribe:solve_rate_cool_gto C++ #331 and established some conventions that improved quality and made transcription of certain aspects of the code a lot easier. Now I'm going back and applying this experience to improve the quality of the code written as part of my first transcription attempt. ↩In the future, this change is going to significantly simplify the C++ versions of
calc_all_tdust_gasgr_1d_gandcalc_grain_size_increment_1d. Currently these routines have multiple instances where the same exact logic is repeated 12 times (once per dust grain species). This change should help us replace instances of this code-duplication with for-loops. ↩Essentially, we want there to be just 1 location in the entire codebase where we directly reference any given Fortran routine before we transcribe that routine. (there are a couple of reasons for this that I could go into if you want) ↩