step_rate_newton_raphson cleanup (1/2)#30
Closed
mabruzzo wants to merge 616 commits into
Closed
Conversation
This was referenced Feb 14, 2025
Closed
bumping the versions of grackle and pygrackle to 3.4.0 and 1.1.0, respectively
Bump to next dev version
comoving length (corresponding to H2_self_shielding_length field), so must be multiplied by uxyz not xbase1 to obtain a proper length when computing the shielding column density.
This change was made in anticipation of shifting the "test_grackle_chemistry_field_synched" test from the pygrackle test-suite to the core-lib test suite. In more detail, the test doesn't really belong in the pygrackle test-suite since it is looking at header files. As currently written, the test current places a strong constraint on the way in which we organize our public headers (it currently can't read a header that depends on an autogenerated header file). Shifting this test to the core-lib test suite will remove this limitation
This introduces some more lines of code. I could probably cut down on
some of it (I reused some generic code I had writen for a personal
project, so it's a little more generic than necessary).
This change has 2 major advantages:
1. with the test_chemistry_struct_synched test in the pygrackle
test-suite, it severely limits the organization of the public grackle
headers. When the test was invoked from the pygrackle-framework, it
meant that the header containing the declaration of the
chemistry_data could not have a transative dependence on the
grackle_float.h generated header (because the pygrackle tests can't
make any assumptions about the existence/location of a
build-directory). This has been inconvenient in the past.
2. it opens the door to reuse the machinery to write additional tests
for other structs. For example:
- we could test consistency of the XMACROS with grackle_field_data
- an argument could be made for testing accessibility of members in
the chemistry_data_storage struct (e.g. for the sort of API
introduced within PR grackle-project#316
- there are a number of internal types introduced within the
newchem-cpp transcription PRs that implement a kind of
"manual-reflection." This machinery is generally important for
managing memory and will be useful for porting to GPUs. It would be
extremely useful if we could reuse this testing-machinery to ensure
that the "manual-reflection" includes all fields of the structs.
While it is theoretically possible to test these scenarios through
pytest, it would involve writing a bunch of extra specialized
additional C & Cython code for each case (more than would be
necessary for testing stuff through GTest). Furthermore, testing the
internal types with Cython would mean that Cython needs access to
internal headers (which I don't think we necessarily want).
> [!NOTE]
> A compelling case could be made that we should be invoking the castxml
> tool & python script when compiling the tests. This (i) trades ~300
> lines of C++ for some CMake logic and (ii) it means that the tests
> will be better behaved if the source-code is modified between
> compilation & running the test. But, I only really considered that
> when I was mostly done (and IMO, this change is already a significant
> improvement)
This PR fixes a significant bug in **GrackleConfig.cmake** that causes applications with CMake-build systems that expect to link against free-standing installations of Grackle (i.e. Grackle is pre-built) to break when using the newest versions of CMake (version 4.0 or newer). Background ---------- As part of the installation-process, the CMake build-system installs a file called **GrackleConfig.cmake** alongside the Grackle headers and libraries. This is a special kind CMake script that a build-system searches for and reads when you want to building a downstream application that links against a free-standing Grackle installation. Basically, the script contains queryable metadata about how Grackle was configured and the necessary linker/compilation flags. While we require version 3.16 of CMake or newer to actually build/install the core Grackle library, I followed standard convention and tried to make it possible for the GrackleConfig.cmake file to be used by CMake 3.3 (basically as early a version of CMake as possible). - Essentially, the idea is that the version used to compile/package a version of Grackle may exceed the version of CMake currently installed on somebody's machine. (This isn't a significant consideration for us right now, but it's worth thinking about for the future) - To put it another way, since the GrackleConfig.cmake file is a really simple script, it wasn't hard to use a subset of the language that was compatible with older versions of the lanugage. The Problem ----------- I mistakenly used the `cmake_minimum_version` command within **GrackleConfig.cmake** to ensure that the file wasn't read by a version of CMake older than 3.3. Everything seemed to work fine at first. I only realized this was an issue today. It turns out that with the release of CMake 4.0 (released at the end of March), that calling `cmake_minimum_version` to try enforce a minimum version before version 3.5 now causes CMake to abort with an error. Consequently, people can use CMake 4.0 (or newer) to perform a full standalone installation of Grackle. However, if they then turn around and try to link against that version of Grackle (with that same CMake version), CMake will abort with an error. It's worth noting that doing an embedded install of Grackle (e.g. compiling Grackle as part of the downstream application) using CMake won't cause any problems, but the scenario I highlighted is an extremely plausible way of using CMake. The Solution ------------ Fixing this is easy. We simply delete the call to `cmake_minimum_version` in the **GrackleConfig.cmake** file. For whatever reason I had previously written a separate redundant check enforcing the minimum CMake version that we can leave in place. Ramifications ------------- I really think we should consider making a 3.4.1 release of grackle with this bugfix. This is only going to be a bigger problem as time goes on (and people start using newer versions of CMake)
…cribe/solve_rate_cool_g
Introduce pre-commit with ``ruff`` for formatting new python files
C/C++ Code formatting
…be_cool_multi_time_g [newchem-cpp] initial transcription of `cool_multi_time_g`
This is going to make a lot of merge conflicts
…ribe/consistency-tweaks
…cribe/internal_types_updates
…ess_API [newchem-cpp] rate access api
…e/solve_rate_cool_g [newchem-cpp] transcribe: `solve_rate_cool_g` to C++
…ranscribe/Implement-SpLUT-with-XMacros
…n2024transcribe/SpeciesLUTFieldAdaptor
…e/consistency-tweaks [newchem-cpp] Consistency tweaks: `GrainSpeciesCollection` and `cool_multi_time_g`
…ranscribe/Implement-SpLUT-with-XMacros
…n2024transcribe/SpeciesLUTFieldAdaptor
…ranscribe/initial-step_rate_newton_raphson
…o gen2024transcribe/step_rate_newton_raphson-cleanup-1of2
Author
|
superseded by grackle-project#375 |
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 must be reviewed after #29
After PR #29 provided an initial transcription of
step_rate_newton_raphson, this does a bunch of cleanup. Most of the work here was focused on preparations for transcribinglookup_cool_rates0d.Along the way, I discovered a few issues with the logic introduced by the gen2024 PR:
step_rate_newton_raphsondoesn't make sensible choices when it coevolves internal energy with the species anditer !=1gaussj_groutine takes a bunch of code directly from Numerical Recipes. Due to licensing issues, we cannot distribute this code with GrackleI will return and expand more on this PR in the future
I plan to submit a third PR that finishes the cleanup of
step_rate_newton_raphsonI have manually confirmed that all tests pass in this PR