Gen2024 transcribe cool_multi_time_g#19
Closed
mabruzzo wants to merge 43 commits into
Closed
Conversation
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.
…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.
Closed
This was referenced Jan 26, 2025
Author
This creates problems when we specify the answer-dir as a relative path because we commonly forget to change the directory back to the starting one. This issue will become more prominent in the next commit. - there wasn't any need to change directories in test_initialisation.py - we could remove all logic relating to explicitly changing directories by passing the ``cwd`` kwarg to ``subprocess.run`` in test_get_grackle_version.py and test_models.py (while the directory is still changed by ``subprocess.run``, they always restore the original one afterwards) More importantly, this is a good practice to have!
Previously, the test_code_examples would eagerly FAIL, if pytest was invoked to run answer-tests in "compare-mode" and the answer-file could not be found for the test_code_examples (whether or not we were planning to skip the test_code_examples). This commit relocates the FAILURE check until after the logic determining whether we should skip the test_code_examples.
… that we are building sphinx docs
Fix ReadTheDocs doc CI build
…ctor Answer test refactor
1. A bug is fixed in the Fortran Bindings 2. All references have been removed about the ``GRACKLE_DATA_DIR`` env variable. An upcoming PR (grackle-project#235) is going to overhaul this machinery, but it probably won't be merged the 3.4 release
….4Tweaks Tweaks required for 3.4 release (Fortran Binding bugfix and a few removals)
…test [newchem-cpp] Temporarily Disable OpenMP tests
Move parameter to correct place in fortran interface.
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)
Author
|
superseded by grackle-project#275 |
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 should be reviewed after PR #12-18 are merged (I could probably reorder commits so that #16-#18 are not hard dependencies, but now that I have started transcribing code, I would like to avoid doing that)
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 may be easiest to review this PR commit-by-commit (I performed a sequence of logical transformations to group together local variables)
I have manually confirmed that all tests successfully pass.
NOTE: I have not run the OpenMP tests. I'm pretty confident that the transcribed code is all valid (if nothing else, I think that it is more likely to work than the Fortran version of the code). But, I am pretty sure that the other untranslated Fortran routines will have all kinds of issues (I don't think that Gen was very careful about OpenMP support). We will need to hold off on testing OpenMP until after we transcribe the other top level routines.