Consistency tweaks: GrainSpeciesCollection and cool_multi_time_g#25
Closed
mabruzzo wants to merge 261 commits into
Closed
Consistency tweaks: GrainSpeciesCollection and cool_multi_time_g#25mabruzzo wants to merge 261 commits into
GrainSpeciesCollection and cool_multi_time_g#25mabruzzo wants to merge 261 commits into
Conversation
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)
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.
…arts of the code.
`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.
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
…cribe/solve_rate_cool_g
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"
…core-lib test-suite
…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
…cribe/solve_rate_cool_g
…ribe/consistency-tweaks
Author
|
superseded by grackle-project#332 |
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
5 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.
To be reviewed after #24 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 #19, to achieve consistency with some of choices made in #241This 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 PRI have manually confirmed that all tests successfully pass
Footnotes
To put it another way, PR Gen2024 transcribe
cool_multi_time_g#19 was my first attempt at transcribing a top-level function. PR transcribe:solve_rate_cool_gto C++ #24 was my second attempt at doing so. I learned a lot while working on transcribe:solve_rate_cool_gto C++ #24 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) ↩