From e4df3f914b2dc47af7ab79e7c6530d3d0a2f9e1c Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 19 Dec 2025 08:22:14 -0500 Subject: [PATCH 01/40] address compiler warning in chemistry_solver_funcs.hppp --- src/clib/chemistry_solver_funcs.hpp | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/clib/chemistry_solver_funcs.hpp b/src/clib/chemistry_solver_funcs.hpp index f36db0a7f..ad3f8a760 100644 --- a/src/clib/chemistry_solver_funcs.hpp +++ b/src/clib/chemistry_solver_funcs.hpp @@ -1,6 +1,11 @@ -// See LICENSE file for license and copyright information - -/// @file chemistry_solver_funcs.hpp +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file /// @brief Defines chemistry reaction related functions invoked by the /// grackle solver in order to integrate the species densities over time. /// @@ -14,6 +19,8 @@ /// be decoupled from the derivative calculation for primoridial species /// - it may also make sense to further divide logic by the kinds of species /// that are affected (e.g. primordial vs grains) +/// +//===----------------------------------------------------------------------===// #ifndef CHEMISTRY_SOLVER_FUNCS_HPP #define CHEMISTRY_SOLVER_FUNCS_HPP @@ -579,7 +586,9 @@ inline void species_density_updates_gauss_seidel( if ( (my_chemistry->metal_chemistry == 1) && (itmask_metal[i] != MASK_FALSE) ) { - scoef = scoef; + // we comment out the following line that assigns scoef to itself since + // it has no practical impact and produces a compiler warning + // scoef = scoef; acoef = acoef + kcol_buf.data[CollisionalRxnLUT::kz44][i] * CII(i,j,k) / 12. + kcol_buf.data[CollisionalRxnLUT::kz45][i] * OII(i,j,k) / 16. @@ -1932,7 +1941,9 @@ inline void species_density_derivatives_0d( if ((my_chemistry->metal_chemistry == 1) && (itmask_metal[0] != MASK_FALSE)) { - scoef = scoef; + // we comment out the following line that assigns scoef to itself since + // it has no practical impact and produces a compiler warning + // scoef = scoef; acoef = acoef + kcr_buf.data[CollisionalRxnLUT::kz44][0] * CII / 12. + kcr_buf.data[CollisionalRxnLUT::kz45][0] * OII / 16. From e2ea0d925f6ec099cb9348fbb9675eeb5cc1f263 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 19 Dec 2025 08:23:58 -0500 Subject: [PATCH 02/40] address warning about uninitialized variables in solve_rate_cool_g-cpp.cpp --- src/clib/solve_rate_cool_g-cpp.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clib/solve_rate_cool_g-cpp.cpp b/src/clib/solve_rate_cool_g-cpp.cpp index f96243f58..bde08114e 100644 --- a/src/clib/solve_rate_cool_g-cpp.cpp +++ b/src/clib/solve_rate_cool_g-cpp.cpp @@ -791,7 +791,7 @@ int solve_rate_cool_g( // declare 2 variables (primarily used for subcycling, but also used in // error reporting) int iter; - double ttmin; + double ttmin = huge8; // ------------------ Loop over subcycles ---------------- From 9ff4b3ad7d13286bdb3141560969044dae6ac145 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 18 Dec 2025 11:00:53 -0500 Subject: [PATCH 03/40] mv testing utility library from tests/unit to tests/grtestutils --- .clang-format-ignore | 14 +-- tests/CMakeLists.txt | 1 + tests/grtestutils/CMakeLists.txt | 108 ++++++++++++++++++ tests/grtestutils/README.md | 3 + .../grtest_cmd.cpp => grtestutils/cmd.cpp} | 2 +- .../grtest_cmd.hpp => grtestutils/cmd.hpp} | 0 .../googletest/check_allclose.hpp} | 0 .../grtest_os.cpp => grtestutils/os.cpp} | 2 +- .../grtest_os.hpp => grtestutils/os.hpp} | 0 .../utils.cpp} | 2 +- .../utils.hpp} | 0 tests/unit/CMakeLists.txt | 60 +--------- tests/unit/test_chemistry_struct_synced.cpp | 2 +- tests/unit/test_ghost_zone.cpp | 2 +- tests/unit/test_linalg.cpp | 2 +- tests/unit/test_status_reporting.cpp | 2 +- 16 files changed, 132 insertions(+), 68 deletions(-) create mode 100644 tests/grtestutils/CMakeLists.txt create mode 100644 tests/grtestutils/README.md rename tests/{unit/grtest_cmd.cpp => grtestutils/cmd.cpp} (98%) rename tests/{unit/grtest_cmd.hpp => grtestutils/cmd.hpp} (100%) rename tests/{unit/utest_helpers.hpp => grtestutils/googletest/check_allclose.hpp} (100%) rename tests/{unit/grtest_os.cpp => grtestutils/os.cpp} (99%) rename tests/{unit/grtest_os.hpp => grtestutils/os.hpp} (100%) rename tests/{unit/grtest_utils.cpp => grtestutils/utils.cpp} (98%) rename tests/{unit/grtest_utils.hpp => grtestutils/utils.hpp} (100%) diff --git a/.clang-format-ignore b/.clang-format-ignore index f6fca214f..0072faee9 100644 --- a/.clang-format-ignore +++ b/.clang-format-ignore @@ -64,17 +64,17 @@ src/include/grackle_float.h.in src/include/grackle_misc.h src/include/grackle_rate_functions.h src/include/grackle_types.h -tests/unit/grtest_cmd.cpp -tests/unit/grtest_cmd.hpp -tests/unit/grtest_os.cpp -tests/unit/grtest_os.hpp -tests/unit/grtest_utils.cpp -tests/unit/grtest_utils.hpp +tests/grtestutils/cmd.cpp +tests/grtestutils/cmd.hpp +tests/grtestutils/os.cpp +tests/grtestutils/os.hpp +tests/grtestutils/utils.cpp +tests/grtestutils/utils.hpp +tests/grtestutils/googletest/check_allclose.hpp tests/unit/test_chemistry_struct_synced.cpp tests/unit/test_ghost_zone.cpp tests/unit/test_interpolators_comparisons.cpp tests/unit/test_linalg.cpp tests/unit/test_status_reporting.cpp tests/unit/test_unit_interpolators_g.cpp -tests/unit/utest_helpers.hpp diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 0af865f10..2320c1c58 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -1,3 +1,4 @@ +add_subdirectory(grtestutils) add_subdirectory(unit) # down below, we add tests for various code-examples diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt new file mode 100644 index 000000000..cd21e6065 --- /dev/null +++ b/tests/grtestutils/CMakeLists.txt @@ -0,0 +1,108 @@ +# Purpose: Defines 2 "libraries" to aid with the testing of Grackle +# ================================================================= + +# 1. testdeps: bundles dependencies used by all tests +# =================================================== +# - this is an interface library (i.e. nothing is compiled) that ONLY exists +# to aggregate dependencies, include directories and compile definitions +# - in other words, a basic unit test (that doesn't require logic from +# grtestutils can list this as its sole dependency) + +add_library(testdeps INTERFACE) +target_link_libraries(testdeps INTERFACE Grackle::Grackle GTest::gtest_main) + +# compiler defs are short-term hacks to help tests invoke internal functions +# from Grackle (both of these will become unnecessary in the near future) +target_compile_definitions(testdeps + # currently needed to invoke the fortran function wrappers from within clib + INTERFACE "$<$:LINUX>" + # suppresses warnings about C internal headers using our deprecated public + # headers (the files should include grackle.h instead). We're currently + # holding off on properly fixing this to minimize conflicts with pending PRs + # on the newchem-cpp branch + INTERFACE GRIMPL_PUBLIC_INCLUDE=1 +) + +# needed to let tests call internal functions from src/clib +target_include_directories(testdeps INTERFACE ${PROJECT_SOURCE_DIR}/src/clib) +target_compile_features(testdeps INTERFACE cxx_std_17) + + +# 2. grtestutils: testing utility library +# ======================================= +# - this is an internal library that defines reusable testing utilities +# - Frankly, I don't love the name. Originally I called it grtest, but I'm a +# a little concerned that may look a little too much like gtest (i.e. the +# shorthand that googletest uses. + +# Code Organization +# ----------------- +# This code in this directory is organized in a slightly peculiar manner +# - code in the main directory should not depend upon googletest +# - the googletest subdirectory is intended to define custom googletest +# assertions and fixtures. This subdirectory should **ONLY** include +# headers (so that, at most, this internal library has a transitive +# dependence on googletest) +# +# RATIONALE: +# - over time, the idea has been floated to introducing C++ logic to +# accelerate certain calculations provided by the python wrapper: +# - PR #343 proposed creating a C++ accelerated harness for performing +# certain kinds of semi-analytic calculations (like freefall or evolving +# constant density) +# - PR #370 proposed introducing a function to infer the internal energy at +# a desired temperature (yes, the PR proposes it as a python function, but +# it would be faster as a C++ function). +# - Relatedly, it would also be useful to be able to directly compute +# temperature or chemical equilibrium +# - in each case this functionality would also be extremely useful for the +# googletest framework or for writing simple C++-only benchmarking programs +# - this is extremely useful if you want to use tools like valgrind or +# compiler sanitizers. While it's generally possible to use these tools in +# a python extension module, it may require compiling CPython itself from +# source. Moreover it can be extremely slow (these tools introduce to +# basic C/C++ operations and that overhead will be apply to the Python +# interpretter) +# - this is important if we want to test grackle when compiled with various +# backends. Historically, the python bindings have never been able to use +# Grackle with the OpenMP backend. While we can and will address this, +# there will additional challenges as we introduce GPU acceleration +# - this library is intended as a location where that kind of hypothetical +# functionality can be introduced, and organization strategy ensures that +# it will be straightforward to provide this hypothetical functionality to +# the python bindings without it depending upon googletest (i.e. we may +# treat the googletest subdirectory as a distinct interface library) + +add_library(grtestutils + cmd.hpp cmd.cpp + utils.hpp utils.cpp + os.hpp os.cpp + + # REMINDER: the googletest subdirectory should only contain headers + googletest/check_allclose.hpp +) + +# we are being a little lazy with our usage of testdeps right here +target_link_libraries(grtestutils PUBLIC testdeps) +target_compile_features(grtestutils PUBLIC cxx_std_17) + +# configure the grtestutils target so that when a file outside of this +# directory includes a header from within this directory, the include +# directive looks something like: +# #include "grtestutils/.hpp" +# OR +# #include "grtestutils/googletest/.hpp +target_include_directories(grtestutils INTERFACE ${PROJECT_SOURCE_DIR}/tests) + +# these compile-definitions act as short-term hacks +target_compile_definitions(grtestutils + # this hack helps us get the path to the directory holding data files (we can + # remove it once we introduce automatic file management in PR 235, PR 237, + # and PR 246) + PRIVATE GR_DATADIR=${CMAKE_CURRENT_SOURCE_DIR}/../../grackle_data_files/input/ + + # this hack lets us use Operating-system specific functionality (Once PR #237 + # is merged, we should make use of the machinery introduced by that PR for + # enabling/disabling os-specific features) + PRIVATE "$<$:PLATFORM_GENERIC_UNIX>" +) diff --git a/tests/grtestutils/README.md b/tests/grtestutils/README.md new file mode 100644 index 000000000..d7d59171f --- /dev/null +++ b/tests/grtestutils/README.md @@ -0,0 +1,3 @@ +Defines a simple testing utility library. + +See the CMakeLists.txt for a discussion of code organization. diff --git a/tests/unit/grtest_cmd.cpp b/tests/grtestutils/cmd.cpp similarity index 98% rename from tests/unit/grtest_cmd.cpp rename to tests/grtestutils/cmd.cpp index 131fa158b..700cafa01 100644 --- a/tests/unit/grtest_cmd.cpp +++ b/tests/grtestutils/cmd.cpp @@ -1,4 +1,4 @@ -#include "grtest_cmd.hpp" +#include "cmd.hpp" #include // FILE, fgets, fprintf, stderr, (popen/pclose on POSIX) diff --git a/tests/unit/grtest_cmd.hpp b/tests/grtestutils/cmd.hpp similarity index 100% rename from tests/unit/grtest_cmd.hpp rename to tests/grtestutils/cmd.hpp diff --git a/tests/unit/utest_helpers.hpp b/tests/grtestutils/googletest/check_allclose.hpp similarity index 100% rename from tests/unit/utest_helpers.hpp rename to tests/grtestutils/googletest/check_allclose.hpp diff --git a/tests/unit/grtest_os.cpp b/tests/grtestutils/os.cpp similarity index 99% rename from tests/unit/grtest_os.cpp rename to tests/grtestutils/os.cpp index eba65b404..bbcd4538e 100644 --- a/tests/unit/grtest_os.cpp +++ b/tests/grtestutils/os.cpp @@ -1,4 +1,4 @@ -#include "grtest_os.hpp" +#include "os.hpp" // the following 2 headers are the c versions of the headers (rather than the // C++ versions) since it seems more likely that posix-specific functions are diff --git a/tests/unit/grtest_os.hpp b/tests/grtestutils/os.hpp similarity index 100% rename from tests/unit/grtest_os.hpp rename to tests/grtestutils/os.hpp diff --git a/tests/unit/grtest_utils.cpp b/tests/grtestutils/utils.cpp similarity index 98% rename from tests/unit/grtest_utils.cpp rename to tests/grtestutils/utils.cpp index 85557d7af..d4781249f 100644 --- a/tests/unit/grtest_utils.cpp +++ b/tests/grtestutils/utils.cpp @@ -1,4 +1,4 @@ -#include "grtest_utils.hpp" +#include "utils.hpp" #include diff --git a/tests/unit/grtest_utils.hpp b/tests/grtestutils/utils.hpp similarity index 100% rename from tests/unit/grtest_utils.hpp rename to tests/grtestutils/utils.hpp diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index f1018e3ca..6bac6b918 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -1,60 +1,12 @@ -# declare testdeps to bundle together dependencies used by all tests -# ------------------------------------------------------------------ -add_library(testdeps INTERFACE) -target_link_libraries(testdeps INTERFACE Grackle::Grackle GTest::gtest_main) +# NOTE: the grtestutils and testdeps library targets are both defined within +# ../grtestutils/CMakeLists.txt -# compiler defs are short-term hacks to help tests invoke internal functions -# from Grackle (both of these will become unnecessary in the near future) -target_compile_definitions(testdeps - # needed to invoke Fortran functions from C - INTERFACE "$<$:LINUX>" - # suppresses warnings about C internal headers using our deprecated public - # headers (the files should include grackle.h instead). We're currently - # holding off on properly fixing this to minimize conflicts with pending PRs - # on the newchem-cpp branch - INTERFACE GRIMPL_PUBLIC_INCLUDE=1 -) - -# needed to let tests call internal functions from C -target_include_directories(testdeps INTERFACE ${PROJECT_SOURCE_DIR}/src/clib) -target_compile_features(testdeps INTERFACE cxx_std_17) - -# declare the grtest utility library -# ---------------------------------- -# -> this is an internal library that defines reusable testing utilities -# -> if we add more files to this library, we should consider relocating the -# library to a different directory - -add_library(grtest_utils - grtest_cmd.hpp grtest_cmd.cpp - grtest_utils.hpp grtest_utils.cpp - grtest_os.hpp grtest_os.cpp -) -# we are being a little lazy with our usage of testdeps right here -target_link_libraries(grtest_utils PUBLIC testdeps) -target_compile_features(grtest_utils PUBLIC cxx_std_17) - -# these compile-definitions act as short-term hacks -target_compile_definitions(grtest_utils - # this hack helps us get path input-file directory (we can remove it once we - # introduce automatic file management in PR 235, PR 237, and PR 246) - PRIVATE GR_DATADIR=${CMAKE_CURRENT_SOURCE_DIR}/../../grackle_data_files/input/ - - # this hack lets us use Operating-system specific functionality (Once PR #237 - # is merged, we should make use of the machinery introduced by that PR for - # enabling/disabling os-specific features) - PRIVATE "$<$:PLATFORM_GENERIC_UNIX>" -) - -# start declaring targets for tests -# --------------------------------- add_executable(runInterpolationTests test_unit_interpolators_g.cpp) target_link_libraries(runInterpolationTests testdeps) - gtest_discover_tests(runInterpolationTests) add_executable(runLinAlgTests test_linalg.cpp) -target_link_libraries(runLinAlgTests testdeps) +target_link_libraries(runLinAlgTests grtestutils) gtest_discover_tests(runLinAlgTests) add_executable(runGrainSpeciesIntoTests test_grain_species_info.cpp) @@ -62,7 +14,7 @@ target_link_libraries(runGrainSpeciesIntoTests testdeps) gtest_discover_tests(runGrainSpeciesIntoTests) add_executable(runStatusReporting test_status_reporting.cpp) -target_link_libraries(runStatusReporting grtest_utils) +target_link_libraries(runStatusReporting grtestutils) gtest_discover_tests(runStatusReporting) # one might argue that the following is more of an integration or end-to-end @@ -74,7 +26,7 @@ gtest_discover_tests(runVisitorTests) # one might argue that the following is more of an integration or end-to-end # test than a unit-test add_executable(runGhostZoneTests test_ghost_zone.cpp) -target_link_libraries(runGhostZoneTests grtest_utils testdeps) +target_link_libraries(runGhostZoneTests grtestutils testdeps) gtest_discover_tests(runGhostZoneTests) # this target tests that the members of the chemistry_data struct can be @@ -84,7 +36,7 @@ gtest_discover_tests(runGhostZoneTests) # problems for "death-tests", so these test-cases should remain separate from # the rest of the gtest framework add_executable(runSyncedChemistryData test_chemistry_struct_synced.cpp) -target_link_libraries(runSyncedChemistryData grtest_utils testdeps) +target_link_libraries(runSyncedChemistryData grtestutils testdeps) target_compile_definitions(runSyncedChemistryData PRIVATE READER_PATH=${PROJECT_SOURCE_DIR}/tests/scripts/castxml_output_reader.py diff --git a/tests/unit/test_chemistry_struct_synced.cpp b/tests/unit/test_chemistry_struct_synced.cpp index 6a3bd9642..ee02a02bb 100644 --- a/tests/unit/test_chemistry_struct_synced.cpp +++ b/tests/unit/test_chemistry_struct_synced.cpp @@ -1,4 +1,4 @@ -#include "grtest_cmd.hpp" +#include "grtestutils/cmd.hpp" #include diff --git a/tests/unit/test_ghost_zone.cpp b/tests/unit/test_ghost_zone.cpp index b009fbe7b..607da2325 100644 --- a/tests/unit/test_ghost_zone.cpp +++ b/tests/unit/test_ghost_zone.cpp @@ -24,7 +24,7 @@ #include #include -#include "grtest_utils.hpp" +#include "grtestutils/utils.hpp" #include diff --git a/tests/unit/test_linalg.cpp b/tests/unit/test_linalg.cpp index 943b83d75..d3fb7d080 100644 --- a/tests/unit/test_linalg.cpp +++ b/tests/unit/test_linalg.cpp @@ -2,7 +2,7 @@ #include #include "fortran_func_wrappers.hpp" -#include "utest_helpers.hpp" +#include "grtestutils/googletest/check_allclose.hpp" /// Records the paramters for a linear algebra test-case diff --git a/tests/unit/test_status_reporting.cpp b/tests/unit/test_status_reporting.cpp index 1922fef2a..91ce3791c 100644 --- a/tests/unit/test_status_reporting.cpp +++ b/tests/unit/test_status_reporting.cpp @@ -12,7 +12,7 @@ #include "grackle.h" // GR_FAIL #include "status_reporting.h" -#include "grtest_os.hpp" +#include "grtestutils/os.hpp" testing::AssertionResult ContainsFormattedMessage_(int n) { From b5b7e65e1461923b944031514e2efa3b299f4536 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 18 Dec 2025 12:47:21 -0500 Subject: [PATCH 04/40] slightly alter we define targets that support testing --- tests/grtestutils/CMakeLists.txt | 83 +++++++++++++++++++++----------- tests/unit/CMakeLists.txt | 11 ++--- 2 files changed, 61 insertions(+), 33 deletions(-) diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index cd21e6065..42675d148 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -1,20 +1,16 @@ -# Purpose: Defines 2 "libraries" to aid with the testing of Grackle -# ================================================================= +# Purpose: define "libraries" to aid with the testing of Grackle +# ============================================================== -# 1. testdeps: bundles dependencies used by all tests -# =================================================== -# - this is an interface library (i.e. nothing is compiled) that ONLY exists -# to aggregate dependencies, include directories and compile definitions -# - in other words, a basic unit test (that doesn't require logic from -# grtestutils can list this as its sole dependency) +# first, define an interface "library" (i.e. nothing is compiled) for +# book-keeping purposes. dependents get "linked" against this library in order +# to be able to access grackle's internal functionality -add_library(testdeps INTERFACE) -target_link_libraries(testdeps INTERFACE Grackle::Grackle GTest::gtest_main) +add_library(_grackle_internals INTERFACE) +target_link_libraries(_grackle_internals INTERFACE Grackle::Grackle) -# compiler defs are short-term hacks to help tests invoke internal functions -# from Grackle (both of these will become unnecessary in the near future) -target_compile_definitions(testdeps - # currently needed to invoke the fortran function wrappers from within clib +# short-term macro definitions (both will become unnecessary in the near future) +target_compile_definitions(_grackle_internals + # currently required for functions declared by fortran_func_wrappers.hpp INTERFACE "$<$:LINUX>" # suppresses warnings about C internal headers using our deprecated public # headers (the files should include grackle.h instead). We're currently @@ -23,17 +19,24 @@ target_compile_definitions(testdeps INTERFACE GRIMPL_PUBLIC_INCLUDE=1 ) -# needed to let tests call internal functions from src/clib -target_include_directories(testdeps INTERFACE ${PROJECT_SOURCE_DIR}/src/clib) -target_compile_features(testdeps INTERFACE cxx_std_17) - +# this makes it possible to write an include-directive of a header from +# src/clib, by writing something like +# #include "{name}.hpp" +# where {name} is replaced by the name of the header +# -> frankly, I don't love this because it's not immediately clear that we are +# including a private header. The alternative is to require the paths +# in the include directives to include the name of the directory holding +# the implementation files (e.g. "clib/{header}.hpp") +target_include_directories(_grackle_internals + INTERFACE ${PROJECT_SOURCE_DIR}/src/clib +) -# 2. grtestutils: testing utility library -# ======================================= +# grtestutils: testing utility library +# ==================================== # - this is an internal library that defines reusable testing utilities # - Frankly, I don't love the name. Originally I called it grtest, but I'm a # a little concerned that may look a little too much like gtest (i.e. the -# shorthand that googletest uses. +# shorthand that googletest uses). # Code Organization # ----------------- @@ -68,22 +71,32 @@ target_compile_features(testdeps INTERFACE cxx_std_17) # Grackle with the OpenMP backend. While we can and will address this, # there will additional challenges as we introduce GPU acceleration # - this library is intended as a location where that kind of hypothetical -# functionality can be introduced, and organization strategy ensures that +# functionality can be introduced. The organization strategy ensures that # it will be straightforward to provide this hypothetical functionality to -# the python bindings without it depending upon googletest (i.e. we may -# treat the googletest subdirectory as a distinct interface library) +# the python bindings (and any C++ testing code) without making the python +# bindings link against googletest. add_library(grtestutils + # files outside of the googletest subdirectory + # -> these shouldn't include headers from the googletest subdirectory cmd.hpp cmd.cpp utils.hpp utils.cpp os.hpp os.cpp - # REMINDER: the googletest subdirectory should only contain headers + # files in the googletest subdirectory (reminder: should just be headers!) googletest/check_allclose.hpp ) -# we are being a little lazy with our usage of testdeps right here -target_link_libraries(grtestutils PUBLIC testdeps) +target_link_libraries(grtestutils + PRIVATE _grackle_internals + PUBLIC Grackle::Grackle + # to express the transitive dependency on googletest (it's transitive since + # its only referenced by header files), we should theoretically write + # INTERFACE GTest::GTest + # we choose not to do this since all test code pulls in this dependency by + # explicitly listing `testdeps` as a dependency (defined below) +) + target_compile_features(grtestutils PUBLIC cxx_std_17) # configure the grtestutils target so that when a file outside of this @@ -106,3 +119,19 @@ target_compile_definitions(grtestutils # enabling/disabling os-specific features) PRIVATE "$<$:PLATFORM_GENERIC_UNIX>" ) + + +# 2. testdeps: bundles dependencies used by tests +# =============================================== +# - this is an interface library (i.e. nothing is compiled) that ONLY exists +# to aggregate dependencies, include directories and compile definitions +# - all tests should depend on this target + +add_library(testdeps INTERFACE) +target_link_libraries(testdeps + INTERFACE Grackle::Grackle grtestutils _grackle_internals GTest::gtest_main +) +# indicates that all consumers are written in C++17 or newer +target_compile_features(testdeps INTERFACE cxx_std_17) + + diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 6bac6b918..fd5a75ac1 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -1,12 +1,11 @@ -# NOTE: the grtestutils and testdeps library targets are both defined within -# ../grtestutils/CMakeLists.txt +# NOTE: the testdeps target is defined within ../grtestutils/CMakeLists.txt add_executable(runInterpolationTests test_unit_interpolators_g.cpp) target_link_libraries(runInterpolationTests testdeps) gtest_discover_tests(runInterpolationTests) add_executable(runLinAlgTests test_linalg.cpp) -target_link_libraries(runLinAlgTests grtestutils) +target_link_libraries(runLinAlgTests testdeps) gtest_discover_tests(runLinAlgTests) add_executable(runGrainSpeciesIntoTests test_grain_species_info.cpp) @@ -14,7 +13,7 @@ target_link_libraries(runGrainSpeciesIntoTests testdeps) gtest_discover_tests(runGrainSpeciesIntoTests) add_executable(runStatusReporting test_status_reporting.cpp) -target_link_libraries(runStatusReporting grtestutils) +target_link_libraries(runStatusReporting testdeps) gtest_discover_tests(runStatusReporting) # one might argue that the following is more of an integration or end-to-end @@ -26,7 +25,7 @@ gtest_discover_tests(runVisitorTests) # one might argue that the following is more of an integration or end-to-end # test than a unit-test add_executable(runGhostZoneTests test_ghost_zone.cpp) -target_link_libraries(runGhostZoneTests grtestutils testdeps) +target_link_libraries(runGhostZoneTests testdeps) gtest_discover_tests(runGhostZoneTests) # this target tests that the members of the chemistry_data struct can be @@ -36,7 +35,7 @@ gtest_discover_tests(runGhostZoneTests) # problems for "death-tests", so these test-cases should remain separate from # the rest of the gtest framework add_executable(runSyncedChemistryData test_chemistry_struct_synced.cpp) -target_link_libraries(runSyncedChemistryData grtestutils testdeps) +target_link_libraries(runSyncedChemistryData testdeps) target_compile_definitions(runSyncedChemistryData PRIVATE READER_PATH=${PROJECT_SOURCE_DIR}/tests/scripts/castxml_output_reader.py From 8cae77700db4cda8599c9563237252ddf1fd588f Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 18 Dec 2025 14:37:13 -0500 Subject: [PATCH 05/40] address a todo item (to reduce code duplication) --- tests/grtestutils/cmd.cpp | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/tests/grtestutils/cmd.cpp b/tests/grtestutils/cmd.cpp index 700cafa01..36128da0b 100644 --- a/tests/grtestutils/cmd.cpp +++ b/tests/grtestutils/cmd.cpp @@ -1,18 +1,13 @@ #include "cmd.hpp" +// internal Grackle routine: +#include "status_reporting.h" // GR_INTERNAL_ERROR, GR_INTERNAL_REQUIRE + #include // FILE, fgets, fprintf, stderr, (popen/pclose on POSIX) -#include // std::exit #include // std::stringstream #include -// TODO: replace with Grackle's existing err machinery -[[noreturn]] static void err_(std::string msg="") { - const char* ptr = (msg.empty()) ? "" : msg.c_str(); - fprintf(stderr, "ERROR: %s\n", ptr); - std::exit(1); -} - #ifdef PLATFORM_GENERIC_UNIX #define TEMP_BUF_SIZE 128 @@ -29,14 +24,14 @@ grtest::ProcessStatusAndStdout grtest::capture_status_and_output( // fp represents a buffered pipe to the standard output of the command. FILE* fp = popen(command.data(), "r"); - if (fp == nullptr) { err_("there was a problem launching the command"); } + GR_INTERNAL_REQUIRE(fp != nullptr, "there's a problem launching the command"); // if our reads from the pipe outpace the rate at which the command writes to // the pipe, fgets won't return until more data becomes available. If the // processess ends, fgets encounters EOF (causing fgets to return) while (fgets(temp_buf, TEMP_BUF_SIZE, fp) != nullptr) { buf << temp_buf; } int status = pclose(fp); - if (status == -1) { err_("there was a problem closing the command"); } + GR_INTERNAL_REQUIRE(status != -1, "there was a problem closing the command"); return {status, buf.str()}; } @@ -45,7 +40,7 @@ grtest::ProcessStatusAndStdout grtest::capture_status_and_output( grtest::ProcessStatusAndStdout grtest::capture_status_and_output( const std::string& command ) { - err_("not implemented on this platform"); + GR_INTERNAL_ERROR("not implemented on this platform"); } #endif /* PLATFORM_GENERIC_UNIX */ From 5cf43bca95b2e26b28d640f692b65098cd91ec41 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 19 Dec 2025 11:11:07 -0500 Subject: [PATCH 06/40] Add gmock to the list of libraries linked via testdeps For context, the gmock library is a component of googletest (it's already being installed). All this commit does is make it possible for us to use it in subsequent PRs --- tests/grtestutils/CMakeLists.txt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index 42675d148..e58b7322b 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -129,7 +129,8 @@ target_compile_definitions(grtestutils add_library(testdeps INTERFACE) target_link_libraries(testdeps - INTERFACE Grackle::Grackle grtestutils _grackle_internals GTest::gtest_main + INTERFACE Grackle::Grackle grtestutils _grackle_internals + GTest::gtest_main GTest::gmock ) # indicates that all consumers are written in C++17 or newer target_compile_features(testdeps INTERFACE cxx_std_17) From deb6c84a93c7537a3c4fb9a39beb19dc96a996b6 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Tue, 16 Dec 2025 16:33:32 -0500 Subject: [PATCH 07/40] mv GrackleCtxPack out of test_ghost_zone and refactor --- src/clib/status_reporting.h | 16 +++ tests/grtestutils/CMakeLists.txt | 4 +- tests/grtestutils/googletest/fixtures.hpp | 110 +++++++++++++++ tests/grtestutils/preset.cpp | 107 +++++++++++++++ tests/grtestutils/preset.hpp | 66 +++++++++ tests/unit/test_ghost_zone.cpp | 157 +++++++--------------- 6 files changed, 351 insertions(+), 109 deletions(-) create mode 100644 tests/grtestutils/googletest/fixtures.hpp create mode 100644 tests/grtestutils/preset.cpp create mode 100644 tests/grtestutils/preset.hpp diff --git a/src/clib/status_reporting.h b/src/clib/status_reporting.h index f52f24651..fe1a1cd5b 100644 --- a/src/clib/status_reporting.h +++ b/src/clib/status_reporting.h @@ -320,6 +320,22 @@ ERRFMT_ATTR_(2) void grimpl_print_err_msg_( #define GrPrintErrMsg(...) \ grimpl_print_err_msg_(__GRIMPL_SRCLOC__, __VA_ARGS__); +/// @def GR_INTERNAL_UNREACHABLE_ERROR() +/// @brief function-like macro that aborts with a (lethal) error message +/// indicating that +/// +/// This macro should be treated as a function with the signature: +/// +/// [[noreturn]] void GR_INTERNAL_UNREACHABLE_ERROR(); +/// +/// @note +/// Unlike gcc/clang's __builtin_unreachable or C++23's std::unreachable, this +/// aborts the program with an error if its executed (the other cases produce +/// undefined behavior). (An argument could be made for conditionally compiling +/// this macro into the alternatives to test speed) +#define GR_INTERNAL_UNREACHABLE_ERROR() \ +{ grimpl_abort_with_internal_err_(__GRIMPL_SRCLOC__, \ +"location shouldn't be reachable"); } // undefine the attributes so we avoid leaking them // ------------------------------------------------ diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index e58b7322b..f05d4ff1f 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -80,11 +80,13 @@ add_library(grtestutils # files outside of the googletest subdirectory # -> these shouldn't include headers from the googletest subdirectory cmd.hpp cmd.cpp - utils.hpp utils.cpp os.hpp os.cpp + preset.hpp preset.cpp + utils.hpp utils.cpp # files in the googletest subdirectory (reminder: should just be headers!) googletest/check_allclose.hpp + googletest/fixtures.hpp ) target_link_libraries(grtestutils diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp new file mode 100644 index 000000000..b1bc74969 --- /dev/null +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -0,0 +1,110 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Define machinery for creating GoogleTest Fixutures to help test Grackle's +/// C API. +/// +//===----------------------------------------------------------------------===// + +#ifndef GRTEST_FIXTURE +#define GRTEST_FIXTURE + +#include +// because we include gtest.h here, we should NOT include this file in any +// grtest source files (in other words, this should be a header-only file) + +#include + +#include "grackle.h" +#include "../preset.hpp" + +namespace grtest { + +/// Tracks the group of Grackle objects needed for executing API functions +/// +/// The primary motivation for this object's existence is making sure that +/// the allocations get cleaned up when a test fails +/// +/// @note +/// Ideally, we would only make it possible to create a fully initialized +/// instance (in that case, we would delete the default constructor), but that +/// involves a bunch more work +/// +/// We choose to implement this in terms of std::unique_ptr (rather than raw +/// pointers) since it implements proper move semantics for us +class GrackleCtxPack { + /// units used for initializing chemistry_data + code_units initial_units_; + /// the fully initialized chemistry_data instance + std::unique_ptr my_chemistry_; + /// the fully initialized chemistry_data_storage_instance + std::unique_ptr my_rates_; + +public: + /// Construct an uninitialized instance + GrackleCtxPack() : my_chemistry_(nullptr), my_rates_(nullptr) {} + + GrackleCtxPack(GrackleCtxPack&&) = default; + GrackleCtxPack& operator=(GrackleCtxPack&&) = default; + + // forbid copy and assignment operations... + // -> we could re-enable them if we wanted to be able add to clone the + // types (or if we wanted to internally use std::shared_ptr + GrackleCtxPack(const GrackleCtxPack&) = delete; + GrackleCtxPack& operator=(const GrackleCtxPack&) = delete; + + ~GrackleCtxPack() { + if (!this->is_initialized()) { + return; + } + local_free_chemistry_data(this->my_chemistry_.get(), this->my_rates_.get()); + // unique_ptr destructor will handle calls to delete + } + + bool is_initialized() const { return this->my_chemistry_ != nullptr; } + + // getter functions + const code_units& initial_units() const { return this->initial_units_; } + chemistry_data* my_chemistry() { return this->my_chemistry_.get(); } + chemistry_data_storage* my_rates() { return this->my_rates_.get(); } + + /// create an initialized instance from a preset + static GrackleCtxPack create(const FullConfPreset& preset, + InitStatus* status) { + std::unique_ptr my_chemistry(new chemistry_data); + InitStatus tmp = + setup_chemistry_data_from_preset(my_chemistry.get(), preset.chemistry); + if (tmp != InitStatus::success) { + if (status != nullptr) { + *status = tmp; + } + return GrackleCtxPack(); // return an unitialized instance + } + + code_units initial_unit = setup_initial_unit(preset.unit); + std::unique_ptr my_rates( + new chemistry_data_storage); + if (local_initialize_chemistry_data(my_chemistry.get(), my_rates.get(), + &initial_unit) != GR_SUCCESS) { + if (status != nullptr) { + *status = InitStatus::generic_fail; + } + return GrackleCtxPack(); // return an unitialized instance + } + + GrackleCtxPack out; + out.initial_units_ = initial_unit; + out.my_chemistry_ = std::move(my_chemistry); + out.my_rates_ = std::move(my_rates); + return out; + } +}; + +} // namespace grtest + +#endif // GRTEST_FIXTURE diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/preset.cpp new file mode 100644 index 000000000..22621cc18 --- /dev/null +++ b/tests/grtestutils/preset.cpp @@ -0,0 +1,107 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// implement logic pertaining to pre-defined configuration presets +/// +//===----------------------------------------------------------------------===// + +#include "./preset.hpp" +#include "./utils.hpp" + +#include "grackle.h" +#include "status_reporting.h" // GR_INTERNAL_UNREACHABLE_ERROR + +static std::string to_string(const grtest::ChemPreset& preset) { + switch (preset) { + case grtest::ChemPreset::primchem0: + return "pc=0"; + case grtest::ChemPreset::primchem1: + return "pc=1"; + case grtest::ChemPreset::primchem2: + return "pc=2"; + case grtest::ChemPreset::primchem3: + return "pc=3"; + } + + GR_INTERNAL_UNREACHABLE_ERROR(); +} + +grtest::InitStatus grtest::setup_chemistry_data_from_preset( + chemistry_data* my_chem, ChemPreset preset) { + if (local_initialize_chemistry_parameters(my_chem) != GR_SUCCESS) { + return InitStatus::generic_fail; + } + + if (!grtest::set_standard_datafile(*my_chem, "CloudyData_UVB=HM2012.h5")) { + return InitStatus::datafile_notfound; + } + + my_chem->use_grackle = 1; // chemistry on + my_chem->use_isrf_field = 1; + my_chem->with_radiative_cooling = 1; // cooling on + my_chem->metal_cooling = 1; // metal cooling on + my_chem->UVbackground = 1; // UV background on + + switch (preset) { + case ChemPreset::primchem0: { + my_chem->primordial_chemistry = 0; + my_chem->dust_chemistry = 0; + return InitStatus::success; + } + case ChemPreset::primchem1: { + my_chem->primordial_chemistry = 1; + my_chem->dust_chemistry = 1; + return InitStatus::success; + } + case ChemPreset::primchem2: { + my_chem->primordial_chemistry = 2; + my_chem->dust_chemistry = 1; + return InitStatus::success; + } + case ChemPreset::primchem3: { + my_chem->primordial_chemistry = 3; + my_chem->dust_chemistry = 1; + return InitStatus::success; + } + } + GR_INTERNAL_UNREACHABLE_ERROR(); +} + +static std::string to_string(const grtest::InitialUnitPreset& preset) { + switch (preset) { + case grtest::InitialUnitPreset::simple_z0: + return "simpleUnit-z=0"; + } + GR_INTERNAL_UNREACHABLE_ERROR(); +} + +code_units grtest::setup_initial_unit(grtest::InitialUnitPreset preset) { + // since we return in the switch statement, the compiler should always warn + // us if we're missing an enumeration + switch (preset) { + case InitialUnitPreset::simple_z0: { + double initial_redshift = 0.; + code_units my_units; + my_units.comoving_coordinates = 0; // 1 if cosmological sim, 0 if not + my_units.density_units = 1.67e-24; + my_units.length_units = 1.0; + my_units.time_units = 1.0e12; + my_units.a_units = 1.0; // units for the expansion factor + // Set expansion factor to 1 for non-cosmological simulation. + my_units.a_value = 1. / (1. + initial_redshift) / my_units.a_units; + set_velocity_units(&my_units); + return my_units; + } + } + GR_INTERNAL_UNREACHABLE_ERROR(); +} + +void grtest::PrintTo(const grtest::FullConfPreset& preset, std::ostream* os) { + *os << "Preset{" << to_string(preset.chemistry) << ',' + << to_string(preset.unit) << '}'; +} diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp new file mode 100644 index 000000000..5f62c4e7b --- /dev/null +++ b/tests/grtestutils/preset.hpp @@ -0,0 +1,66 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// declare some standard pre-defined configuration presets +/// +//===----------------------------------------------------------------------===// +#ifndef GRTESTUTILS_PRESET_HPP +#define GRTESTUTILS_PRESET_HPP + +#include "grackle.h" +#include +#include + +namespace grtest { + +/// this only exists so that we can determine the reason that a test fails +enum class InitStatus { + success, + generic_fail, + datafile_notfound, +}; + +/// represents different presets for initializing chemistry_data +/// +/// @note +/// In the future, we probably want to add more +enum class ChemPreset { + primchem0, + primchem1, + primchem2, + primchem3, +}; + +/// override the settings of my_chem based on the specified preset +InitStatus setup_chemistry_data_from_preset(chemistry_data* my_chem, + ChemPreset preset); + +/// Preset for constructing the code_unit instance used for initializing the +/// Grackle Solver +/// +/// @note +/// In the future, we probably want to add more +enum class InitialUnitPreset { + simple_z0, // <- no cosmology, z=0 +}; + +/// return a code_unit instance initialized based on the specified preset +code_units setup_initial_unit(InitialUnitPreset preset); + +/// Represents the preset for creating a GrackleCtxPack +struct FullConfPreset { + ChemPreset chemistry; + InitialUnitPreset unit; +}; + +// teach googletest how to print FullConfPreset +void PrintTo(const FullConfPreset& preset, std::ostream* os); + +} // namespace grtest + +#endif // GRTESTUTILS_PRESET_HPP diff --git a/tests/unit/test_ghost_zone.cpp b/tests/unit/test_ghost_zone.cpp index 607da2325..2ee3807e5 100644 --- a/tests/unit/test_ghost_zone.cpp +++ b/tests/unit/test_ghost_zone.cpp @@ -25,6 +25,7 @@ #include #include "grtestutils/utils.hpp" +#include "grtestutils/googletest/fixtures.hpp" #include @@ -33,7 +34,8 @@ #define mh 1.67262171e-24 #define kboltz 1.3806504e-16 -typedef int (*property_func)(code_units*, grackle_field_data*, gr_float*); +typedef int (*property_func)(chemistry_data*, chemistry_data_storage*, + code_units*, grackle_field_data*, gr_float*); typedef std::map> val_vec_map_t; @@ -79,7 +81,8 @@ class FieldInitHelper{ // allocates the grackle_field_data struct void construct_field_data(grackle_field_data& my_fields, grid_props& my_grid_props, - code_units& my_units, + const chemistry_data* my_chemistry, + const code_units& my_units, val_vec_map_t& val_map, std::minstd_rand& generator){ @@ -162,11 +165,11 @@ void construct_field_data(grackle_field_data& my_fields, for (int ix = gx; ix < (mx - gx); ix++){ int i = ix + mx * (iy + my*iz); my_fields.density[i] = 1.0; - my_fields.HI_density[i] = grackle_data->HydrogenFractionByMass * + my_fields.HI_density[i] = my_chemistry->HydrogenFractionByMass * my_fields.density[i]; my_fields.HII_density[i] = tiny_number * my_fields.density[i]; my_fields.HM_density[i] = tiny_number * my_fields.density[i]; - my_fields.HeI_density[i] = (1.0 - grackle_data->HydrogenFractionByMass) + my_fields.HeI_density[i] = (1.0 - my_chemistry->HydrogenFractionByMass) * my_fields.density[i]; my_fields.HeII_density[i] = tiny_number * my_fields.density[i]; my_fields.HeIII_density[i] = tiny_number * my_fields.density[i]; @@ -177,7 +180,7 @@ void construct_field_data(grackle_field_data& my_fields, my_fields.HDI_density[i] = tiny_number * my_fields.density[i]; my_fields.e_density[i] = tiny_number * my_fields.density[i]; // solar metallicity - my_fields.metal_density[i] = grackle_data->SolarMetalFractionByMass * + my_fields.metal_density[i] = my_chemistry->SolarMetalFractionByMass * my_fields.density[i]; my_fields.x_velocity[i] = 0.0; @@ -196,7 +199,7 @@ void construct_field_data(grackle_field_data& my_fields, my_fields.RT_H2_dissociation_rate[i] = 0.0; my_fields.RT_heating_rate[i] = 0.0; - my_fields.isrf_habing[i] = grackle_data->interstellar_radiation_field; + my_fields.isrf_habing[i] = my_chemistry->interstellar_radiation_field; } } } @@ -246,77 +249,8 @@ bool equal_ghost_values(val_vec_map_t& ref, val_vec_map_t& actual, return true; } -namespace { // stuff within anonymous namespace is local to the current file - -/// the following is just a dummy struct that primarily exists to assist with -/// cleanup (and avoid memory leaks) -struct GrackleCtxPack { - bool successful_default = false; - bool successful_data_file = false; - bool successful_init = false; - code_units my_units; - chemistry_data* my_chemistry = nullptr; -}; - -void cleanup_grackle_conditions(GrackleCtxPack& pack) { - if (pack.successful_init) { free_chemistry_data(); } - if (pack.my_chemistry != nullptr) { delete pack.my_chemistry; } -} - -GrackleCtxPack setup_simple_grackle_conditions(int primordial_chemistry) { - /********************************************************************* - / Initial setup of units and chemistry objects. - *********************************************************************/ - - GrackleCtxPack pack; - - // Set initial redshift (for internal units). - double initial_redshift = 0.; - - // First, set up the units system. - // These are conversions from code units to cgs. - pack.my_units.comoving_coordinates = 0; // 1 if cosmological sim, 0 if not - pack.my_units.density_units = 1.67e-24; - pack.my_units.length_units = 1.0; - pack.my_units.time_units = 1.0e12; - pack.my_units.a_units = 1.0; // units for the expansion factor - // Set expansion factor to 1 for non-cosmological simulation. - pack.my_units.a_value = 1. / (1. + initial_redshift) / pack.my_units.a_units; - set_velocity_units(&pack.my_units); - - // Second, create a chemistry object for parameters. - pack.my_chemistry = new chemistry_data; - if (set_default_chemistry_parameters(pack.my_chemistry) != GR_SUCCESS) { - return pack; - } - pack.successful_default=true; - - // Set parameter values for chemistry. - // Access the parameter storage with the struct you've created - // or with the grackle_data pointer declared in grackle.h (see further below). - pack.my_chemistry->use_grackle = 1; // chemistry on - pack.my_chemistry->use_isrf_field = 1; - pack.my_chemistry->with_radiative_cooling = 1; // cooling on - pack.my_chemistry->primordial_chemistry = primordial_chemistry; - pack.my_chemistry->dust_chemistry = (primordial_chemistry == 0) ? 0 : 1; - pack.my_chemistry->metal_cooling = 1; // metal cooling on - pack.my_chemistry->UVbackground = 1; // UV background on - - pack.successful_data_file = grtest::set_standard_datafile( - *pack.my_chemistry, "CloudyData_UVB=HM2012.h5" - ); - if (!pack.successful_data_file) { return pack; } - - // Finally, initialize the chemistry object. - if (initialize_chemistry_data(&pack.my_units) != GR_SUCCESS) { return pack; } - pack.successful_init = true; - return pack; -} - -} // anonymous namespace - // this defines a parameterized test-fixture (it is parameterized on -// primordial_chemistry) +// InitStatus::data_file_not_found) // -> it has a GetParam() method to access the parameters // -> to assist with avoiding memory leaks, I decided to also make this setup // and teardown GrackleCtxPack. @@ -324,30 +258,25 @@ GrackleCtxPack setup_simple_grackle_conditions(int primordial_chemistry) { // really doesn't care how grackle is configured (other than that // primordial_chemistry varies and that it will actually perform // calculations) -class APIConventionTest : public testing::TestWithParam { +class APIConventionTest : public testing::TestWithParam { protected: void SetUp() override { // Disable output grackle_verbose = 0; // called immediately after the constructor (but before the test-case) - int primordial_chemistry = GetParam(); - - pack_ = setup_simple_grackle_conditions(primordial_chemistry); - if (!pack_.successful_default) { - FAIL() << "Error in set_default_chemistry_parameters."; - } else if (!pack_.successful_data_file) { - GTEST_SKIP() << "something went wrong with finding the data file"; - } else if (!pack_.successful_init) { - FAIL() << "Error in initialize_chemistry_data."; + grtest::InitStatus status; + pack_ = grtest::GrackleCtxPack::create(GetParam(), &status); + if (!pack_.is_initialized()) { + if (status == grtest::InitStatus::datafile_notfound) { + GTEST_SKIP() << "something went wrong with finding the data file"; + } else { + FAIL() << "Error in initialize_chemistry_data."; + } } } - void TearDown() override { - cleanup_grackle_conditions(this->pack_); - } - - GrackleCtxPack pack_; + grtest::GrackleCtxPack pack_; }; TEST_P(APIConventionTest, GridZoneStartEnd) { @@ -355,7 +284,8 @@ TEST_P(APIConventionTest, GridZoneStartEnd) { grid_props my_grid_props = {{5,6,7}, {1,0,2}}; // alias the pack_ attribute tracked by the fixture - GrackleCtxPack& pack = pack_; + grtest::GrackleCtxPack& pack = pack_; + code_units my_units = pack_.initial_units(); // initialize pseudo random number generator std::uint32_t seed = 1379069008; @@ -377,7 +307,7 @@ TEST_P(APIConventionTest, GridZoneStartEnd) { // For example: my_fields.density = my_field_map["density"].data() val_vec_map_t my_field_map; construct_field_data( - my_fields, my_grid_props, pack.my_units, my_field_map, generator + my_fields, my_grid_props, pack.my_chemistry(), my_units, my_field_map, generator ); // orig_field_map_copy is a deepcopy of my_field_map. We will use this as a @@ -393,9 +323,11 @@ TEST_P(APIConventionTest, GridZoneStartEnd) { // Evolving the chemistry. // some timestep - double dt = 3.15e7 * 1e6 / pack.my_units.time_units; + double dt = 3.15e7 * 1e6 / my_units.time_units; - if (solve_chemistry(&pack.my_units, &my_fields, dt) != GR_SUCCESS) { + if (local_solve_chemistry(pack.my_chemistry(), pack.my_rates(), + &my_units, &my_fields, + dt)!= GR_SUCCESS) { FAIL() << "Error running solve_chemistry"; } @@ -404,17 +336,17 @@ TEST_P(APIConventionTest, GridZoneStartEnd) { FAIL() << "Some ghost values were modified in solve_chemistry."; } - // Now check what hapens when computing various properties - const char* func_names[5] = {"calculate_cooling_time", - "calculate_temperature", - "calculate_pressure", - "calculate_gamma", - "calculate_dust_temperature"}; - property_func func_ptrs[5] = {&calculate_cooling_time, - &calculate_temperature, - &calculate_pressure, - &calculate_gamma, - &calculate_dust_temperature}; + // Now check what happens when computing various properties + const char* func_names[5] = {"local_calculate_cooling_time", + "local_calculate_temperature", + "local_calculate_pressure", + "local_calculate_gamma", + "local_calculate_dust_temperature"}; + property_func func_ptrs[5] = {&local_calculate_cooling_time, + &local_calculate_temperature, + &local_calculate_pressure, + &local_calculate_gamma, + &local_calculate_dust_temperature}; for (int i = 0; i < 5; i++){ std::uint32_t seed2 = 1860889605; std::minstd_rand generator2(seed2); @@ -426,7 +358,7 @@ TEST_P(APIConventionTest, GridZoneStartEnd) { // perform the calculation property_func func_ptr = func_ptrs[i]; - if ( (*func_ptr)(&pack.my_units, &my_fields, out_vals.data()) + if ( (*func_ptr)(pack.my_chemistry(), pack.my_rates(), &my_units, &my_fields, out_vals.data()) != GR_SUCCESS ) { FAIL() << "Error reported by " << func_names; } @@ -439,6 +371,15 @@ TEST_P(APIConventionTest, GridZoneStartEnd) { } +using grtest::FullConfPreset; +using grtest::ChemPreset; +using grtest::InitialUnitPreset; + INSTANTIATE_TEST_SUITE_P( - VaryingPrimordialChem, APIConventionTest, ::testing::Range(0, 4) + VaryingPrimordialChem, APIConventionTest, + ::testing::Values( + FullConfPreset{ChemPreset::primchem0, InitialUnitPreset::simple_z0}, + FullConfPreset{ChemPreset::primchem1, InitialUnitPreset::simple_z0}, + FullConfPreset{ChemPreset::primchem2, InitialUnitPreset::simple_z0}, + FullConfPreset{ChemPreset::primchem3, InitialUnitPreset::simple_z0}) ); From c4495e7a9cd8bdd0423601b23432e06beb3f4688 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Tue, 16 Dec 2025 16:55:57 -0500 Subject: [PATCH 08/40] transfer grtest_api_fixture out of test_ghost_zone --- tests/grtestutils/googletest/fixtures.hpp | 96 ++++++----------------- tests/grtestutils/preset.hpp | 80 +++++++++++++++++++ tests/unit/test_ghost_zone.cpp | 35 +-------- 3 files changed, 107 insertions(+), 104 deletions(-) diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index b1bc74969..e139ed072 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -25,84 +25,34 @@ namespace grtest { -/// Tracks the group of Grackle objects needed for executing API functions +/// defines a parameterized test-fixture that can used to run a parametrized +/// set of tests that are initialized with different chemistry data presets. /// -/// The primary motivation for this object's existence is making sure that -/// the allocations get cleaned up when a test fails +/// This sets up a GrackleCtxPack (where the contents are configured with +/// appropriate presets) and deallocates that memory at the end of the test. /// -/// @note -/// Ideally, we would only make it possible to create a fully initialized -/// instance (in that case, we would delete the default constructor), but that -/// involves a bunch more work -/// -/// We choose to implement this in terms of std::unique_ptr (rather than raw -/// pointers) since it implements proper move semantics for us -class GrackleCtxPack { - /// units used for initializing chemistry_data - code_units initial_units_; - /// the fully initialized chemistry_data instance - std::unique_ptr my_chemistry_; - /// the fully initialized chemistry_data_storage_instance - std::unique_ptr my_rates_; - -public: - /// Construct an uninitialized instance - GrackleCtxPack() : my_chemistry_(nullptr), my_rates_(nullptr) {} - - GrackleCtxPack(GrackleCtxPack&&) = default; - GrackleCtxPack& operator=(GrackleCtxPack&&) = default; - - // forbid copy and assignment operations... - // -> we could re-enable them if we wanted to be able add to clone the - // types (or if we wanted to internally use std::shared_ptr - GrackleCtxPack(const GrackleCtxPack&) = delete; - GrackleCtxPack& operator=(const GrackleCtxPack&) = delete; - - ~GrackleCtxPack() { - if (!this->is_initialized()) { - return; - } - local_free_chemistry_data(this->my_chemistry_.get(), this->my_rates_.get()); - // unique_ptr destructor will handle calls to delete - } - - bool is_initialized() const { return this->my_chemistry_ != nullptr; } - - // getter functions - const code_units& initial_units() const { return this->initial_units_; } - chemistry_data* my_chemistry() { return this->my_chemistry_.get(); } - chemistry_data_storage* my_rates() { return this->my_rates_.get(); } - - /// create an initialized instance from a preset - static GrackleCtxPack create(const FullConfPreset& preset, - InitStatus* status) { - std::unique_ptr my_chemistry(new chemistry_data); - InitStatus tmp = - setup_chemistry_data_from_preset(my_chemistry.get(), preset.chemistry); - if (tmp != InitStatus::success) { - if (status != nullptr) { - *status = tmp; +/// How To Use +/// ========== +/// To make use of this, you might create a subclass of this type that is named +/// for the test suite. I don't love this strategy, but it seems to be the +/// standard way to do things. We can revisit this in the future. +class ParametrizedConfigPresetFixture + : public testing::TestWithParam { +protected: + void SetUp() override { + // called immediately after the constructor (but before the test-case) + grtest::InitStatus status; + pack_ = GrackleCtxPack::create(GetParam(), &status); + if (!pack_.is_initialized()) { + if (status == InitStatus::datafile_notfound) { + GTEST_SKIP() << "something went wrong with finding the data file"; + } else { + FAIL() << "Error in initialize_chemistry_data."; } - return GrackleCtxPack(); // return an unitialized instance } - - code_units initial_unit = setup_initial_unit(preset.unit); - std::unique_ptr my_rates( - new chemistry_data_storage); - if (local_initialize_chemistry_data(my_chemistry.get(), my_rates.get(), - &initial_unit) != GR_SUCCESS) { - if (status != nullptr) { - *status = InitStatus::generic_fail; - } - return GrackleCtxPack(); // return an unitialized instance - } - - GrackleCtxPack out; - out.initial_units_ = initial_unit; - out.my_chemistry_ = std::move(my_chemistry); - out.my_rates_ = std::move(my_rates); - return out; } + + GrackleCtxPack pack_; }; } // namespace grtest diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp index 5f62c4e7b..c8a22a526 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -61,6 +61,86 @@ struct FullConfPreset { // teach googletest how to print FullConfPreset void PrintTo(const FullConfPreset& preset, std::ostream* os); +/// Tracks the group of Grackle objects needed for executing API functions +/// +/// The primary motivation for this object's existence is making sure that +/// the allocations get cleaned up when a test fails +/// +/// @note +/// Ideally, we would only make it possible to create a fully initialized +/// instance (in that case, we would delete the default constructor), but that +/// involves a bunch more work +/// +/// We choose to implement this in terms of std::unique_ptr (rather than raw +/// pointers) since it implements proper move semantics for us +class GrackleCtxPack { + /// units used for initializing chemistry_data + code_units initial_units_; + /// the fully initialized chemistry_data instance + std::unique_ptr my_chemistry_; + /// the fully initialized chemistry_data_storage_instance + std::unique_ptr my_rates_; + +public: + /// Construct an uninitialized instance + GrackleCtxPack() : my_chemistry_(nullptr), my_rates_(nullptr) {} + + GrackleCtxPack(GrackleCtxPack&&) = default; + GrackleCtxPack& operator=(GrackleCtxPack&&) = default; + + // forbid copy and assignment operations... + // -> we could re-enable them if we wanted to be able add to clone the + // types (or if we wanted to internally use std::shared_ptr + GrackleCtxPack(const GrackleCtxPack&) = delete; + GrackleCtxPack& operator=(const GrackleCtxPack&) = delete; + + ~GrackleCtxPack() { + if (!this->is_initialized()) { + return; + } + local_free_chemistry_data(this->my_chemistry_.get(), this->my_rates_.get()); + // unique_ptr destructor will handle calls to delete + } + + bool is_initialized() const { return this->my_chemistry_ != nullptr; } + + // getter functions + const code_units& initial_units() const { return this->initial_units_; } + chemistry_data* my_chemistry() { return this->my_chemistry_.get(); } + chemistry_data_storage* my_rates() { return this->my_rates_.get(); } + + /// create an initialized instance from a preset + static GrackleCtxPack create(const FullConfPreset& preset, + InitStatus* status) { + std::unique_ptr my_chemistry(new chemistry_data); + InitStatus tmp = + setup_chemistry_data_from_preset(my_chemistry.get(), preset.chemistry); + if (tmp != InitStatus::success) { + if (status != nullptr) { + *status = tmp; + } + return GrackleCtxPack(); // return an unitialized instance + } + + code_units initial_unit = setup_initial_unit(preset.unit); + std::unique_ptr my_rates( + new chemistry_data_storage); + if (local_initialize_chemistry_data(my_chemistry.get(), my_rates.get(), + &initial_unit) != GR_SUCCESS) { + if (status != nullptr) { + *status = InitStatus::generic_fail; + } + return GrackleCtxPack(); // return an unitialized instance + } + + GrackleCtxPack out; + out.initial_units_ = initial_unit; + out.my_chemistry_ = std::move(my_chemistry); + out.my_rates_ = std::move(my_rates); + return out; + } +}; + } // namespace grtest #endif // GRTESTUTILS_PRESET_HPP diff --git a/tests/unit/test_ghost_zone.cpp b/tests/unit/test_ghost_zone.cpp index 2ee3807e5..c565183a6 100644 --- a/tests/unit/test_ghost_zone.cpp +++ b/tests/unit/test_ghost_zone.cpp @@ -249,37 +249,10 @@ bool equal_ghost_values(val_vec_map_t& ref, val_vec_map_t& actual, return true; } -// this defines a parameterized test-fixture (it is parameterized on -// InitStatus::data_file_not_found) -// -> it has a GetParam() method to access the parameters -// -> to assist with avoiding memory leaks, I decided to also make this setup -// and teardown GrackleCtxPack. -// -> Frankly, I don't love this, but I think it is okay since the test -// really doesn't care how grackle is configured (other than that -// primordial_chemistry varies and that it will actually perform -// calculations) -class APIConventionTest : public testing::TestWithParam { - protected: - void SetUp() override { - // Disable output - grackle_verbose = 0; - - // called immediately after the constructor (but before the test-case) - grtest::InitStatus status; - pack_ = grtest::GrackleCtxPack::create(GetParam(), &status); - if (!pack_.is_initialized()) { - if (status == grtest::InitStatus::datafile_notfound) { - GTEST_SKIP() << "something went wrong with finding the data file"; - } else { - FAIL() << "Error in initialize_chemistry_data."; - } - } - } - - grtest::GrackleCtxPack pack_; -}; +class APIGhostZoneTest: public grtest::ParametrizedConfigPresetFixture +{}; -TEST_P(APIConventionTest, GridZoneStartEnd) { +TEST_P(APIGhostZoneTest, GridZoneStartEnd) { grid_props my_grid_props = {{5,6,7}, {1,0,2}}; @@ -376,7 +349,7 @@ using grtest::ChemPreset; using grtest::InitialUnitPreset; INSTANTIATE_TEST_SUITE_P( - VaryingPrimordialChem, APIConventionTest, + VaryingPrimordialChem, APIGhostZoneTest, ::testing::Values( FullConfPreset{ChemPreset::primchem0, InitialUnitPreset::simple_z0}, FullConfPreset{ChemPreset::primchem1, InitialUnitPreset::simple_z0}, From 268b9b4650e03549aeb7f13f707d16d8c9afeb24 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Tue, 16 Dec 2025 19:20:53 -0500 Subject: [PATCH 09/40] introduce some basic tests of ratequery --- tests/grtestutils/googletest/fixtures.hpp | 51 +++++++++++++++++++---- tests/grtestutils/preset.cpp | 10 +++++ tests/grtestutils/preset.hpp | 1 + tests/unit/CMakeLists.txt | 11 ++--- tests/unit/test_api_ratequery.cpp | 49 ++++++++++++++++++++++ tests/unit/test_ghost_zone.cpp | 8 ++-- 6 files changed, 113 insertions(+), 17 deletions(-) create mode 100644 tests/unit/test_api_ratequery.cpp diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index e139ed072..e3998dbbe 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -25,7 +25,42 @@ namespace grtest { -/// defines a parameterized test-fixture that can used to run a parametrized +/// test-fixture that can used to run one or more tests with a chemistry data +/// configuration initialized from a given chemistry presets. +/// +/// This sets up a GrackleCtxPack (where the contents are configured with +/// appropriate presets) and deallocates that memory at the end of the test. +/// +/// How To Use +/// ========== +/// To make use of this fixture in a test-suite called `MyFeatureTest`, you +/// need to either: +/// 1. make a type alias (via `using` or `typedef`), named `MyFeatureTest`, of +/// the relevant instantiation of this class template, OR +/// 2. make a subclass, named `MyFeatureTest`, of the relevant instantiation of +/// this class template +template +class ConfigPresetFixture : public testing::Test { +protected: + void SetUp() override { + // called immediately after the constructor (but before the test-case) + + grtest::InitStatus status; + pack = GrackleCtxPack::create(FullConfPreset{chem_preset, unit_preset}, + &status); + if (!pack.is_initialized()) { + if (status == InitStatus::datafile_notfound) { + GTEST_SKIP() << "something went wrong with finding the data file"; + } else { + FAIL() << "Error in initialize_chemistry_data."; + } + } + } + + GrackleCtxPack pack; +}; + +/// defines a parameterized test-fixture that can be used to run a parametrized /// set of tests that are initialized with different chemistry data presets. /// /// This sets up a GrackleCtxPack (where the contents are configured with @@ -33,17 +68,19 @@ namespace grtest { /// /// How To Use /// ========== -/// To make use of this, you might create a subclass of this type that is named -/// for the test suite. I don't love this strategy, but it seems to be the -/// standard way to do things. We can revisit this in the future. +/// To make use of this fixture in a test-suite called `MyFeatureTest`, you +/// need to either: +/// 1. make a type alias (via `using` or `typedef`), named `MyFeatureTest`, of +/// this class, OR +/// 2. make a subclass, named `MyFeatureTest`, of this class class ParametrizedConfigPresetFixture : public testing::TestWithParam { protected: void SetUp() override { // called immediately after the constructor (but before the test-case) grtest::InitStatus status; - pack_ = GrackleCtxPack::create(GetParam(), &status); - if (!pack_.is_initialized()) { + pack = GrackleCtxPack::create(GetParam(), &status); + if (!pack.is_initialized()) { if (status == InitStatus::datafile_notfound) { GTEST_SKIP() << "something went wrong with finding the data file"; } else { @@ -52,7 +89,7 @@ class ParametrizedConfigPresetFixture } } - GrackleCtxPack pack_; + GrackleCtxPack pack; }; } // namespace grtest diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/preset.cpp index 22621cc18..53f3a64e3 100644 --- a/tests/grtestutils/preset.cpp +++ b/tests/grtestutils/preset.cpp @@ -26,6 +26,8 @@ static std::string to_string(const grtest::ChemPreset& preset) { return "pc=2"; case grtest::ChemPreset::primchem3: return "pc=3"; + case grtest::ChemPreset::primchem4_dustspecies3: + return "pc=3-dust_species=4"; } GR_INTERNAL_UNREACHABLE_ERROR(); @@ -68,6 +70,14 @@ grtest::InitStatus grtest::setup_chemistry_data_from_preset( my_chem->dust_chemistry = 1; return InitStatus::success; } + case ChemPreset::primchem4_dustspecies3: { + my_chem->primordial_chemistry = 4; + my_chem->dust_chemistry = 1; + my_chem->metal_chemistry = 1; + my_chem->dust_species = 1; + my_chem->use_dust_density_field = 1; + return InitStatus::success; + } } GR_INTERNAL_UNREACHABLE_ERROR(); } diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp index c8a22a526..b30f69632 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -34,6 +34,7 @@ enum class ChemPreset { primchem1, primchem2, primchem3, + primchem4_dustspecies3, }; /// override the settings of my_chem based on the specified preset diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index fd5a75ac1..915a28c93 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -22,11 +22,12 @@ add_executable(runVisitorTests test_visitor.cpp) target_link_libraries(runVisitorTests testdeps) gtest_discover_tests(runVisitorTests) -# one might argue that the following is more of an integration or end-to-end -# test than a unit-test -add_executable(runGhostZoneTests test_ghost_zone.cpp) -target_link_libraries(runGhostZoneTests testdeps) -gtest_discover_tests(runGhostZoneTests) +# tests of the API functions +# -> one might argue that these are better classified as integration or +# end-to-end tests than as unit-test +add_executable(runApiTests test_ghost_zone.cpp test_api_ratequery.cpp) +target_link_libraries(runApiTests testdeps) +gtest_discover_tests(runApiTests) # this target tests that the members of the chemistry_data struct can be # accessed through the "dynamic api." The test cases in this target are diff --git a/tests/unit/test_api_ratequery.cpp b/tests/unit/test_api_ratequery.cpp new file mode 100644 index 000000000..260d69ba1 --- /dev/null +++ b/tests/unit/test_api_ratequery.cpp @@ -0,0 +1,49 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Define some basic tests of the experimental ratequery API +/// +//===----------------------------------------------------------------------===// + +#include +#include +#include "grtestutils/googletest/fixtures.hpp" + +#include "grackle.h" + +/// returns the rateid used to denote invalid rate names +grunstable_rateid_type get_invalid_rateid(const grtest::GrackleCtxPack& pack) { + // although we don't use pack, yet a forthcoming refactor requires it + return grunstable_ratequery_id(nullptr); +} + +using SimpleRateQueryTest = + grtest::ConfigPresetFixture; + +TEST_F(SimpleRateQueryTest, InvalidIthRate) { + const char* name = grunstable_ith_rate( + std::numeric_limits::max(), nullptr); + EXPECT_EQ(name, nullptr); +} + +TEST_F(SimpleRateQueryTest, EmptyNameQuery) { + grunstable_rateid_type rateid = grunstable_ratequery_id(""); + EXPECT_EQ(rateid, get_invalid_rateid(pack)); +} + +TEST_F(SimpleRateQueryTest, InvalidNameQuery) { + grunstable_rateid_type rateid = grunstable_ratequery_id("NotAValidName"); + EXPECT_EQ(rateid, get_invalid_rateid(pack)); +} + +TEST_F(SimpleRateQueryTest, PtrInvalidRateId) { + double* ptr = + grunstable_ratequery_get_ptr(pack.my_rates(), get_invalid_rateid(pack)); + EXPECT_EQ(ptr, nullptr); +} diff --git a/tests/unit/test_ghost_zone.cpp b/tests/unit/test_ghost_zone.cpp index c565183a6..5bdf4ac8a 100644 --- a/tests/unit/test_ghost_zone.cpp +++ b/tests/unit/test_ghost_zone.cpp @@ -249,16 +249,14 @@ bool equal_ghost_values(val_vec_map_t& ref, val_vec_map_t& actual, return true; } -class APIGhostZoneTest: public grtest::ParametrizedConfigPresetFixture -{}; +using APIGhostZoneTest = grtest::ParametrizedConfigPresetFixture; TEST_P(APIGhostZoneTest, GridZoneStartEnd) { grid_props my_grid_props = {{5,6,7}, {1,0,2}}; - // alias the pack_ attribute tracked by the fixture - grtest::GrackleCtxPack& pack = pack_; - code_units my_units = pack_.initial_units(); + // the pack attribute holds grtest::GrackleCtxPack + code_units my_units = pack.initial_units(); // initialize pseudo random number generator std::uint32_t seed = 1379069008; From cd7deacef442058e1305c30516bfd65a056aa223 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Tue, 16 Dec 2025 21:10:15 -0500 Subject: [PATCH 10/40] add more ratequery tests --- tests/unit/test_api_ratequery.cpp | 112 ++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/tests/unit/test_api_ratequery.cpp b/tests/unit/test_api_ratequery.cpp index 260d69ba1..9033334d6 100644 --- a/tests/unit/test_api_ratequery.cpp +++ b/tests/unit/test_api_ratequery.cpp @@ -10,7 +10,10 @@ /// //===----------------------------------------------------------------------===// +#include #include +#include +#include #include #include "grtestutils/googletest/fixtures.hpp" @@ -47,3 +50,112 @@ TEST_F(SimpleRateQueryTest, PtrInvalidRateId) { grunstable_ratequery_get_ptr(pack.my_rates(), get_invalid_rateid(pack)); EXPECT_EQ(ptr, nullptr); } + +// will be implemented (in a much more robust manner) in the near future +unsigned long long grunstable_ratequery_nrates( + const chemistry_data_storage* my_rates) { + // current implementation is stupid! (in future, will use my_rates) + unsigned long long i = 0; + while (nullptr != grunstable_ith_rate(i, nullptr)) { + i++; + } + return i; +} + +// most of the remaining tests (and future planned tests) involve iterating +// through all rates made available via the ratequery interface. To make the +// tests themselves as easy to read as possible, we implate a C++-style +// iterator to wrap part of the interface + +struct RateNameIdPair { + std::string name; + grunstable_rateid_type rateid; +}; + +class RQIterator { + chemistry_data_storage* my_rates_; + unsigned long long counter_; + unsigned long long n_rates_; + RateNameIdPair pair_; + + RQIterator& update_pair_and_ret_(unsigned long long val) { + if (val < n_rates_) { + pair_.name = std::string(grunstable_ith_rate(val, &pair_.rateid)); + } + return *this; + } + +public: + using iterator_category = std::input_iterator_tag; + using value_type = RateNameIdPair; + using difference_type = std::ptrdiff_t; + using pointer = const RateNameIdPair*; + using reference = const RateNameIdPair; + + RQIterator(unsigned long long counter, unsigned long long n_rates, + chemistry_data_storage* my_rates) + : my_rates_(my_rates), counter_(counter), n_rates_(n_rates) { + update_pair_and_ret_(counter); + } + + bool operator==(RQIterator other) const { + return (counter_ == other.counter_) && (my_rates_ == other.my_rates_); + } + + bool operator!=(RQIterator other) const { return !(*this == other); } + reference operator*() const { return pair_; } + RQIterator& operator++() { return update_pair_and_ret_(++counter_); } + + RQIterator operator++(int) { + RQIterator ret = *this; + ++(*this); + return ret; + } +}; + +// used for creating the iterator and within range-based for-loops +class RateQueryRange { + grtest::GrackleCtxPack& pack_; + long long n_rates_; + +public: + explicit RateQueryRange(grtest::GrackleCtxPack& pack) + : pack_(pack), n_rates_(grunstable_ratequery_nrates(pack.my_rates())) {} + + RQIterator begin() { return RQIterator(0, n_rates_, pack_.my_rates()); } + RQIterator end() { return RQIterator(n_rates_, n_rates_, pack_.my_rates()); } +}; + +using ParametrizedRateQueryTest = grtest::ParametrizedConfigPresetFixture; + +TEST_P(ParametrizedRateQueryTest, AllUnique) { + std::set name_set; + std::set id_set; + for (const RateNameIdPair pair : RateQueryRange(pack)) { + ASSERT_TRUE(name_set.insert(pair.name).second) + << "the name, \"" << pair.name << "\" appears more than once"; + ASSERT_TRUE(id_set.insert(pair.rateid).second) + << "the id, " << pair.rateid << " appears more than once"; + } +} + +TEST_P(ParametrizedRateQueryTest, ConsistentIDs) { + for (const RateNameIdPair pair : RateQueryRange(pack)) { + grunstable_rateid_type rateid = grunstable_ratequery_id(pair.name.c_str()); + EXPECT_EQ(rateid, pair.rateid); + } +} + +using grtest::ChemPreset; +using grtest::FullConfPreset; +using grtest::InitialUnitPreset; + +INSTANTIATE_TEST_SUITE_P( + /* 1st arg is intentionally empty */, ParametrizedRateQueryTest, + ::testing::Values( + FullConfPreset{ChemPreset::primchem0, InitialUnitPreset::simple_z0}, + FullConfPreset{ChemPreset::primchem1, InitialUnitPreset::simple_z0}, + FullConfPreset{ChemPreset::primchem2, InitialUnitPreset::simple_z0}, + FullConfPreset{ChemPreset::primchem3, InitialUnitPreset::simple_z0}, + FullConfPreset{ChemPreset::primchem4_dustspecies3, + InitialUnitPreset::simple_z0})); From 24257017a30d3c805894e966c81b3bc50824d5f2 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Tue, 16 Dec 2025 21:11:49 -0500 Subject: [PATCH 11/40] tweak the name of the test_ghost_zone tests --- tests/unit/test_ghost_zone.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_ghost_zone.cpp b/tests/unit/test_ghost_zone.cpp index 5bdf4ac8a..16628721f 100644 --- a/tests/unit/test_ghost_zone.cpp +++ b/tests/unit/test_ghost_zone.cpp @@ -347,7 +347,7 @@ using grtest::ChemPreset; using grtest::InitialUnitPreset; INSTANTIATE_TEST_SUITE_P( - VaryingPrimordialChem, APIGhostZoneTest, + /* 1st arg is intentionally empty */, APIGhostZoneTest, ::testing::Values( FullConfPreset{ChemPreset::primchem0, InitialUnitPreset::simple_z0}, FullConfPreset{ChemPreset::primchem1, InitialUnitPreset::simple_z0}, From 37fad17d0a47c5822aa05af0bd578657900521c3 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 18 Dec 2025 17:01:18 -0500 Subject: [PATCH 12/40] factor out machinery for creating adaptor --- tests/grtestutils/CMakeLists.txt | 1 + tests/grtestutils/iterator_adaptor.hpp | 140 +++++++++++++++++++++++++ tests/unit/test_api_ratequery.cpp | 81 ++------------ 3 files changed, 147 insertions(+), 75 deletions(-) create mode 100644 tests/grtestutils/iterator_adaptor.hpp diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index f05d4ff1f..27bf11d17 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -80,6 +80,7 @@ add_library(grtestutils # files outside of the googletest subdirectory # -> these shouldn't include headers from the googletest subdirectory cmd.hpp cmd.cpp + iterator_adaptor.hpp os.hpp os.cpp preset.hpp preset.cpp utils.hpp utils.cpp diff --git a/tests/grtestutils/iterator_adaptor.hpp b/tests/grtestutils/iterator_adaptor.hpp new file mode 100644 index 000000000..ad84a3559 --- /dev/null +++ b/tests/grtestutils/iterator_adaptor.hpp @@ -0,0 +1,140 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Declare and implement the IteratorAdaptor +/// +//===----------------------------------------------------------------------===// +#ifndef GRTESTUTILS_ITERATOR_ADAPTOR_HPP +#define GRTESTUTILS_ITERATOR_ADAPTOR_HPP + +#include +#include + +#include "grackle.h" +#include "preset.hpp" + +namespace grtest { + +/// the standard value-type that an IteratorAdaptor instantiation refers to +struct NameIdPair { + std::string name; + long long id; +}; + +/// implements a C++ style InputIterator by adapting a simple Plugin type +/// that wraps a set of Grackle functions +/// +/// This is useful for making use of C++ standard library algorithms and +/// (arguably more importantly) making use of range-based for-loops +template +class IteratorAdaptor { + unsigned long long counter_; + unsigned long long n_rates_; + Plugin plugin_; + NameIdPair current_pair_; + + /// Updates current_pair_ and returns `*this` + IteratorAdaptor& update_pair_and_ret_(unsigned long long current_count) { + if (current_count < this->n_rates_) { + this->current_pair_ = this->plugin_(current_count); + } + return *this; + } + +public: + using iterator_category = std::input_iterator_tag; + using value_type = NameIdPair; + using difference_type = std::ptrdiff_t; + using pointer = const NameIdPair*; + using reference = const NameIdPair; + + /// construct a new instance + IteratorAdaptor(unsigned long long counter, unsigned long long n_rates, + Plugin plugin) + : counter_(counter), n_rates_(n_rates), plugin_(plugin) { + update_pair_and_ret_(counter); + } + + /// implements the equality operation + bool operator==(const IteratorAdaptor& other) const { + return (counter_ == other.counter_) && (plugin_ == other.plugin_); + } + + /// implements the inequality operation + bool operator!=(const IteratorAdaptor& other) const { + return !(*this == other); + } + + /// implements the dereference operation + reference operator*() const { return current_pair_; } + + /// implements the prefix increment operation + /// + /// This effectively implements `++x`, which increments the value of `x` + /// before determining the returned value. In other words, `++x` returns the + /// value of `x` from **after** after the increment + IteratorAdaptor& operator++() { return update_pair_and_ret_(++counter_); } + + /// implements the prefix increment operation + /// + /// This effectively implements `x++`, which increments the value of `x` + /// after determining the returned value. In other words, `x++` returns the + /// value of `x` from **before** the increment + IteratorAdaptor operator++(int) { + IteratorAdaptor ret = *this; + ++(*this); + return ret; + } +}; + +// Now lets use this machinery to implement logic iterating over the names +// accessible through the ratequery api + +struct RateQueryPlugin { + chemistry_data_storage* my_rates; + + NameIdPair operator()(unsigned long long i) const { + grunstable_rateid_type tmp; + const char* name = grunstable_ith_rate(i, &tmp); + return NameIdPair{name, tmp}; + } + + bool operator==(const RateQueryPlugin& other) const { + return my_rates == other.my_rates; + } +}; + +// will be implemented (in a much more robust manner) in the near future +inline unsigned long long grunstable_ratequery_nrates( + const chemistry_data_storage* my_rates) { + // current implementation is stupid! (in future, will use my_rates) + unsigned long long i = 0; + while (nullptr != grunstable_ith_rate(i, nullptr)) { + i++; + } + return i; +} + +/// used for creating the iterator and within range-based for-loops +class RateQueryRange { + RateQueryPlugin plugin_; + using iterator = IteratorAdaptor; + long long n_rates_; + +public: + explicit RateQueryRange(grtest::GrackleCtxPack& pack) + : plugin_(RateQueryPlugin{pack.my_rates()}), + n_rates_(grunstable_ratequery_nrates(pack.my_rates())) {} + + iterator begin() { return iterator(0, n_rates_, plugin_); } + iterator end() { return iterator(n_rates_, n_rates_, plugin_); } +}; + +} // namespace grtest + +#endif // GRTESTUTILS_ITERATOR_ADAPTOR_HPP diff --git a/tests/unit/test_api_ratequery.cpp b/tests/unit/test_api_ratequery.cpp index 9033334d6..02d0882bd 100644 --- a/tests/unit/test_api_ratequery.cpp +++ b/tests/unit/test_api_ratequery.cpp @@ -15,6 +15,7 @@ #include #include #include +#include "grtestutils/iterator_adaptor.hpp" #include "grtestutils/googletest/fixtures.hpp" #include "grackle.h" @@ -51,98 +52,28 @@ TEST_F(SimpleRateQueryTest, PtrInvalidRateId) { EXPECT_EQ(ptr, nullptr); } -// will be implemented (in a much more robust manner) in the near future -unsigned long long grunstable_ratequery_nrates( - const chemistry_data_storage* my_rates) { - // current implementation is stupid! (in future, will use my_rates) - unsigned long long i = 0; - while (nullptr != grunstable_ith_rate(i, nullptr)) { - i++; - } - return i; -} - // most of the remaining tests (and future planned tests) involve iterating // through all rates made available via the ratequery interface. To make the // tests themselves as easy to read as possible, we implate a C++-style // iterator to wrap part of the interface -struct RateNameIdPair { - std::string name; - grunstable_rateid_type rateid; -}; - -class RQIterator { - chemistry_data_storage* my_rates_; - unsigned long long counter_; - unsigned long long n_rates_; - RateNameIdPair pair_; - - RQIterator& update_pair_and_ret_(unsigned long long val) { - if (val < n_rates_) { - pair_.name = std::string(grunstable_ith_rate(val, &pair_.rateid)); - } - return *this; - } - -public: - using iterator_category = std::input_iterator_tag; - using value_type = RateNameIdPair; - using difference_type = std::ptrdiff_t; - using pointer = const RateNameIdPair*; - using reference = const RateNameIdPair; - - RQIterator(unsigned long long counter, unsigned long long n_rates, - chemistry_data_storage* my_rates) - : my_rates_(my_rates), counter_(counter), n_rates_(n_rates) { - update_pair_and_ret_(counter); - } - - bool operator==(RQIterator other) const { - return (counter_ == other.counter_) && (my_rates_ == other.my_rates_); - } - - bool operator!=(RQIterator other) const { return !(*this == other); } - reference operator*() const { return pair_; } - RQIterator& operator++() { return update_pair_and_ret_(++counter_); } - - RQIterator operator++(int) { - RQIterator ret = *this; - ++(*this); - return ret; - } -}; - -// used for creating the iterator and within range-based for-loops -class RateQueryRange { - grtest::GrackleCtxPack& pack_; - long long n_rates_; - -public: - explicit RateQueryRange(grtest::GrackleCtxPack& pack) - : pack_(pack), n_rates_(grunstable_ratequery_nrates(pack.my_rates())) {} - - RQIterator begin() { return RQIterator(0, n_rates_, pack_.my_rates()); } - RQIterator end() { return RQIterator(n_rates_, n_rates_, pack_.my_rates()); } -}; - using ParametrizedRateQueryTest = grtest::ParametrizedConfigPresetFixture; TEST_P(ParametrizedRateQueryTest, AllUnique) { std::set name_set; std::set id_set; - for (const RateNameIdPair pair : RateQueryRange(pack)) { + for (const grtest::NameIdPair pair : grtest::RateQueryRange(pack)) { ASSERT_TRUE(name_set.insert(pair.name).second) << "the name, \"" << pair.name << "\" appears more than once"; - ASSERT_TRUE(id_set.insert(pair.rateid).second) - << "the id, " << pair.rateid << " appears more than once"; + ASSERT_TRUE(id_set.insert(pair.id).second) + << "the id, " << pair.id << " appears more than once"; } } TEST_P(ParametrizedRateQueryTest, ConsistentIDs) { - for (const RateNameIdPair pair : RateQueryRange(pack)) { + for (const grtest::NameIdPair pair : grtest::RateQueryRange(pack)) { grunstable_rateid_type rateid = grunstable_ratequery_id(pair.name.c_str()); - EXPECT_EQ(rateid, pair.rateid); + EXPECT_EQ(rateid, pair.id); } } From 39e1c0c7d8183b53c366f033e69b7a4fe95e8ec7 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 19 Dec 2025 08:14:21 -0500 Subject: [PATCH 13/40] fix a broken header include --- tests/grtestutils/googletest/fixtures.hpp | 3 --- tests/grtestutils/preset.hpp | 1 + 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index e3998dbbe..25c53bb22 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -18,9 +18,6 @@ // because we include gtest.h here, we should NOT include this file in any // grtest source files (in other words, this should be a header-only file) -#include - -#include "grackle.h" #include "../preset.hpp" namespace grtest { diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp index b30f69632..6f942167d 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -13,6 +13,7 @@ #define GRTESTUTILS_PRESET_HPP #include "grackle.h" +#include #include #include From 804efcfe0f22d315c94368ea2be4a2e2131aa42e Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 11 Dec 2025 20:46:54 -0500 Subject: [PATCH 14/40] some light refactoring of the rate utlities --- src/clib/rate_utils.cpp | 50 ++++++++++++++++++++++++----------------- 1 file changed, 30 insertions(+), 20 deletions(-) diff --git a/src/clib/rate_utils.cpp b/src/clib/rate_utils.cpp index f24cf66fd..4cb8d3315 100644 --- a/src/clib/rate_utils.cpp +++ b/src/clib/rate_utils.cpp @@ -10,9 +10,8 @@ /// //===----------------------------------------------------------------------===// -#include // bool, true, and false are defined -#include // strcmp -#include // LLONG_MAX +#include // strcmp +#include // LLONG_MAX #include "grackle.h" #include "internal_types.hpp" // CollisionalRxnRateCollection #include "LUT.hpp" // CollisionalRxnLUT @@ -39,15 +38,17 @@ // offsetof in this fashion // +namespace grackle::impl::ratequery { + // we have reserved the right to change this value at any time enum { UNDEFINED_RATE_ID_ = 0 }; // introduce some basic machinery to help us implement dynamic lookup of rates -typedef struct { +struct rateprop_ { double* data; const char* name; -} rateprop_; +}; static inline rateprop_ mk_rateprop_(double* rate, const char* name) { rateprop_ out; @@ -68,9 +69,9 @@ static inline rateprop_ mk_rateprop_standard_kcol_( } #define MKPROP_(PTR, NAME) \ - mk_rateprop_(((PTR) == NULL) ? NULL : (PTR)->NAME, #NAME) + mk_rateprop_(((PTR) == nullptr) ? nullptr : (PTR)->NAME, #NAME) #define MKPROP_SCALAR_(PTR, NAME) \ - mk_rateprop_(((PTR) == NULL) ? NULL : &((PTR)->NAME), #NAME) + mk_rateprop_(((PTR) == nullptr) ? nullptr : &((PTR)->NAME), #NAME) #define MKPROP_STANDARD_KCOL_(PTR, NAME, INDEX) \ mk_rateprop_standard_kcol_(PTR, #NAME, INDEX) @@ -97,7 +98,7 @@ static rateprop_ get_CollisionalRxn_rateprop_(chemistry_data_storage* my_rates, #include "collisional_rxn_rate_members.def" #undef ENTRY default: { - rateprop_ out = {NULL, NULL}; + rateprop_ out = {nullptr, nullptr}; return out; } } @@ -145,7 +146,7 @@ static rateprop_ get_MiscRxn_rateprop_(chemistry_data_storage* my_rates, case MiscRxn_k31: return MKPROP_SCALAR_(my_rates, k31); default: { - rateprop_ out = {NULL, NULL}; + rateprop_ out = {nullptr, nullptr}; return out; } } @@ -199,40 +200,49 @@ static struct ratequery_rslt_ query_rateprop_(chemistry_data_storage* my_rates, } total_len += cur_set.len; } - struct ratequery_rslt_ out = {UNDEFINED_RATE_ID_, {NULL, NULL}}; + struct ratequery_rslt_ out = {UNDEFINED_RATE_ID_, {nullptr, nullptr}}; return out; } +} // namespace grackle::impl::ratequery + // here we implement the public API // -------------------------------- extern "C" grunstable_rateid_type grunstable_ratequery_id(const char* name) { - if (name == NULL) { - return UNDEFINED_RATE_ID_; + namespace rate_q = grackle::impl::ratequery; + + if (name == nullptr) { + return rate_q::UNDEFINED_RATE_ID_; } - for (int set_idx = 0; set_idx < rate_registry_.len; set_idx++) { - const struct rateprop_set_ cur_set = rate_registry_.sets[set_idx]; + for (int set_idx = 0; set_idx < rate_q::rate_registry_.len; set_idx++) { + const rate_q::rateprop_set_ cur_set = rate_q::rate_registry_.sets[set_idx]; for (int i = 0; i < cur_set.len; i++) { - rateprop_ prop = cur_set.fn(NULL, i); - if (strcmp(name, prop.name) == 0) { + rate_q::rateprop_ prop = cur_set.fn(nullptr, i); + if (std::strcmp(name, prop.name) == 0) { return cur_set.id_offset + i; } } } - return UNDEFINED_RATE_ID_; + return rate_q::UNDEFINED_RATE_ID_; } extern "C" double* grunstable_ratequery_get_ptr( chemistry_data_storage* my_rates, grunstable_rateid_type rate_id) { - return query_rateprop_(my_rates, rate_id, true).prop.data; + namespace rate_q = grackle::impl::ratequery; + + return rate_q::query_rateprop_(my_rates, rate_id, true).prop.data; } extern "C" const char* grunstable_ith_rate( unsigned long long i, grunstable_rateid_type* out_rate_id) { + namespace rate_q = grackle::impl::ratequery; + const long long sanitized_i = (i < LLONG_MAX) ? (long long)i : -1; - struct ratequery_rslt_ tmp = query_rateprop_(NULL, sanitized_i, false); - if (out_rate_id != NULL) { + rate_q::ratequery_rslt_ tmp = + rate_q::query_rateprop_(nullptr, sanitized_i, false); + if (out_rate_id != nullptr) { *out_rate_id = tmp.rate_id; } return tmp.prop.name; From ff9eca6b864d15fdd1b449bff0d75db29336e85a Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 11 Dec 2025 20:58:36 -0500 Subject: [PATCH 15/40] rename rateprop -> ratequery::Descr (as in a rate description) --- src/clib/rate_utils.cpp | 105 ++++++++++++++++++++-------------------- 1 file changed, 52 insertions(+), 53 deletions(-) diff --git a/src/clib/rate_utils.cpp b/src/clib/rate_utils.cpp index 4cb8d3315..67ceee203 100644 --- a/src/clib/rate_utils.cpp +++ b/src/clib/rate_utils.cpp @@ -45,35 +45,36 @@ enum { UNDEFINED_RATE_ID_ = 0 }; // introduce some basic machinery to help us implement dynamic lookup of rates -struct rateprop_ { +/// A description of a rate +struct Descr { double* data; const char* name; }; -static inline rateprop_ mk_rateprop_(double* rate, const char* name) { - rateprop_ out; +static inline Descr mk_Descr(double* rate, const char* name) { + Descr out; out.data = rate; out.name = name; return out; } -static inline rateprop_ mk_rateprop_standard_kcol_( - chemistry_data_storage* my_rates, const char* name, int index) { +static inline Descr mk_Descr_standard_kcol_(chemistry_data_storage* my_rates, + const char* name, int index) { if ((my_rates == nullptr) || (my_rates->opaque_storage == nullptr) || (my_rates->opaque_storage->kcol_rate_tables == nullptr)) { - return mk_rateprop_(nullptr, name); + return mk_Descr(nullptr, name); } else { - return mk_rateprop_(my_rates->opaque_storage->kcol_rate_tables->data[index], - name); + return mk_Descr(my_rates->opaque_storage->kcol_rate_tables->data[index], + name); } } -#define MKPROP_(PTR, NAME) \ - mk_rateprop_(((PTR) == nullptr) ? nullptr : (PTR)->NAME, #NAME) -#define MKPROP_SCALAR_(PTR, NAME) \ - mk_rateprop_(((PTR) == nullptr) ? nullptr : &((PTR)->NAME), #NAME) -#define MKPROP_STANDARD_KCOL_(PTR, NAME, INDEX) \ - mk_rateprop_standard_kcol_(PTR, #NAME, INDEX) +#define MKDESCR_(PTR, NAME) \ + mk_Descr(((PTR) == nullptr) ? nullptr : (PTR)->NAME, #NAME) +#define MKDESCR_SCALAR_(PTR, NAME) \ + mk_Descr(((PTR) == nullptr) ? nullptr : &((PTR)->NAME), #NAME) +#define MKDESCR_STANDARD_KCOL_(PTR, NAME, INDEX) \ + mk_Descr_standard_kcol_(PTR, #NAME, INDEX) // Create machinery to lookup Standard-Form Collisional Reaction Rates // ------------------------------------------------------------------- @@ -88,17 +89,16 @@ static inline rateprop_ mk_rateprop_standard_kcol_( // of the previous value (if a value isn't explicitly specified) // - CollisionalRxnLUT::NUM_ENTRIES specifies the number of other enumeration // constants (excluding CollisionalRxnLUT::NUM_ENTRIES) in the enum -static rateprop_ get_CollisionalRxn_rateprop_(chemistry_data_storage* my_rates, - int i) { +static Descr get_CollisionalRxn_Descr(chemistry_data_storage* my_rates, int i) { switch (i) { #define ENTRY(NAME) \ case CollisionalRxnLUT::NAME: { \ - return MKPROP_STANDARD_KCOL_(my_rates, NAME, CollisionalRxnLUT::NAME); \ + return MKDESCR_STANDARD_KCOL_(my_rates, NAME, CollisionalRxnLUT::NAME); \ } #include "collisional_rxn_rate_members.def" #undef ENTRY default: { - rateprop_ out = {nullptr, nullptr}; + Descr out = {nullptr, nullptr}; return out; } } @@ -121,32 +121,31 @@ enum MiscRxnRateKind_ { MiscRxn_NRATES // <- will hold the number of reactions }; -static rateprop_ get_MiscRxn_rateprop_(chemistry_data_storage* my_rates, - int i) { +static Descr get_MiscRxn_Descr(chemistry_data_storage* my_rates, int i) { switch (i) { // density dependent version of k13 (which is a CollisionalRxn) case MiscRxn_k13dd: - return MKPROP_(my_rates, k13dd); + return MKDESCR_(my_rates, k13dd); // Radiative rates for 6-species (for external field): case MiscRxn_k24: - return MKPROP_SCALAR_(my_rates, k24); + return MKDESCR_SCALAR_(my_rates, k24); case MiscRxn_k25: - return MKPROP_SCALAR_(my_rates, k25); + return MKDESCR_SCALAR_(my_rates, k25); case MiscRxn_k26: - return MKPROP_SCALAR_(my_rates, k26); + return MKDESCR_SCALAR_(my_rates, k26); // Radiative rates for 9-species case MiscRxn_k27: - return MKPROP_SCALAR_(my_rates, k27); + return MKDESCR_SCALAR_(my_rates, k27); case MiscRxn_k28: - return MKPROP_SCALAR_(my_rates, k28); + return MKDESCR_SCALAR_(my_rates, k28); case MiscRxn_k29: - return MKPROP_SCALAR_(my_rates, k29); + return MKDESCR_SCALAR_(my_rates, k29); case MiscRxn_k30: - return MKPROP_SCALAR_(my_rates, k30); + return MKDESCR_SCALAR_(my_rates, k30); case MiscRxn_k31: - return MKPROP_SCALAR_(my_rates, k31); + return MKDESCR_SCALAR_(my_rates, k31); default: { - rateprop_ out = {nullptr, nullptr}; + Descr out = {nullptr, nullptr}; return out; } } @@ -155,47 +154,47 @@ static rateprop_ get_MiscRxn_rateprop_(chemistry_data_storage* my_rates, // define some additional generic machinery // ---------------------------------------- -#define RATE_SET_COUNT 2 -typedef rateprop_ fetch_rateprop_fn(chemistry_data_storage*, int); -struct rateprop_set_ { +#define DESCR_SET_COUNT 2 +typedef Descr fetch_Descr_fn(chemistry_data_storage*, int); +struct Descr_set_ { int id_offset; int len; - fetch_rateprop_fn* fn; + fetch_Descr_fn* fn; }; struct rate_registry_type_ { int len; - struct rateprop_set_ sets[RATE_SET_COUNT]; + Descr_set_ sets[DESCR_SET_COUNT]; }; static const struct rate_registry_type_ rate_registry_ = { - /* len: */ RATE_SET_COUNT, + /* len: */ DESCR_SET_COUNT, /* sets: */ { - {1000, CollisionalRxnLUT::NUM_ENTRIES, &get_CollisionalRxn_rateprop_}, - {2000, MiscRxn_NRATES, &get_MiscRxn_rateprop_}}}; + {1000, CollisionalRxnLUT::NUM_ENTRIES, &get_CollisionalRxn_Descr}, + {2000, MiscRxn_NRATES, &get_MiscRxn_Descr}}}; struct ratequery_rslt_ { grunstable_rateid_type rate_id; - rateprop_ prop; + Descr descr; }; -/// internal function to search for the rate_property i +/// internal function to search for the rate description i /// /// We interpret i as rate_id, when use_rate_id is true. Otherwise, we just -/// look for the ith rateprop (we introduce an artificial distinction between -/// the 2 cases because we want to reserve the right to be able to change the -/// relationship if it becomes convenient in the future) -static struct ratequery_rslt_ query_rateprop_(chemistry_data_storage* my_rates, - long long i, bool use_rate_id) { +/// look for the ith rate description (we introduce an artificial distinction +/// between the 2 cases because we want to reserve the right to be able to +/// change the relationship if it becomes convenient in the future) +static struct ratequery_rslt_ query_Descr(chemistry_data_storage* my_rates, + long long i, bool use_rate_id) { int total_len = 0; // <- we increment this as we go through the rates for (int set_idx = 0; set_idx < rate_registry_.len; set_idx++) { - const struct rateprop_set_ cur_set = rate_registry_.sets[set_idx]; + const struct Descr_set_ cur_set = rate_registry_.sets[set_idx]; const long long tmp = (use_rate_id) ? i - cur_set.id_offset : i - total_len; if ((tmp >= 0) && (tmp < cur_set.len)) { struct ratequery_rslt_ out; out.rate_id = tmp + cur_set.id_offset; - out.prop = cur_set.fn(my_rates, tmp); + out.descr = cur_set.fn(my_rates, tmp); return out; } total_len += cur_set.len; @@ -217,10 +216,10 @@ extern "C" grunstable_rateid_type grunstable_ratequery_id(const char* name) { } for (int set_idx = 0; set_idx < rate_q::rate_registry_.len; set_idx++) { - const rate_q::rateprop_set_ cur_set = rate_q::rate_registry_.sets[set_idx]; + const rate_q::Descr_set_ cur_set = rate_q::rate_registry_.sets[set_idx]; for (int i = 0; i < cur_set.len; i++) { - rate_q::rateprop_ prop = cur_set.fn(nullptr, i); - if (std::strcmp(name, prop.name) == 0) { + rate_q::Descr descr = cur_set.fn(nullptr, i); + if (std::strcmp(name, descr.name) == 0) { return cur_set.id_offset + i; } } @@ -232,7 +231,7 @@ extern "C" double* grunstable_ratequery_get_ptr( chemistry_data_storage* my_rates, grunstable_rateid_type rate_id) { namespace rate_q = grackle::impl::ratequery; - return rate_q::query_rateprop_(my_rates, rate_id, true).prop.data; + return rate_q::query_Descr(my_rates, rate_id, true).descr.data; } extern "C" const char* grunstable_ith_rate( @@ -241,9 +240,9 @@ extern "C" const char* grunstable_ith_rate( const long long sanitized_i = (i < LLONG_MAX) ? (long long)i : -1; rate_q::ratequery_rslt_ tmp = - rate_q::query_rateprop_(nullptr, sanitized_i, false); + rate_q::query_Descr(nullptr, sanitized_i, false); if (out_rate_id != nullptr) { *out_rate_id = tmp.rate_id; } - return tmp.prop.name; + return tmp.descr.name; } From 9a32a9e121f9c44bae3e270f42597c0d7767edcd Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 11 Dec 2025 21:47:41 -0500 Subject: [PATCH 16/40] slightly more refactoring --- src/clib/rate_utils.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/clib/rate_utils.cpp b/src/clib/rate_utils.cpp index 67ceee203..1d075b3a0 100644 --- a/src/clib/rate_utils.cpp +++ b/src/clib/rate_utils.cpp @@ -156,17 +156,17 @@ static Descr get_MiscRxn_Descr(chemistry_data_storage* my_rates, int i) { #define DESCR_SET_COUNT 2 typedef Descr fetch_Descr_fn(chemistry_data_storage*, int); -struct Descr_set_ { +struct DescrSet { int id_offset; int len; fetch_Descr_fn* fn; }; -struct rate_registry_type_ { +struct DescrRegistry { int len; - Descr_set_ sets[DESCR_SET_COUNT]; + DescrSet sets[DESCR_SET_COUNT]; }; -static const struct rate_registry_type_ rate_registry_ = { +static const struct DescrRegistry descr_registry_ = { /* len: */ DESCR_SET_COUNT, /* sets: */ { {1000, CollisionalRxnLUT::NUM_ENTRIES, &get_CollisionalRxn_Descr}, @@ -187,8 +187,8 @@ static struct ratequery_rslt_ query_Descr(chemistry_data_storage* my_rates, long long i, bool use_rate_id) { int total_len = 0; // <- we increment this as we go through the rates - for (int set_idx = 0; set_idx < rate_registry_.len; set_idx++) { - const struct Descr_set_ cur_set = rate_registry_.sets[set_idx]; + for (int set_idx = 0; set_idx < descr_registry_.len; set_idx++) { + const struct DescrSet cur_set = descr_registry_.sets[set_idx]; const long long tmp = (use_rate_id) ? i - cur_set.id_offset : i - total_len; if ((tmp >= 0) && (tmp < cur_set.len)) { @@ -215,8 +215,8 @@ extern "C" grunstable_rateid_type grunstable_ratequery_id(const char* name) { return rate_q::UNDEFINED_RATE_ID_; } - for (int set_idx = 0; set_idx < rate_q::rate_registry_.len; set_idx++) { - const rate_q::Descr_set_ cur_set = rate_q::rate_registry_.sets[set_idx]; + for (int set_idx = 0; set_idx < rate_q::descr_registry_.len; set_idx++) { + const rate_q::DescrSet cur_set = rate_q::descr_registry_.sets[set_idx]; for (int i = 0; i < cur_set.len; i++) { rate_q::Descr descr = cur_set.fn(nullptr, i); if (std::strcmp(name, descr.name) == 0) { From d13591ea98784a627e8f44c326268cfb57652c90 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 11 Dec 2025 22:13:40 -0500 Subject: [PATCH 17/40] tweak signatures of grunstable_(ratequery_id|ith_rate) --- src/clib/rate_utils.cpp | 6 ++-- src/include/grackle.h | 9 ++++-- src/python/gracklepy/grackle_defs.pxd | 5 +++- src/python/gracklepy/grackle_wrapper.pyx | 37 ++++++++++++++---------- tests/grtestutils/iterator_adaptor.hpp | 4 +-- tests/grtestutils/preset.hpp | 8 +++++ tests/unit/test_api_ratequery.cpp | 12 ++++---- 7 files changed, 53 insertions(+), 28 deletions(-) diff --git a/src/clib/rate_utils.cpp b/src/clib/rate_utils.cpp index 1d075b3a0..ce28a5be7 100644 --- a/src/clib/rate_utils.cpp +++ b/src/clib/rate_utils.cpp @@ -208,7 +208,8 @@ static struct ratequery_rslt_ query_Descr(chemistry_data_storage* my_rates, // here we implement the public API // -------------------------------- -extern "C" grunstable_rateid_type grunstable_ratequery_id(const char* name) { +extern "C" grunstable_rateid_type grunstable_ratequery_id( + const chemistry_data_storage* my_rates, const char* name) { namespace rate_q = grackle::impl::ratequery; if (name == nullptr) { @@ -235,7 +236,8 @@ extern "C" double* grunstable_ratequery_get_ptr( } extern "C" const char* grunstable_ith_rate( - unsigned long long i, grunstable_rateid_type* out_rate_id) { + const chemistry_data_storage* my_rates, unsigned long long i, + grunstable_rateid_type* out_rate_id) { namespace rate_q = grackle::impl::ratequery; const long long sanitized_i = (i < LLONG_MAX) ? (long long)i : -1; diff --git a/src/include/grackle.h b/src/include/grackle.h index 0473ebd0e..84b63c51d 100644 --- a/src/include/grackle.h +++ b/src/include/grackle.h @@ -231,7 +231,8 @@ typedef long long grunstable_rateid_type; /// > [!note] /// > While this is unstable, not all rates may be known yet (but, the number /// > rates are all accessible). -grunstable_rateid_type grunstable_ratequery_id(const char* name); +grunstable_rateid_type grunstable_ratequery_id( + const chemistry_data_storage* my_rates, const char* name); /// Access the pointer associated with the rateid from myrates /// @@ -270,14 +271,16 @@ double* grunstable_ratequery_get_ptr( /// > [!warning] /// > The order of parameters may change between different versions of Grackle /// -/// @param[in] i the index of the access rate +/// @param[in] my_rates The object being queried +/// @param[in] i the index of the accessed rate /// @param[out] out_rate_id A pointer to store the rate of the queried rate_id. /// The behavior is **NOT** currently well defined when there are `i` or /// fewer registered rates. /// @result Pointer to the string-literal specifying the rate's name. This is /// `NULL`, if there are `i` or fewer registered rates. const char* grunstable_ith_rate( - unsigned long long i, grunstable_rateid_type* out_rate_id + const chemistry_data_storage* my_rates, unsigned long long i, + grunstable_rateid_type* out_rate_id ); /** @}*/ // end of group diff --git a/src/python/gracklepy/grackle_defs.pxd b/src/python/gracklepy/grackle_defs.pxd index 7ac1eecd2..462b83503 100644 --- a/src/python/gracklepy/grackle_defs.pxd +++ b/src/python/gracklepy/grackle_defs.pxd @@ -298,12 +298,15 @@ cdef extern from "grackle.h": # the unstable API ctypedef long long grunstable_rateid_type - grunstable_rateid_type grunstable_ratequery_id(const char* name) + grunstable_rateid_type grunstable_ratequery_id( + const c_chemistry_data_storage* my_rates, + const char* name) double* grunstable_ratequery_get_ptr( c_chemistry_data_storage* my_rates, grunstable_rateid_type rate_id) const char* grunstable_ith_rate( + const c_chemistry_data_storage* my_rates, unsigned long long i, grunstable_rateid_type* out_rate_id) diff --git a/src/python/gracklepy/grackle_wrapper.pyx b/src/python/gracklepy/grackle_wrapper.pyx index d8d827f88..320fa1ea1 100644 --- a/src/python/gracklepy/grackle_wrapper.pyx +++ b/src/python/gracklepy/grackle_wrapper.pyx @@ -897,18 +897,6 @@ def _get_rate_shape(wrapped_chemistry_data_obj, rate_name): "the shape of the rate {rate_name!r} has not been specified yet" ) -@functools.lru_cache # (could use functools.cache starting in python3.9) -def _name_rateid_map(): - cdef dict out = {} - cdef const char* rate_name - cdef grunstable_rateid_type rate_id - cdef unsigned long long i = 0 - while True: - rate_name = grunstable_ith_rate(i, &rate_id) - if rate_name is NULL: - return out - out[rate_name.decode('UTF-8')] = int(rate_id) - i+=1 cdef class _rate_mapping_access: # This class is used internally by the chemistry_data extension class to @@ -925,10 +913,12 @@ cdef class _rate_mapping_access: cdef c_chemistry_data_storage *_ptr cdef object _rate_shape_callback + cdef dict _cached_name_rateid_map def __cinit__(self): self._ptr = NULL self._rate_shape_callback = None + self._cached_name_rateid_map = None def __init__(self): # Prevent accidental instantiation from normal Python code @@ -945,11 +935,28 @@ cdef class _rate_mapping_access: out._rate_shape_callback = callback return out + @property + def _name_rateid_map(self): + if self._cached_name_rateid_map is not None: + return self._cached_name_rateid_map + + cdef dict out = {} + cdef const char* rate_name + cdef grunstable_rateid_type rate_id + cdef unsigned long long i = 0 + while True: + rate_name = grunstable_ith_rate(self._ptr, i, &rate_id) + if rate_name is NULL: + self._cached_name_rateid_map = out + return out + out[rate_name.decode('UTF-8')] = int(rate_id) + i+=1 + def _access_rate(self, key, val): # determine whether the rate needs to be updated update_rate = (val is not _NOSETVAL) - rate_id = _name_rateid_map()[key] # will raise a KeyError if not known + rate_id = self._name_rateid_map[key] # will raise a KeyError if not known if self._ptr is NULL: raise RuntimeError( @@ -991,9 +998,9 @@ cdef class _rate_mapping_access: def __setitem__(self, key, value): self._access_rate(key, value) - def __iter__(self): return iter(_name_rateid_map()) + def __iter__(self): return iter(self._name_rateid_map) - def __len__(self): return len(_name_rateid_map()) + def __len__(self): return len(self._name_rateid_map) diff --git a/tests/grtestutils/iterator_adaptor.hpp b/tests/grtestutils/iterator_adaptor.hpp index ad84a3559..af5f9f259 100644 --- a/tests/grtestutils/iterator_adaptor.hpp +++ b/tests/grtestutils/iterator_adaptor.hpp @@ -100,7 +100,7 @@ struct RateQueryPlugin { NameIdPair operator()(unsigned long long i) const { grunstable_rateid_type tmp; - const char* name = grunstable_ith_rate(i, &tmp); + const char* name = grunstable_ith_rate(my_rates, i, &tmp); return NameIdPair{name, tmp}; } @@ -114,7 +114,7 @@ inline unsigned long long grunstable_ratequery_nrates( const chemistry_data_storage* my_rates) { // current implementation is stupid! (in future, will use my_rates) unsigned long long i = 0; - while (nullptr != grunstable_ith_rate(i, nullptr)) { + while (nullptr != grunstable_ith_rate(my_rates, i, nullptr)) { i++; } return i; diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp index 6f942167d..738a79dad 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -108,8 +108,16 @@ class GrackleCtxPack { // getter functions const code_units& initial_units() const { return this->initial_units_; } + chemistry_data* my_chemistry() { return this->my_chemistry_.get(); } + const chemistry_data* my_chemistry() const { + return this->my_chemistry_.get(); + } + chemistry_data_storage* my_rates() { return this->my_rates_.get(); } + const chemistry_data_storage* my_rates() const { + return this->my_rates_.get(); + } /// create an initialized instance from a preset static GrackleCtxPack create(const FullConfPreset& preset, diff --git a/tests/unit/test_api_ratequery.cpp b/tests/unit/test_api_ratequery.cpp index 02d0882bd..cf9c5f52e 100644 --- a/tests/unit/test_api_ratequery.cpp +++ b/tests/unit/test_api_ratequery.cpp @@ -23,7 +23,7 @@ /// returns the rateid used to denote invalid rate names grunstable_rateid_type get_invalid_rateid(const grtest::GrackleCtxPack& pack) { // although we don't use pack, yet a forthcoming refactor requires it - return grunstable_ratequery_id(nullptr); + return grunstable_ratequery_id(pack.my_rates(), nullptr); } using SimpleRateQueryTest = @@ -32,17 +32,18 @@ using SimpleRateQueryTest = TEST_F(SimpleRateQueryTest, InvalidIthRate) { const char* name = grunstable_ith_rate( - std::numeric_limits::max(), nullptr); + pack.my_rates(), std::numeric_limits::max(), nullptr); EXPECT_EQ(name, nullptr); } TEST_F(SimpleRateQueryTest, EmptyNameQuery) { - grunstable_rateid_type rateid = grunstable_ratequery_id(""); + grunstable_rateid_type rateid = grunstable_ratequery_id(pack.my_rates(), ""); EXPECT_EQ(rateid, get_invalid_rateid(pack)); } TEST_F(SimpleRateQueryTest, InvalidNameQuery) { - grunstable_rateid_type rateid = grunstable_ratequery_id("NotAValidName"); + grunstable_rateid_type rateid = + grunstable_ratequery_id(pack.my_rates(), "NotAValidName"); EXPECT_EQ(rateid, get_invalid_rateid(pack)); } @@ -72,7 +73,8 @@ TEST_P(ParametrizedRateQueryTest, AllUnique) { TEST_P(ParametrizedRateQueryTest, ConsistentIDs) { for (const grtest::NameIdPair pair : grtest::RateQueryRange(pack)) { - grunstable_rateid_type rateid = grunstable_ratequery_id(pair.name.c_str()); + grunstable_rateid_type rateid = + grunstable_ratequery_id(pack.my_rates(), pair.name.c_str()); EXPECT_EQ(rateid, pair.id); } } From 1fff13c7552459502544a12eb4c0c5a817bb47fb Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 12 Dec 2025 08:24:24 -0500 Subject: [PATCH 18/40] rename rate_utils to ratequery --- src/clib/CMakeLists.txt | 2 +- src/clib/Make.config.objects | 2 +- src/clib/{rate_utils.cpp => ratequery.cpp} | 0 3 files changed, 2 insertions(+), 2 deletions(-) rename src/clib/{rate_utils.cpp => ratequery.cpp} (100%) diff --git a/src/clib/CMakeLists.txt b/src/clib/CMakeLists.txt index 693d6ea37..9d417790b 100644 --- a/src/clib/CMakeLists.txt +++ b/src/clib/CMakeLists.txt @@ -120,7 +120,7 @@ add_library(Grackle_Grackle lookup_cool_rates1d.hpp make_consistent.cpp make_consistent.hpp opaque_storage.hpp - rate_utils.cpp + ratequery.cpp solve_chemistry.cpp scale_fields.cpp scale_fields.hpp solve_rate_cool_g-cpp.cpp solve_rate_cool_g-cpp.h diff --git a/src/clib/Make.config.objects b/src/clib/Make.config.objects index 7a4bb87af..d3a721cbd 100644 --- a/src/clib/Make.config.objects +++ b/src/clib/Make.config.objects @@ -49,7 +49,7 @@ OBJS_CONFIG_LIB = \ calc_tdust_3d.lo \ calc_grain_size_increment_1d.lo \ rate_functions.lo \ - rate_utils.lo \ + ratequery.lo \ gaussj_g.lo \ utils.lo \ utils-cpp.lo \ diff --git a/src/clib/rate_utils.cpp b/src/clib/ratequery.cpp similarity index 100% rename from src/clib/rate_utils.cpp rename to src/clib/ratequery.cpp From 3659a3abad0d577c43c5d380bde785e75f101a84 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 12 Dec 2025 09:54:24 -0500 Subject: [PATCH 19/40] rename Descr->Entry --- src/clib/ratequery.cpp | 107 +++++++++++++++++++++-------------------- 1 file changed, 56 insertions(+), 51 deletions(-) diff --git a/src/clib/ratequery.cpp b/src/clib/ratequery.cpp index ce28a5be7..e0eb0fec3 100644 --- a/src/clib/ratequery.cpp +++ b/src/clib/ratequery.cpp @@ -6,7 +6,7 @@ //===----------------------------------------------------------------------===// /// /// @file -/// Defines functions that perform basic utilities related to rate data. +/// Defines functionality for querying rate data. /// //===----------------------------------------------------------------------===// @@ -45,36 +45,36 @@ enum { UNDEFINED_RATE_ID_ = 0 }; // introduce some basic machinery to help us implement dynamic lookup of rates -/// A description of a rate -struct Descr { +/// Description of a queryable entity +struct Entry { double* data; const char* name; }; -static inline Descr mk_Descr(double* rate, const char* name) { - Descr out; +static inline Entry mk_Entry(double* rate, const char* name) { + Entry out; out.data = rate; out.name = name; return out; } -static inline Descr mk_Descr_standard_kcol_(chemistry_data_storage* my_rates, +static inline Entry mk_Entry_standard_kcol_(chemistry_data_storage* my_rates, const char* name, int index) { if ((my_rates == nullptr) || (my_rates->opaque_storage == nullptr) || (my_rates->opaque_storage->kcol_rate_tables == nullptr)) { - return mk_Descr(nullptr, name); + return mk_Entry(nullptr, name); } else { - return mk_Descr(my_rates->opaque_storage->kcol_rate_tables->data[index], + return mk_Entry(my_rates->opaque_storage->kcol_rate_tables->data[index], name); } } -#define MKDESCR_(PTR, NAME) \ - mk_Descr(((PTR) == nullptr) ? nullptr : (PTR)->NAME, #NAME) -#define MKDESCR_SCALAR_(PTR, NAME) \ - mk_Descr(((PTR) == nullptr) ? nullptr : &((PTR)->NAME), #NAME) -#define MKDESCR_STANDARD_KCOL_(PTR, NAME, INDEX) \ - mk_Descr_standard_kcol_(PTR, #NAME, INDEX) +#define MKENTRY_(PTR, NAME) \ + mk_Entry(((PTR) == nullptr) ? nullptr : (PTR)->NAME, #NAME) +#define MKENTRY_SCALAR_(PTR, NAME) \ + mk_Entry(((PTR) == nullptr) ? nullptr : &((PTR)->NAME), #NAME) +#define MKENTRY_STANDARD_KCOL_(PTR, NAME, INDEX) \ + mk_Entry_standard_kcol_(PTR, #NAME, INDEX) // Create machinery to lookup Standard-Form Collisional Reaction Rates // ------------------------------------------------------------------- @@ -89,16 +89,16 @@ static inline Descr mk_Descr_standard_kcol_(chemistry_data_storage* my_rates, // of the previous value (if a value isn't explicitly specified) // - CollisionalRxnLUT::NUM_ENTRIES specifies the number of other enumeration // constants (excluding CollisionalRxnLUT::NUM_ENTRIES) in the enum -static Descr get_CollisionalRxn_Descr(chemistry_data_storage* my_rates, int i) { +static Entry get_CollisionalRxn_Entry(chemistry_data_storage* my_rates, int i) { switch (i) { #define ENTRY(NAME) \ case CollisionalRxnLUT::NAME: { \ - return MKDESCR_STANDARD_KCOL_(my_rates, NAME, CollisionalRxnLUT::NAME); \ + return MKENTRY_STANDARD_KCOL_(my_rates, NAME, CollisionalRxnLUT::NAME); \ } #include "collisional_rxn_rate_members.def" #undef ENTRY default: { - Descr out = {nullptr, nullptr}; + Entry out = {nullptr, nullptr}; return out; } } @@ -121,31 +121,31 @@ enum MiscRxnRateKind_ { MiscRxn_NRATES // <- will hold the number of reactions }; -static Descr get_MiscRxn_Descr(chemistry_data_storage* my_rates, int i) { +static Entry get_MiscRxn_Entry(chemistry_data_storage* my_rates, int i) { switch (i) { // density dependent version of k13 (which is a CollisionalRxn) case MiscRxn_k13dd: - return MKDESCR_(my_rates, k13dd); + return MKENTRY_(my_rates, k13dd); // Radiative rates for 6-species (for external field): case MiscRxn_k24: - return MKDESCR_SCALAR_(my_rates, k24); + return MKENTRY_SCALAR_(my_rates, k24); case MiscRxn_k25: - return MKDESCR_SCALAR_(my_rates, k25); + return MKENTRY_SCALAR_(my_rates, k25); case MiscRxn_k26: - return MKDESCR_SCALAR_(my_rates, k26); + return MKENTRY_SCALAR_(my_rates, k26); // Radiative rates for 9-species case MiscRxn_k27: - return MKDESCR_SCALAR_(my_rates, k27); + return MKENTRY_SCALAR_(my_rates, k27); case MiscRxn_k28: - return MKDESCR_SCALAR_(my_rates, k28); + return MKENTRY_SCALAR_(my_rates, k28); case MiscRxn_k29: - return MKDESCR_SCALAR_(my_rates, k29); + return MKENTRY_SCALAR_(my_rates, k29); case MiscRxn_k30: - return MKDESCR_SCALAR_(my_rates, k30); + return MKENTRY_SCALAR_(my_rates, k30); case MiscRxn_k31: - return MKDESCR_SCALAR_(my_rates, k31); + return MKENTRY_SCALAR_(my_rates, k31); default: { - Descr out = {nullptr, nullptr}; + Entry out = {nullptr, nullptr}; return out; } } @@ -154,27 +154,32 @@ static Descr get_MiscRxn_Descr(chemistry_data_storage* my_rates, int i) { // define some additional generic machinery // ---------------------------------------- -#define DESCR_SET_COUNT 2 -typedef Descr fetch_Descr_fn(chemistry_data_storage*, int); -struct DescrSet { +/// describes a recipe for creating 1 or more different entries +/// from a chemistry_data_storage pointer +typedef Entry fetch_Entry_recipe_fn(chemistry_data_storage*, int); + +/// Entryibes a set of rate descriptions that +struct RecipeEntrySet { int id_offset; int len; - fetch_Descr_fn* fn; + fetch_Entry_recipe_fn* fn; }; -struct DescrRegistry { + +#define DESCR_SET_COUNT 2 +struct EntryRegistry { int len; - DescrSet sets[DESCR_SET_COUNT]; + RecipeEntrySet sets[DESCR_SET_COUNT]; }; -static const struct DescrRegistry descr_registry_ = { +static const struct EntryRegistry entry_registry_ = { /* len: */ DESCR_SET_COUNT, /* sets: */ { - {1000, CollisionalRxnLUT::NUM_ENTRIES, &get_CollisionalRxn_Descr}, - {2000, MiscRxn_NRATES, &get_MiscRxn_Descr}}}; + {1000, CollisionalRxnLUT::NUM_ENTRIES, &get_CollisionalRxn_Entry}, + {2000, MiscRxn_NRATES, &get_MiscRxn_Entry}}}; struct ratequery_rslt_ { grunstable_rateid_type rate_id; - Descr descr; + Entry entry; }; /// internal function to search for the rate description i @@ -183,18 +188,18 @@ struct ratequery_rslt_ { /// look for the ith rate description (we introduce an artificial distinction /// between the 2 cases because we want to reserve the right to be able to /// change the relationship if it becomes convenient in the future) -static struct ratequery_rslt_ query_Descr(chemistry_data_storage* my_rates, +static struct ratequery_rslt_ query_Entry(chemistry_data_storage* my_rates, long long i, bool use_rate_id) { int total_len = 0; // <- we increment this as we go through the rates - for (int set_idx = 0; set_idx < descr_registry_.len; set_idx++) { - const struct DescrSet cur_set = descr_registry_.sets[set_idx]; + for (int set_idx = 0; set_idx < entry_registry_.len; set_idx++) { + const struct RecipeEntrySet cur_set = entry_registry_.sets[set_idx]; const long long tmp = (use_rate_id) ? i - cur_set.id_offset : i - total_len; if ((tmp >= 0) && (tmp < cur_set.len)) { struct ratequery_rslt_ out; out.rate_id = tmp + cur_set.id_offset; - out.descr = cur_set.fn(my_rates, tmp); + out.entry = cur_set.fn(my_rates, tmp); return out; } total_len += cur_set.len; @@ -216,12 +221,12 @@ extern "C" grunstable_rateid_type grunstable_ratequery_id( return rate_q::UNDEFINED_RATE_ID_; } - for (int set_idx = 0; set_idx < rate_q::descr_registry_.len; set_idx++) { - const rate_q::DescrSet cur_set = rate_q::descr_registry_.sets[set_idx]; - for (int i = 0; i < cur_set.len; i++) { - rate_q::Descr descr = cur_set.fn(nullptr, i); - if (std::strcmp(name, descr.name) == 0) { - return cur_set.id_offset + i; + for (int set_idx = 0; set_idx < rate_q::entry_registry_.len; set_idx++) { + const rate_q::RecipeEntrySet set = rate_q::entry_registry_.sets[set_idx]; + for (int i = 0; i < set.len; i++) { + rate_q::Entry entry = set.fn(nullptr, i); + if (std::strcmp(name, entry.name) == 0) { + return set.id_offset + i; } } } @@ -232,7 +237,7 @@ extern "C" double* grunstable_ratequery_get_ptr( chemistry_data_storage* my_rates, grunstable_rateid_type rate_id) { namespace rate_q = grackle::impl::ratequery; - return rate_q::query_Descr(my_rates, rate_id, true).descr.data; + return rate_q::query_Entry(my_rates, rate_id, true).entry.data; } extern "C" const char* grunstable_ith_rate( @@ -242,9 +247,9 @@ extern "C" const char* grunstable_ith_rate( const long long sanitized_i = (i < LLONG_MAX) ? (long long)i : -1; rate_q::ratequery_rslt_ tmp = - rate_q::query_Descr(nullptr, sanitized_i, false); + rate_q::query_Entry(nullptr, sanitized_i, false); if (out_rate_id != nullptr) { *out_rate_id = tmp.rate_id; } - return tmp.descr.name; + return tmp.entry.name; } From 11ddf78b8df50a4b7b60dc49199176a44e461445 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 12 Dec 2025 11:17:28 -0500 Subject: [PATCH 20/40] introduce ratequery.hpp --- src/clib/CMakeLists.txt | 2 +- src/clib/ratequery.cpp | 49 ++++--------------- src/clib/ratequery.hpp | 103 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+), 40 deletions(-) create mode 100644 src/clib/ratequery.hpp diff --git a/src/clib/CMakeLists.txt b/src/clib/CMakeLists.txt index 9d417790b..8bd5db35d 100644 --- a/src/clib/CMakeLists.txt +++ b/src/clib/CMakeLists.txt @@ -120,7 +120,7 @@ add_library(Grackle_Grackle lookup_cool_rates1d.hpp make_consistent.cpp make_consistent.hpp opaque_storage.hpp - ratequery.cpp + ratequery.cpp ratequery.hpp solve_chemistry.cpp scale_fields.cpp scale_fields.hpp solve_rate_cool_g-cpp.cpp solve_rate_cool_g-cpp.h diff --git a/src/clib/ratequery.cpp b/src/clib/ratequery.cpp index e0eb0fec3..70bdc0dfb 100644 --- a/src/clib/ratequery.cpp +++ b/src/clib/ratequery.cpp @@ -16,6 +16,7 @@ #include "internal_types.hpp" // CollisionalRxnRateCollection #include "LUT.hpp" // CollisionalRxnLUT #include "opaque_storage.hpp" // gr_opaque_storage +#include "ratequery.hpp" // In comparison to the dynamic API for accessing elements of chemistry_data, // we have explicitly opted NOT to make use of offsetof to access arbitrary @@ -45,36 +46,23 @@ enum { UNDEFINED_RATE_ID_ = 0 }; // introduce some basic machinery to help us implement dynamic lookup of rates -/// Description of a queryable entity -struct Entry { - double* data; - const char* name; -}; - -static inline Entry mk_Entry(double* rate, const char* name) { - Entry out; - out.data = rate; - out.name = name; - return out; -} - -static inline Entry mk_Entry_standard_kcol_(chemistry_data_storage* my_rates, - const char* name, int index) { +static Entry new_Entry_standard_kcol_(chemistry_data_storage* my_rates, + const char* name, int index) { if ((my_rates == nullptr) || (my_rates->opaque_storage == nullptr) || (my_rates->opaque_storage->kcol_rate_tables == nullptr)) { - return mk_Entry(nullptr, name); + return new_Entry(nullptr, name); } else { - return mk_Entry(my_rates->opaque_storage->kcol_rate_tables->data[index], - name); + return new_Entry(my_rates->opaque_storage->kcol_rate_tables->data[index], + name); } } #define MKENTRY_(PTR, NAME) \ - mk_Entry(((PTR) == nullptr) ? nullptr : (PTR)->NAME, #NAME) + new_Entry(((PTR) == nullptr) ? nullptr : (PTR)->NAME, #NAME) #define MKENTRY_SCALAR_(PTR, NAME) \ - mk_Entry(((PTR) == nullptr) ? nullptr : &((PTR)->NAME), #NAME) + new_Entry(((PTR) == nullptr) ? nullptr : &((PTR)->NAME), #NAME) #define MKENTRY_STANDARD_KCOL_(PTR, NAME, INDEX) \ - mk_Entry_standard_kcol_(PTR, #NAME, INDEX) + new_Entry_standard_kcol_(PTR, #NAME, INDEX) // Create machinery to lookup Standard-Form Collisional Reaction Rates // ------------------------------------------------------------------- @@ -154,25 +142,8 @@ static Entry get_MiscRxn_Entry(chemistry_data_storage* my_rates, int i) { // define some additional generic machinery // ---------------------------------------- -/// describes a recipe for creating 1 or more different entries -/// from a chemistry_data_storage pointer -typedef Entry fetch_Entry_recipe_fn(chemistry_data_storage*, int); - -/// Entryibes a set of rate descriptions that -struct RecipeEntrySet { - int id_offset; - int len; - fetch_Entry_recipe_fn* fn; -}; - -#define DESCR_SET_COUNT 2 -struct EntryRegistry { - int len; - RecipeEntrySet sets[DESCR_SET_COUNT]; -}; - static const struct EntryRegistry entry_registry_ = { - /* len: */ DESCR_SET_COUNT, + /* len: */ ENTRY_SET_COUNT, /* sets: */ { {1000, CollisionalRxnLUT::NUM_ENTRIES, &get_CollisionalRxn_Entry}, {2000, MiscRxn_NRATES, &get_MiscRxn_Entry}}}; diff --git a/src/clib/ratequery.hpp b/src/clib/ratequery.hpp new file mode 100644 index 000000000..0ab72145b --- /dev/null +++ b/src/clib/ratequery.hpp @@ -0,0 +1,103 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Declares functionality for querying rate data +/// +//===----------------------------------------------------------------------===// +#ifndef RATEQUERY_HPP +#define RATEQUERY_HPP + +#include "grackle.h" + +namespace grackle::impl::ratequery { + +/// @defgroup Dynamic Rate Query Machinery +/// +/// This group of entities provides the machinery for implementing the API for +/// dynamically accessing rate data. +/// +/// The design of this machinery is based on a simple model: +/// - the query machinery queries a Registry +/// - you can think of a Registry as a container. Each item is an Entry that +/// describes a unique queryable rate. +/// +/// The actualy implementation is a little more sophisticated. In practice is +/// divided into subsets of `Entry` instances. Moreover, subsets are free to +/// treat `Entry` instances as ephemeral objects (i.e. a subset can lazily +/// construct a new `Entry` instance and is allowed to forget about the +/// instance). +/// +/// Design Considerations +/// ===================== +/// Ideally, this machinery should balance 3 design considerations: (1) memory +/// usage, (2) impact on grackle solver initialization, (3) Query Performance. +/// +/// The relative of importance of these considerations should be informed by +/// the fact that dynamic rate API logic is not central to querying logic is +/// not of central important to Grackle as a library: +/// - currently, none of the rates ever need to queried for regular Grackle +/// usage (i.e. they are only queried for debugging/experimentation) +/// - moreover, if we do need to query some information in certain +/// configurations (e.g. assumed grain species yields for different injection +/// pathways), the data probably isn't in the most useful format for +/// everyone. Even if queries were ultra-fast, a performance-oriented user +/// would only query that information once, repack the data so that it's +/// structured in a more useful way, and cache the repacked data. +/// +/// In principle, if we had an idea for an delivered significantly better +/// performance, at the cost of increased memory usage and/or longer +/// initialization, we could consider making construction of the registry (or +/// non-essential registry-entries) a runtime parameter. +/// +/// ASIDE: The current design is very much prioritizes doing something easy +/// over runtime performance. +/** @{ */ + +/// A queryable entity +struct Entry { + double* data; + const char* name; +}; + +/// Constructs an entry +inline Entry new_Entry(double* rate, const char* name) { + Entry out; + out.data = rate; + out.name = name; + return out; +} + +/// a recipe for querying 1 or more entries from a chemistry_data_storage +/// pointer given an index. A recipe generally lazily creates an Entry as +/// the underlying data is fetched. +/// +/// If `N` denotes the number of entries that can be queried by a given recipe, +/// then this function should produce unique entries for each unique index that +/// satisfies `0 <= index <= (N-1)` +typedef Entry fetch_Entry_recipe_fn(chemistry_data_storage*, int); + +/// Describes the set of entries that can be accessed through a given recipe +struct RecipeEntrySet { + int id_offset; + int len; + fetch_Entry_recipe_fn* fn; +}; + +#define ENTRY_SET_COUNT 2 + +/// Describes a registry of queryable entries +struct EntryRegistry { + int len; + RecipeEntrySet sets[ENTRY_SET_COUNT]; +}; + +/** @}*/ // end of group + +} // namespace grackle::impl::ratequery + +#endif // RATEQUERY_HPP From fb030ffd697b49c1de993e82b6b12a24d03f2945 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 12 Dec 2025 12:05:52 -0500 Subject: [PATCH 21/40] Store the `Entry` Registry within chemistry_data_storage --- src/clib/initialize_chemistry_data.cpp | 16 ++++++ src/clib/opaque_storage.hpp | 4 ++ src/clib/ratequery.cpp | 72 +++++++++++++++++++------- src/clib/ratequery.hpp | 12 +++-- 4 files changed, 81 insertions(+), 23 deletions(-) diff --git a/src/clib/initialize_chemistry_data.cpp b/src/clib/initialize_chemistry_data.cpp index b8b39fabf..8d74713ab 100644 --- a/src/clib/initialize_chemistry_data.cpp +++ b/src/clib/initialize_chemistry_data.cpp @@ -28,6 +28,7 @@ #include "internal_types.hpp" // drop_CollisionalRxnRateCollection #include "opaque_storage.hpp" // gr_opaque_storage #include "phys_constants.h" +#include "ratequery.hpp" #ifdef _OPENMP #include @@ -403,6 +404,7 @@ extern "C" int local_initialize_chemistry_data(chemistry_data *my_chemistry, init_empty_interp_grid_props_( &my_rates->opaque_storage->h2dust_grain_interp_props); my_rates->opaque_storage->grain_species_info = nullptr; + my_rates->opaque_storage->registry = nullptr; double co_length_units, co_density_units; if (my_units->comoving_coordinates == TRUE) { @@ -456,6 +458,11 @@ extern "C" int local_initialize_chemistry_data(chemistry_data *my_chemistry, /* store a copy of the initial units */ my_rates->initial_units = *my_units; + // initialize the registry + my_rates->opaque_storage->registry = new grackle::impl::ratequery::Registry( + grackle::impl::ratequery::new_Registry() + ); + if (grackle_verbose) { time_t timer; char tstr[80]; @@ -659,6 +666,15 @@ extern "C" int local_free_chemistry_data(chemistry_data *my_chemistry, delete my_rates->opaque_storage->grain_species_info; } + if (my_rates->opaque_storage->registry != nullptr) { + // delete contents of registry + grackle::impl::ratequery::drop_Registry( + my_rates->opaque_storage->registry + ); + // delete registry, itself + delete my_rates->opaque_storage->registry; + } + delete my_rates->opaque_storage; my_rates->opaque_storage = nullptr; diff --git a/src/clib/opaque_storage.hpp b/src/clib/opaque_storage.hpp index a118d42c4..e79776094 100644 --- a/src/clib/opaque_storage.hpp +++ b/src/clib/opaque_storage.hpp @@ -16,6 +16,7 @@ #include "grackle.h" #include "dust/grain_species_info.hpp" #include "internal_types.hpp" +#include "ratequery.hpp" /// a struct that used to wrap some private storage details /// @@ -95,6 +96,9 @@ struct gr_opaque_storage { /// > calculations). An alternative would be to briefly initialize an /// > instance during setup and then repack the data. grackle::impl::GrainSpeciesInfo* grain_species_info; + + /// used to implement the experimental ratequery machinery + grackle::impl::ratequery::Registry* registry; }; #endif /* OPAQUE_STORAGE_HPP */ diff --git a/src/clib/ratequery.cpp b/src/clib/ratequery.cpp index 70bdc0dfb..a60ac3519 100644 --- a/src/clib/ratequery.cpp +++ b/src/clib/ratequery.cpp @@ -139,44 +139,76 @@ static Entry get_MiscRxn_Entry(chemistry_data_storage* my_rates, int i) { } } -// define some additional generic machinery -// ---------------------------------------- +} // namespace grackle::impl::ratequery + +grackle::impl::ratequery::Registry grackle::impl::ratequery::new_Registry() { + const RecipeEntrySet standard_sets[] = { + {1000, CollisionalRxnLUT::NUM_ENTRIES, &get_CollisionalRxn_Entry}, + {2000, MiscRxn_NRATES, &get_MiscRxn_Entry}}; + int len = static_cast(sizeof(standard_sets) / sizeof(RecipeEntrySet)); + RecipeEntrySet* sets = new RecipeEntrySet[len]; + for (int i = 0; i < len; i++) { + sets[i] = standard_sets[i]; + } + + return Registry{len, sets}; +} -static const struct EntryRegistry entry_registry_ = { - /* len: */ ENTRY_SET_COUNT, - /* sets: */ { - {1000, CollisionalRxnLUT::NUM_ENTRIES, &get_CollisionalRxn_Entry}, - {2000, MiscRxn_NRATES, &get_MiscRxn_Entry}}}; +void grackle::impl::ratequery::drop_Registry( + grackle::impl::ratequery::Registry* ptr) { + if (ptr->sets != nullptr) { + delete[] ptr->sets; + ptr->sets = nullptr; + } +} + +namespace grackle::impl::ratequery { struct ratequery_rslt_ { grunstable_rateid_type rate_id; Entry entry; }; +static ratequery_rslt_ invalid_rslt_() { + return {UNDEFINED_RATE_ID_, {nullptr, nullptr}}; +} + +static const Registry* get_registry(const chemistry_data_storage* my_rates) { + if ((my_rates == nullptr) || (my_rates->opaque_storage == nullptr)) { + return nullptr; + } + return my_rates->opaque_storage->registry; +} + /// internal function to search for the rate description i /// /// We interpret i as rate_id, when use_rate_id is true. Otherwise, we just /// look for the ith rate description (we introduce an artificial distinction /// between the 2 cases because we want to reserve the right to be able to /// change the relationship if it becomes convenient in the future) -static struct ratequery_rslt_ query_Entry(chemistry_data_storage* my_rates, - long long i, bool use_rate_id) { +static ratequery_rslt_ query_Entry(chemistry_data_storage* my_rates, + long long i, bool use_rate_id) { + const Registry* registry = get_registry(my_rates); + + if (registry == nullptr) { + return invalid_rslt_(); + } + int total_len = 0; // <- we increment this as we go through the rates - for (int set_idx = 0; set_idx < entry_registry_.len; set_idx++) { - const struct RecipeEntrySet cur_set = entry_registry_.sets[set_idx]; + for (int set_idx = 0; set_idx < registry->len; set_idx++) { + const struct RecipeEntrySet cur_set = registry->sets[set_idx]; const long long tmp = (use_rate_id) ? i - cur_set.id_offset : i - total_len; if ((tmp >= 0) && (tmp < cur_set.len)) { - struct ratequery_rslt_ out; + ratequery_rslt_ out; out.rate_id = tmp + cur_set.id_offset; out.entry = cur_set.fn(my_rates, tmp); return out; } total_len += cur_set.len; } - struct ratequery_rslt_ out = {UNDEFINED_RATE_ID_, {nullptr, nullptr}}; - return out; + return invalid_rslt_(); } } // namespace grackle::impl::ratequery @@ -188,12 +220,13 @@ extern "C" grunstable_rateid_type grunstable_ratequery_id( const chemistry_data_storage* my_rates, const char* name) { namespace rate_q = grackle::impl::ratequery; - if (name == nullptr) { + const rate_q::Registry* registry = rate_q::get_registry(my_rates); + if ((name == nullptr) || (registry == nullptr)) { return rate_q::UNDEFINED_RATE_ID_; } - for (int set_idx = 0; set_idx < rate_q::entry_registry_.len; set_idx++) { - const rate_q::RecipeEntrySet set = rate_q::entry_registry_.sets[set_idx]; + for (int set_idx = 0; set_idx < registry->len; set_idx++) { + const rate_q::RecipeEntrySet set = registry->sets[set_idx]; for (int i = 0; i < set.len; i++) { rate_q::Entry entry = set.fn(nullptr, i); if (std::strcmp(name, entry.name) == 0) { @@ -217,8 +250,9 @@ extern "C" const char* grunstable_ith_rate( namespace rate_q = grackle::impl::ratequery; const long long sanitized_i = (i < LLONG_MAX) ? (long long)i : -1; - rate_q::ratequery_rslt_ tmp = - rate_q::query_Entry(nullptr, sanitized_i, false); + // short-term hack! (it's bad practice to "cast away the const") + rate_q::ratequery_rslt_ tmp = rate_q::query_Entry( + const_cast(my_rates), sanitized_i, false); if (out_rate_id != nullptr) { *out_rate_id = tmp.rate_id; } diff --git a/src/clib/ratequery.hpp b/src/clib/ratequery.hpp index 0ab72145b..69cb8e20d 100644 --- a/src/clib/ratequery.hpp +++ b/src/clib/ratequery.hpp @@ -88,14 +88,18 @@ struct RecipeEntrySet { fetch_Entry_recipe_fn* fn; }; -#define ENTRY_SET_COUNT 2 - /// Describes a registry of queryable entries -struct EntryRegistry { +struct Registry { int len; - RecipeEntrySet sets[ENTRY_SET_COUNT]; + RecipeEntrySet* sets; }; +/// construct a new registry +Registry new_Registry(); + +/// deallocate the contents of a registry +void drop_Registry(Registry* ptr); + /** @}*/ // end of group } // namespace grackle::impl::ratequery From e42e0dd75b97461c1eabd227880580e26d46e240 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 12 Dec 2025 15:16:34 -0500 Subject: [PATCH 22/40] lay the foundation for querying properties --- src/clib/initialize_chemistry_data.cpp | 2 +- src/clib/ratequery.cpp | 88 ++++++++++++++++++++++---- src/clib/ratequery.hpp | 29 +++++++-- src/clib/status_reporting.h | 19 ++++++ src/include/grackle.h | 33 ++++++++++ 5 files changed, 151 insertions(+), 20 deletions(-) diff --git a/src/clib/initialize_chemistry_data.cpp b/src/clib/initialize_chemistry_data.cpp index 8d74713ab..95900c440 100644 --- a/src/clib/initialize_chemistry_data.cpp +++ b/src/clib/initialize_chemistry_data.cpp @@ -460,7 +460,7 @@ extern "C" int local_initialize_chemistry_data(chemistry_data *my_chemistry, // initialize the registry my_rates->opaque_storage->registry = new grackle::impl::ratequery::Registry( - grackle::impl::ratequery::new_Registry() + grackle::impl::ratequery::new_Registry(*my_chemistry) ); if (grackle_verbose) { diff --git a/src/clib/ratequery.cpp b/src/clib/ratequery.cpp index a60ac3519..be4c582d7 100644 --- a/src/clib/ratequery.cpp +++ b/src/clib/ratequery.cpp @@ -17,6 +17,7 @@ #include "LUT.hpp" // CollisionalRxnLUT #include "opaque_storage.hpp" // gr_opaque_storage #include "ratequery.hpp" +#include "status_reporting.h" // In comparison to the dynamic API for accessing elements of chemistry_data, // we have explicitly opted NOT to make use of offsetof to access arbitrary @@ -57,8 +58,8 @@ static Entry new_Entry_standard_kcol_(chemistry_data_storage* my_rates, } } -#define MKENTRY_(PTR, NAME) \ - new_Entry(((PTR) == nullptr) ? nullptr : (PTR)->NAME, #NAME) +#define MKENTRY_(PTR, NAME, FAMILY) \ + new_Entry(((PTR) == nullptr) ? nullptr : (PTR)->NAME, #NAME, FAMILY) #define MKENTRY_SCALAR_(PTR, NAME) \ new_Entry(((PTR) == nullptr) ? nullptr : &((PTR)->NAME), #NAME) #define MKENTRY_STANDARD_KCOL_(PTR, NAME, INDEX) \ @@ -86,17 +87,24 @@ static Entry get_CollisionalRxn_Entry(chemistry_data_storage* my_rates, int i) { #include "collisional_rxn_rate_members.def" #undef ENTRY default: { - Entry out = {nullptr, nullptr}; - return out; + return mk_invalid_Entry(); } } } +static Entry get_k13dd_Entry(chemistry_data_storage* my_rates, int i) { + if (i == 0) { + double* ptr = (my_rates == nullptr) ? nullptr : my_rates->k13dd; + return new_Entry(ptr, "k13dd"); + } else { + return mk_invalid_Entry(); + } +} + // Create machinery to lookup Other Miscellaneous Rates // ---------------------------------------------------- enum MiscRxnRateKind_ { - MiscRxn_k13dd, MiscRxn_k24, MiscRxn_k25, MiscRxn_k26, @@ -111,9 +119,6 @@ enum MiscRxnRateKind_ { static Entry get_MiscRxn_Entry(chemistry_data_storage* my_rates, int i) { switch (i) { - // density dependent version of k13 (which is a CollisionalRxn) - case MiscRxn_k13dd: - return MKENTRY_(my_rates, k13dd); // Radiative rates for 6-species (for external field): case MiscRxn_k24: return MKENTRY_SCALAR_(my_rates, k24); @@ -133,18 +138,33 @@ static Entry get_MiscRxn_Entry(chemistry_data_storage* my_rates, int i) { case MiscRxn_k31: return MKENTRY_SCALAR_(my_rates, k31); default: { - Entry out = {nullptr, nullptr}; - return out; + return mk_invalid_Entry(); } } } } // namespace grackle::impl::ratequery -grackle::impl::ratequery::Registry grackle::impl::ratequery::new_Registry() { +grackle::impl::ratequery::Registry grackle::impl::ratequery::new_Registry( + const chemistry_data& my_chemistry) { + EntryProps props_LogTLinInterp = mk_invalid_EntryProps(); + props_LogTLinInterp.ndim = 1; + props_LogTLinInterp.shape[0] = my_chemistry.NumberOfTemperatureBins; + + // maybe k13dd should be considered multi-dimensional? + EntryProps props_k13dd = mk_invalid_EntryProps(); + props_k13dd.ndim = 1; + props_k13dd.shape[0] = my_chemistry.NumberOfTemperatureBins * 14; + + EntryProps props_scalar = mk_invalid_EntryProps(); + props_scalar.ndim = 0; + const RecipeEntrySet standard_sets[] = { - {1000, CollisionalRxnLUT::NUM_ENTRIES, &get_CollisionalRxn_Entry}, - {2000, MiscRxn_NRATES, &get_MiscRxn_Entry}}; + {1000, CollisionalRxnLUT::NUM_ENTRIES, &get_CollisionalRxn_Entry, + props_LogTLinInterp}, + {1999, 1, &get_k13dd_Entry, props_k13dd}, + {2000, MiscRxn_NRATES, &get_MiscRxn_Entry, props_scalar}}; + int len = static_cast(sizeof(standard_sets) / sizeof(RecipeEntrySet)); RecipeEntrySet* sets = new RecipeEntrySet[len]; for (int i = 0; i < len; i++) { @@ -204,6 +224,7 @@ static ratequery_rslt_ query_Entry(chemistry_data_storage* my_rates, ratequery_rslt_ out; out.rate_id = tmp + cur_set.id_offset; out.entry = cur_set.fn(my_rates, tmp); + out.entry.props = cur_set.common_props; return out; } total_len += cur_set.len; @@ -244,6 +265,47 @@ extern "C" double* grunstable_ratequery_get_ptr( return rate_q::query_Entry(my_rates, rate_id, true).entry.data; } +extern "C" int grunstable_ratequery_prop( + const chemistry_data_storage* my_rates, grunstable_rateid_type rate_id, + enum grunstable_ratequery_prop_kind prop_kind, long long* ptr) { + namespace rate_q = grackle::impl::ratequery; + + // short-term hack! (it's bad practice to "cast away the const") + rate_q::Entry entry = + rate_q::query_Entry(const_cast(my_rates), + rate_id, true) + .entry; + + const rate_q::EntryProps& props = entry.props; + if ((entry.name == nullptr) || !rate_q::EntryProps_is_valid(props)) { + return GR_FAIL; + } + + switch (prop_kind) { + case GRUNSTABLE_QPROP_NDIM: { + *ptr = static_cast(props.ndim); + return GR_SUCCESS; + } + case GRUNSTABLE_QPROP_SHAPE: { + for (int i = 0; i < props.ndim; i++) { + ptr[i] = static_cast(props.shape[i]); + } + return GR_SUCCESS; + } + case GRUNSTABLE_QPROP_TYPE: + return GR_FAIL; + case GRUNSTABLE_QPROP_MAXITEMSIZE: { + *ptr = static_cast(sizeof(double)); + return GR_SUCCESS; + } + case GRUNSTABLE_QPROP_WRITABLE: { + *ptr = 1LL; + } + default: + GR_INTERNAL_UNREACHABLE_ERROR(); + } +} + extern "C" const char* grunstable_ith_rate( const chemistry_data_storage* my_rates, unsigned long long i, grunstable_rateid_type* out_rate_id) { diff --git a/src/clib/ratequery.hpp b/src/clib/ratequery.hpp index 69cb8e20d..55b01c27f 100644 --- a/src/clib/ratequery.hpp +++ b/src/clib/ratequery.hpp @@ -58,18 +58,34 @@ namespace grackle::impl::ratequery { /// over runtime performance. /** @{ */ +/// Describes properties about the data in an entry +struct EntryProps { + int ndim; + int shape[GRACKLE_CLOUDY_TABLE_MAX_DIMENSION]; +}; + +inline EntryProps mk_invalid_EntryProps() { + EntryProps out; + out.ndim = -1; + return out; +} + +inline bool EntryProps_is_valid(EntryProps obj) { return obj.ndim >= 0; } + /// A queryable entity struct Entry { double* data; const char* name; + EntryProps props; }; -/// Constructs an entry +inline Entry mk_invalid_Entry() { + return Entry{nullptr, nullptr, mk_invalid_EntryProps()}; +} + +/// Constructs an Entry inline Entry new_Entry(double* rate, const char* name) { - Entry out; - out.data = rate; - out.name = name; - return out; + return Entry{rate, name, mk_invalid_EntryProps()}; } /// a recipe for querying 1 or more entries from a chemistry_data_storage @@ -86,6 +102,7 @@ struct RecipeEntrySet { int id_offset; int len; fetch_Entry_recipe_fn* fn; + EntryProps common_props; }; /// Describes a registry of queryable entries @@ -95,7 +112,7 @@ struct Registry { }; /// construct a new registry -Registry new_Registry(); +Registry new_Registry(const chemistry_data&); /// deallocate the contents of a registry void drop_Registry(Registry* ptr); diff --git a/src/clib/status_reporting.h b/src/clib/status_reporting.h index fe1a1cd5b..60373d4b0 100644 --- a/src/clib/status_reporting.h +++ b/src/clib/status_reporting.h @@ -230,6 +230,7 @@ ERRFMT_ATTR_(2) NORETURN_ATTR_ void grimpl_abort_with_internal_err_( #define GRIMPL_ERROR(...) \ { grimpl_abort_with_internal_err_(__GRIMPL_SRCLOC__, __VA_ARGS__); } + /// @def GR_INTERNAL_REQUIRE /// @brief implements functionality analogous to the assert() macro /// @@ -255,6 +256,24 @@ ERRFMT_ATTR_(2) NORETURN_ATTR_ void grimpl_abort_with_internal_err_( { if (!(cond)) \ { grimpl_abort_with_internal_err_(__GRIMPL_SRCLOC__, __VA_ARGS__); } } + +/// @def GR_INTERNAL_UNREACHABLE_ERROR() +/// @brief function-like macro that aborts with a (lethal) error message +/// indicating that +/// +/// This macro should be treated as a function with the signature: +/// +/// [[noreturn]] void GR_INTERNAL_UNREACHABLE_ERROR(); +/// +/// @note +/// Unlike gcc/clang's __builtin_unreachable or C++23's std::unreachable, this +/// aborts the program with an error if its executed (the other cases produce +/// undefined behavior). (An argument could be made for conditionally compiling +/// this macro into the alternatives to test speed) +#define GR_INTERNAL_UNREACHABLE_ERROR() \ + { grimpl_abort_with_internal_err_(__GRIMPL_SRCLOC__, \ + "location shouldn't be reachable"); } + // helper function ERRFMT_ATTR_(2) NODISCARD_ATTR_ int grimpl_print_and_return_err_( const struct grimpl_source_location_ locinfo, const char* msg, ... diff --git a/src/include/grackle.h b/src/include/grackle.h index 84b63c51d..ab6e03e01 100644 --- a/src/include/grackle.h +++ b/src/include/grackle.h @@ -266,6 +266,39 @@ double* grunstable_ratequery_get_ptr( chemistry_data_storage* my_rates, grunstable_rateid_type rate_id ); +/// Describe Rate-Query Property Types +/// +/// > [!note] +/// > It may make more sense to use macros if we want to support these from +/// > Fortran +/// +/// > [!important] +/// > Users should obviously avoid hardcoding values in their codebase. +enum grunstable_ratequery_prop_kind { + GRUNSTABLE_QPROP_NDIM = 1, + GRUNSTABLE_QPROP_SHAPE = 2, + GRUNSTABLE_QPROP_TYPE = 3, + GRUNSTABLE_QPROP_MAXITEMSIZE = 4, + // I don't like the next one + GRUNSTABLE_QPROP_WRITABLE = 5, +}; + +/// Query a property of the specified rate +/// +/// @param[in] my_rates The object being queried +/// @param[in] rate_id The id of the rate for which the property is queried +/// @param[in] prop_kind The proprty to query +/// @param[out] ptr The pointer where the property is recorded +/// +/// @returns GR_SUCCESS if successful. Otherwise, a different value is returned. +/// +/// The behavior is undefined when @p my_rates is a `nullptr`, @p ptr is a +/// nullptr or @p ptr doesn't have enough space to store the queried property +int grunstable_ratequery_prop(const chemistry_data_storage* my_rates, + grunstable_rateid_type rate_id, + enum grunstable_ratequery_prop_kind prop_kind, + long long* ptr); + /// Query the name (and optionally the rate_id) of the ith registered rate /// /// > [!warning] From 97d3ce621ccf234e1f1b49e274cf17bde8cb4674 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 12 Dec 2025 16:26:17 -0500 Subject: [PATCH 23/40] gracklepy: start directly querying shapes --- src/python/gracklepy/grackle_defs.pxd | 14 +++++++++++ src/python/gracklepy/grackle_wrapper.pyx | 31 +++++++++++++++++++++++- 2 files changed, 44 insertions(+), 1 deletion(-) diff --git a/src/python/gracklepy/grackle_defs.pxd b/src/python/gracklepy/grackle_defs.pxd index 462b83503..a8e6b761c 100644 --- a/src/python/gracklepy/grackle_defs.pxd +++ b/src/python/gracklepy/grackle_defs.pxd @@ -212,6 +212,7 @@ cdef extern from "grackle.h": # defined in "grackle.h" # ---------------------- + cdef int GR_SUCCESS cdef int GRACKLE_FAIL_VALUE "GR_FAIL" cdef int GR_SPECIFY_INITIAL_A_VALUE @@ -306,6 +307,19 @@ cdef extern from "grackle.h": c_chemistry_data_storage* my_rates, grunstable_rateid_type rate_id) + cdef enum grunstable_ratequery_prop_kind: + GRUNSTABLE_QPROP_NDIM + GRUNSTABLE_QPROP_SHAPE + GRUNSTABLE_QPROP_TYPE + GRUNSTABLE_QPROP_MAXITEMSIZE + GRUNSTABLE_QPROP_WRITABLE + + int grunstable_ratequery_prop( + const c_chemistry_data_storage* my_rates, + grunstable_rateid_type rate_id, + grunstable_ratequery_prop_kind prop_kind, + long long* ptr) + const char* grunstable_ith_rate( const c_chemistry_data_storage* my_rates, unsigned long long i, diff --git a/src/python/gracklepy/grackle_wrapper.pyx b/src/python/gracklepy/grackle_wrapper.pyx index 320fa1ea1..d1b872a62 100644 --- a/src/python/gracklepy/grackle_wrapper.pyx +++ b/src/python/gracklepy/grackle_wrapper.pyx @@ -952,11 +952,35 @@ cdef class _rate_mapping_access: out[rate_name.decode('UTF-8')] = int(rate_id) i+=1 + def _try_get_shape(self, rate_id): + cdef long long buf[7] + cdef int ret = grunstable_ratequery_prop( + self._ptr, rate_id, GRUNSTABLE_QPROP_NDIM, &buf[0] + ) + if ret != GR_SUCCESS: + return None + ndim = int(buf[0]) + if ndim == 0: + return () + elif ndim >7: + tmp = int(ndim) + raise RuntimeError( + f"rate_id {rate_id} has a questionable number of dims: {tmp}" + ) + + ret = grunstable_ratequery_prop( + self._ptr, rate_id, GRUNSTABLE_QPROP_SHAPE, &buf[0] + ) + if ret != GR_SUCCESS: + raise RuntimeError( + "the query for shape failed after query for ndim succeeded") + return tuple(int(buf[i]) for i in range(ndim)) + def _access_rate(self, key, val): # determine whether the rate needs to be updated update_rate = (val is not _NOSETVAL) - rate_id = self._name_rateid_map[key] # will raise a KeyError if not known + rate_id = self._name_rateid_map[key] # will raise KeyError if not known if self._ptr is NULL: raise RuntimeError( @@ -967,10 +991,15 @@ cdef class _rate_mapping_access: # retrieve the pointer cdef double* rate_ptr = grunstable_ratequery_get_ptr(self._ptr, rate_id) + # experimental shape retrieval + exp_shape = self._try_get_shape(rate_id) + # lookup the shape of the rates callback = self._rate_shape_callback shape = callback(rate_name=key) + assert shape == exp_shape + # predeclare a memoryview to use with 1d arrays cdef double[:] memview From ab816b4aea72eb21141342f7d6bb3d1120c79f7c Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 12 Dec 2025 16:35:37 -0500 Subject: [PATCH 24/40] gracklepy: use new shape query logic --- src/python/gracklepy/grackle_wrapper.pyx | 49 ++---------------------- 1 file changed, 3 insertions(+), 46 deletions(-) diff --git a/src/python/gracklepy/grackle_wrapper.pyx b/src/python/gracklepy/grackle_wrapper.pyx index d1b872a62..46ccf3e67 100644 --- a/src/python/gracklepy/grackle_wrapper.pyx +++ b/src/python/gracklepy/grackle_wrapper.pyx @@ -12,7 +12,6 @@ ######################################################################## import copy -import functools from gracklepy.utilities.physical_constants import \ boltzmann_constant_cgs, \ mass_hydrogen_cgs @@ -38,12 +37,7 @@ cdef class chemistry_data: def __cinit__(self): self.data = _wrapped_c_chemistry_data() - self._rate_map = _rate_mapping_access.from_ptr_and_callback( - ptr=&self.rates, - callback=functools.partial( - _get_rate_shape, wrapped_chemistry_data_obj = self.data - ) - ) + self._rate_map = _rate_mapping_access.from_ptr(&self.rates) self.data_copy_from_init = None cdef void _try_uninitialize(self): @@ -871,32 +865,6 @@ cdef class _wrapped_c_chemistry_data: out[k] = self[k] return out -def _get_rate_shape(wrapped_chemistry_data_obj, rate_name): - # for now we need to manually keep this updated. - # -> in the future, we could add probably encode some/all of this - # information within Grackle's ratequery API - - def _is_standard_colrecombination_rate(rate_name): - if rate_name[:2] == 'kz' and rate_name[2:].isdecimal(): - return 11 <= int(rate_name[2:]) <= 54 - elif rate_name[:1] == 'k' and rate_name[1:].isdecimal(): - digit = int(rate_name[1:]) - return ( (1 <= digit <= 23) or - (50 <= digit <= 58) or - (125 <= digit <= 153) ) - return False - - if rate_name in ("k24", "k25", "k26", "k27", "k28", "k29", "k30", "k31"): - return () # the rate is a scalar - elif _is_standard_colrecombination_rate(rate_name): - return (wrapped_chemistry_data_obj['NumberOfTemperatureBins'],) - elif rate_name == 'k13dd': - return (wrapped_chemistry_data_obj['NumberOfTemperatureBins'] * 14,) - else: - raise RuntimeError( - "the shape of the rate {rate_name!r} has not been specified yet" - ) - cdef class _rate_mapping_access: # This class is used internally by the chemistry_data extension class to @@ -912,12 +880,10 @@ cdef class _rate_mapping_access: # we might make some different choices) cdef c_chemistry_data_storage *_ptr - cdef object _rate_shape_callback cdef dict _cached_name_rateid_map def __cinit__(self): self._ptr = NULL - self._rate_shape_callback = None self._cached_name_rateid_map = None def __init__(self): @@ -925,14 +891,11 @@ cdef class _rate_mapping_access: raise TypeError("This class cannot be instantiated directly.") @staticmethod - cdef _rate_mapping_access from_ptr_and_callback( - c_chemistry_data_storage *ptr, object callback - ): + cdef _rate_mapping_access from_ptr(c_chemistry_data_storage *ptr): cdef _rate_mapping_access out = _rate_mapping_access.__new__( _rate_mapping_access ) out._ptr = ptr - out._rate_shape_callback = callback return out @property @@ -991,14 +954,8 @@ cdef class _rate_mapping_access: # retrieve the pointer cdef double* rate_ptr = grunstable_ratequery_get_ptr(self._ptr, rate_id) - # experimental shape retrieval - exp_shape = self._try_get_shape(rate_id) - # lookup the shape of the rates - callback = self._rate_shape_callback - shape = callback(rate_name=key) - - assert shape == exp_shape + shape = self._try_get_shape(rate_id) # predeclare a memoryview to use with 1d arrays cdef double[:] memview From 080a097df43d743d83da12228c62b5eec0268d3c Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 13 Dec 2025 09:26:23 -0500 Subject: [PATCH 25/40] Tweak grackle wrapper This change ensures that the chemistry_data extension type will provide a consistent set of attributes, even if we make unused rate information inaccessible through the ratequery interface --- src/python/gracklepy/grackle_wrapper.pyx | 65 ++++++++++++++++++++---- 1 file changed, 56 insertions(+), 9 deletions(-) diff --git a/src/python/gracklepy/grackle_wrapper.pyx b/src/python/gracklepy/grackle_wrapper.pyx index 46ccf3e67..772ccfb36 100644 --- a/src/python/gracklepy/grackle_wrapper.pyx +++ b/src/python/gracklepy/grackle_wrapper.pyx @@ -20,6 +20,41 @@ from libc.limits cimport INT_MAX from .grackle_defs cimport * import numpy as np +# define a set of rate-related properties that the chemistry_data extension +# type must support as attributes: +# - historically, these rates have been accessible regardless of whether a +# chemistry solver class has been defined to use them. +# - in the near future, the _rate_mapping_access machinery may lose the ability +# to access rate buffers that are not being actively used +# - we will use this set to ensure that these types remain accessible +_legacy_rate_attrs = frozenset( + [ + "k1", "k2", "k3", "k4", "k5", "k6", "k7", "k8", "k9", "k10", "k11", + "k12", "k13", "k14", "k15", "k16", "k17", "k18", "k19", "k20", "k21", + "k22", "k23", "k50", "k51", "k52", "k53", "k54", "k55", "k56", "k57", + "k58", "k13dd", + # radiative rates: + "k24", "k25", "k26", "k27", "k28", "k29", "k30", "k31", + + # A question for another time (before releasing Grackle 3.5): + # - do we want to provide an alternative approach for people to query + # rates? (like a method or function?) + # - If so, then maybe we don't the following to be attributes of + # chemistry_data + # - for now, they are accessible as attributes of chemistry_data + + # 15 species rates (with DM, HDII, HeHII) + "k125", "k129", "k130", "k131", "k132", "k133", "k134", "k135", "k136", + "k137", "k148", "k149", "k150", "k151", "k152", "k153", + # metal species rates: + "kz15", "kz16", "kz17", "kz18", "kz19", "kz20", "kz21", "kz22", "kz23", + "kz24", "kz25", "kz26", "kz27", "kz28", "kz29", "kz30", "kz31", "kz32", + "kz33", "kz34", "kz35", "kz36", "kz37", "kz38", "kz39", "kz40", "kz41", + "kz42", "kz43", "kz44", "kz45", "kz46", "kz47", "kz48", "kz49", "kz50", + "kz51", "kz52", "kz53", "kz54", + ] +) + cdef class chemistry_data: cdef _wrapped_c_chemistry_data data cdef c_chemistry_data_storage rates @@ -93,10 +128,8 @@ cdef class chemistry_data: except KeyError: pass - try: - return self._rate_map[name] # case where name specifies a rate - except KeyError: - pass + if name in _legacy_rate_attrs: + return self._rate_map.get(name) # this method is expected to raise AttributeError when it fails raise AttributeError( @@ -143,11 +176,15 @@ cdef class chemistry_data: except KeyError: pass - try: - self._rate_map[name] = value - return # early exit - except KeyError: - pass + if name in _legacy_rate_attrs: + try: + self._rate_map[name] = value + return # early exit + except KeyError: + raise AttributeError( + f"attribute '{name}' of '{type(self).__name__}' can't be " + "mutated under the current configuration" + ) from None raise AttributeError( f"'{type(self).__name__}' object has no attribute '{name}'" @@ -980,6 +1017,16 @@ cdef class _rate_mapping_access: "no support is in place for higher dimensional arrays" ) + def get(self, key, default=None, /): + """ + Retrieve the value associated with key, if key is known. Otherwise, + return the default. + """ + try: + return self._access_rate(key, _NOSETVAL) + except: + return default + def __getitem__(self, key): return self._access_rate(key, _NOSETVAL) def __setitem__(self, key, value): self._access_rate(key, value) From 3a95ef02cfca9bba1f22f156345495d758bf7ba0 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 13 Dec 2025 09:30:19 -0500 Subject: [PATCH 26/40] remove unimplemented rate properties --- src/clib/ratequery.cpp | 5 ----- src/include/grackle.h | 5 +---- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/src/clib/ratequery.cpp b/src/clib/ratequery.cpp index be4c582d7..deb8328b7 100644 --- a/src/clib/ratequery.cpp +++ b/src/clib/ratequery.cpp @@ -292,15 +292,10 @@ extern "C" int grunstable_ratequery_prop( } return GR_SUCCESS; } - case GRUNSTABLE_QPROP_TYPE: - return GR_FAIL; case GRUNSTABLE_QPROP_MAXITEMSIZE: { *ptr = static_cast(sizeof(double)); return GR_SUCCESS; } - case GRUNSTABLE_QPROP_WRITABLE: { - *ptr = 1LL; - } default: GR_INTERNAL_UNREACHABLE_ERROR(); } diff --git a/src/include/grackle.h b/src/include/grackle.h index ab6e03e01..19da9c6d9 100644 --- a/src/include/grackle.h +++ b/src/include/grackle.h @@ -277,10 +277,7 @@ double* grunstable_ratequery_get_ptr( enum grunstable_ratequery_prop_kind { GRUNSTABLE_QPROP_NDIM = 1, GRUNSTABLE_QPROP_SHAPE = 2, - GRUNSTABLE_QPROP_TYPE = 3, - GRUNSTABLE_QPROP_MAXITEMSIZE = 4, - // I don't like the next one - GRUNSTABLE_QPROP_WRITABLE = 5, + GRUNSTABLE_QPROP_MAXITEMSIZE = 3, }; /// Query a property of the specified rate From 5df10fcf487dc8ef5c9d8ab1a5574c85754e27f6 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 19 Dec 2025 08:57:59 -0500 Subject: [PATCH 27/40] address a gcc compiler warning --- src/clib/ratequery.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clib/ratequery.cpp b/src/clib/ratequery.cpp index deb8328b7..2d0bcfc80 100644 --- a/src/clib/ratequery.cpp +++ b/src/clib/ratequery.cpp @@ -190,7 +190,7 @@ struct ratequery_rslt_ { }; static ratequery_rslt_ invalid_rslt_() { - return {UNDEFINED_RATE_ID_, {nullptr, nullptr}}; + return {UNDEFINED_RATE_ID_, mk_invalid_Entry()}; } static const Registry* get_registry(const chemistry_data_storage* my_rates) { From fe1aba6a1893257a5f5030bc60253719e234f7bc Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 19 Dec 2025 11:37:55 -0500 Subject: [PATCH 28/40] introduce grunstable_ratequery_nrates --- src/clib/ratequery.cpp | 12 ++++++++++++ src/include/grackle.h | 4 ++++ tests/grtestutils/iterator_adaptor.hpp | 11 ----------- tests/unit/test_api_ratequery.cpp | 10 +++++++++- 4 files changed, 25 insertions(+), 12 deletions(-) diff --git a/src/clib/ratequery.cpp b/src/clib/ratequery.cpp index 2d0bcfc80..5c54f74cb 100644 --- a/src/clib/ratequery.cpp +++ b/src/clib/ratequery.cpp @@ -315,3 +315,15 @@ extern "C" const char* grunstable_ith_rate( } return tmp.entry.name; } + +extern "C" unsigned long long grunstable_ratequery_nrates( + const chemistry_data_storage* my_rates) { + namespace rate_q = grackle::impl::ratequery; + const rate_q::Registry* registry = my_rates->opaque_storage->registry; + + unsigned long long out = 0; + for (int i = 0; i < registry->len; i++) { + out += static_cast(registry->sets[i].len); + } + return out; +} diff --git a/src/include/grackle.h b/src/include/grackle.h index 19da9c6d9..a599a15d5 100644 --- a/src/include/grackle.h +++ b/src/include/grackle.h @@ -313,6 +313,10 @@ const char* grunstable_ith_rate( grunstable_rateid_type* out_rate_id ); +/// Query the number of rates accessible through the ratequery API +unsigned long long grunstable_ratequery_nrates( + const chemistry_data_storage* my_rates); + /** @}*/ // end of group #ifdef __cplusplus diff --git a/tests/grtestutils/iterator_adaptor.hpp b/tests/grtestutils/iterator_adaptor.hpp index af5f9f259..cb9781047 100644 --- a/tests/grtestutils/iterator_adaptor.hpp +++ b/tests/grtestutils/iterator_adaptor.hpp @@ -109,17 +109,6 @@ struct RateQueryPlugin { } }; -// will be implemented (in a much more robust manner) in the near future -inline unsigned long long grunstable_ratequery_nrates( - const chemistry_data_storage* my_rates) { - // current implementation is stupid! (in future, will use my_rates) - unsigned long long i = 0; - while (nullptr != grunstable_ith_rate(my_rates, i, nullptr)) { - i++; - } - return i; -} - /// used for creating the iterator and within range-based for-loops class RateQueryRange { RateQueryPlugin plugin_; diff --git a/tests/unit/test_api_ratequery.cpp b/tests/unit/test_api_ratequery.cpp index cf9c5f52e..375f8257c 100644 --- a/tests/unit/test_api_ratequery.cpp +++ b/tests/unit/test_api_ratequery.cpp @@ -22,7 +22,6 @@ /// returns the rateid used to denote invalid rate names grunstable_rateid_type get_invalid_rateid(const grtest::GrackleCtxPack& pack) { - // although we don't use pack, yet a forthcoming refactor requires it return grunstable_ratequery_id(pack.my_rates(), nullptr); } @@ -36,6 +35,15 @@ TEST_F(SimpleRateQueryTest, InvalidIthRate) { EXPECT_EQ(name, nullptr); } +TEST_F(SimpleRateQueryTest, IthRateChecks) { + chemistry_data_storage* my_rates = pack.my_rates(); + unsigned long long n_rates = grunstable_ratequery_nrates(my_rates); + ASSERT_GT(n_rates, 0); // <- sanity check! + EXPECT_NE(nullptr, grunstable_ith_rate(my_rates, n_rates - 1, nullptr)); + EXPECT_EQ(nullptr, grunstable_ith_rate(my_rates, n_rates, nullptr)); + EXPECT_EQ(nullptr, grunstable_ith_rate(my_rates, n_rates + 1, nullptr)); +} + TEST_F(SimpleRateQueryTest, EmptyNameQuery) { grunstable_rateid_type rateid = grunstable_ratequery_id(pack.my_rates(), ""); EXPECT_EQ(rateid, get_invalid_rateid(pack)); From 93c4c74842dfacc7a90d2e03f1654d81616efdc6 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 19 Dec 2025 17:39:51 -0500 Subject: [PATCH 29/40] add some tests that query rates --- tests/grtestutils/CMakeLists.txt | 1 + tests/grtestutils/googletest/assertions.hpp | 70 ++++++ tests/grtestutils/iterator_adaptor.hpp | 9 + tests/unit/test_api_ratequery.cpp | 258 ++++++++++++++++++++ 4 files changed, 338 insertions(+) create mode 100644 tests/grtestutils/googletest/assertions.hpp diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index 27bf11d17..42bb65246 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -86,6 +86,7 @@ add_library(grtestutils utils.hpp utils.cpp # files in the googletest subdirectory (reminder: should just be headers!) + googletest/assertions.hpp googletest/check_allclose.hpp googletest/fixtures.hpp ) diff --git a/tests/grtestutils/googletest/assertions.hpp b/tests/grtestutils/googletest/assertions.hpp new file mode 100644 index 000000000..14177d7d8 --- /dev/null +++ b/tests/grtestutils/googletest/assertions.hpp @@ -0,0 +1,70 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Declares some general-purpose assertions for testing grackle +/// +//===----------------------------------------------------------------------===// +#ifndef GRTEST_GOOGLETEST_ASSERTIONS_HPP +#define GRTEST_GOOGLETEST_ASSERTIONS_HPP + +#include + +#include +#include "grackle.h" + +namespace grtest { + +inline testing::AssertionResult IsGRSUCCESS(const char* expr1, int val1) { + if (val1 == GR_SUCCESS) { + return testing::AssertionSuccess(); + } + + std::string descr; + switch (val1) { + case GR_SUCCESS: { + descr = "GR_SUCCESS"; + break; + } + case GR_FAIL: { + descr = "GR_FAIL"; + break; + } + default: { + descr = "not a standard code"; + } + } + testing::AssertionResult out = testing::AssertionFailure(); + out << "Evaluated: " << expr1 << '\n' + << " Expected: GR_SUCCESS (aka " << GR_SUCCESS << ")\n" + << " Actual: " << val1 << " (" << descr << ')'; + return out; +} + +inline testing::AssertionResult IsGRError(const char* expr1, int val1) { + if (val1 != GR_SUCCESS) { + return testing::AssertionSuccess(); + } + + testing::AssertionResult out = testing::AssertionFailure(); + out << "Evaluated: " << expr1 << '\n' + << " Expected: a value other than GR_SUCCESS\n" + << " Actual: " << val1 << " (GR_SUCCESS)"; + return out; +} + +} // namespace grtest + +#define EXPECT_GR_SUCCESS(expr) EXPECT_PRED_FORMAT1(::grtest::IsGRSUCCESS, expr) + +#define ASSERT_GR_SUCCESS(expr) ASSERT_PRED_FORMAT1(::grtest::IsGRSUCCESS, expr) + +#define EXPECT_GR_ERR(expr) EXPECT_PRED_FORMAT1(::grtest::IsGRError, expr) + +#define ASSERT_GR_ERR(expr) ASSERT_PRED_FORMAT1(::grtest::IsGRError, expr) + +#endif // GRTEST_GOOGLETEST_ASSERTIONS_HPP diff --git a/tests/grtestutils/iterator_adaptor.hpp b/tests/grtestutils/iterator_adaptor.hpp index cb9781047..71bd745a2 100644 --- a/tests/grtestutils/iterator_adaptor.hpp +++ b/tests/grtestutils/iterator_adaptor.hpp @@ -13,6 +13,7 @@ #define GRTESTUTILS_ITERATOR_ADAPTOR_HPP #include +#include #include #include "grackle.h" @@ -26,6 +27,14 @@ struct NameIdPair { long long id; }; +/// teach std::ostream how to format NameIdPair +/// +/// The motivation is to make it easier write detailed error messages +inline std::ostream& operator<<(std::ostream& os, const NameIdPair& pair) { + os << "{name=\"" << pair.name << "\", id=" << pair.id << "}"; + return os; +} + /// implements a C++ style InputIterator by adapting a simple Plugin type /// that wraps a set of Grackle functions /// diff --git a/tests/unit/test_api_ratequery.cpp b/tests/unit/test_api_ratequery.cpp index 375f8257c..9a0b58f02 100644 --- a/tests/unit/test_api_ratequery.cpp +++ b/tests/unit/test_api_ratequery.cpp @@ -10,15 +10,22 @@ /// //===----------------------------------------------------------------------===// +#include // std::min, std::max #include #include +#include +#include #include #include +#include #include +#include #include "grtestutils/iterator_adaptor.hpp" +#include "grtestutils/googletest/assertions.hpp" #include "grtestutils/googletest/fixtures.hpp" #include "grackle.h" +#include "status_reporting.h" /// returns the rateid used to denote invalid rate names grunstable_rateid_type get_invalid_rateid(const grtest::GrackleCtxPack& pack) { @@ -66,6 +73,71 @@ TEST_F(SimpleRateQueryTest, PtrInvalidRateId) { // tests themselves as easy to read as possible, we implate a C++-style // iterator to wrap part of the interface +std::string stringify_prop_kind(enum grunstable_ratequery_prop_kind kind) { + switch (kind) { + case GRUNSTABLE_QPROP_NDIM: + return "GRUNSTABLE_QPROP_NDIM"; + case GRUNSTABLE_QPROP_SHAPE: + return "GRUNSTABLE_QPROP_SHAPE"; + case GRUNSTABLE_QPROP_MAXITEMSIZE: + return "GRUNSTABLE_QPROP_MAXITEMSIZE"; + } + GR_INTERNAL_UNREACHABLE_ERROR(); +} + +/// return a vector of some invalid rateids +std::vector invalid_rateids( + grtest::GrackleCtxPack& pack) { + grunstable_rateid_type max_id = + std::numeric_limits::lowest(); + grunstable_rateid_type min_id = + std::numeric_limits::max(); + + // iterate over (parameter-name, rate-id) pairs + for (const grtest::NameIdPair pair : grtest::RateQueryRange(pack)) { + max_id = std::max(max_id, static_cast(pair.id)); + min_id = std::min(min_id, static_cast(pair.id)); + } + + std::vector bad_ids; + if (min_id > std::numeric_limits::lowest()) { + bad_ids.push_back(min_id - 1); + } + if (max_id < std::numeric_limits::max()) { + bad_ids.push_back(max_id + 1); + } + return bad_ids; +} + +TEST_F(SimpleRateQueryTest, PropertyInvalidRateID) { + std::vector invalid_ids = invalid_rateids(pack); + if (invalid_ids.empty()) { + GTEST_SKIP() << "unable to come up with known invalid rate ids to use for " + << "the test"; + } + + std::vector prop_kinds{ + GRUNSTABLE_QPROP_NDIM, GRUNSTABLE_QPROP_SHAPE, + GRUNSTABLE_QPROP_MAXITEMSIZE}; + std::vector buf; + + for (grunstable_rateid_type invalid_id : invalid_ids) { + for (enum grunstable_ratequery_prop_kind kind : prop_kinds) { + constexpr std::size_t BUF_LEN = 20; // <- arbitrarily large value + constexpr long long DEFAULT_VAL = -25634634LL; // <- arbitrary value + buf.assign(BUF_LEN, DEFAULT_VAL); + ASSERT_GR_ERR(grunstable_ratequery_prop(pack.my_rates(), invalid_id, kind, + buf.data())) + << "executed with invalid_id=" << invalid_id + << ", kind=" << stringify_prop_kind(kind); + EXPECT_THAT(buf, testing::Each(testing::Eq(DEFAULT_VAL))) + << "grunstable_ratequery_prop mutated the ptr even though it " + << "reported a failure. It was called with an invalid id of " + << invalid_id << " and a kind of " << stringify_prop_kind(kind); + } + } +} + using ParametrizedRateQueryTest = grtest::ParametrizedConfigPresetFixture; TEST_P(ParametrizedRateQueryTest, AllUnique) { @@ -87,6 +159,102 @@ TEST_P(ParametrizedRateQueryTest, ConsistentIDs) { } } +TEST_P(ParametrizedRateQueryTest, Property) { + std::vector buf; + for (const grtest::NameIdPair pair : grtest::RateQueryRange(pack)) { + constexpr long long DEFAULT_VAL = -25634634LL; // <- arbitrary value + + // check ndim + long long ndim = DEFAULT_VAL; + EXPECT_GR_SUCCESS(grunstable_ratequery_prop(pack.my_rates(), pair.id, + GRUNSTABLE_QPROP_NDIM, &ndim)) + << "for " << pair; + ASSERT_GE(ndim, 0LL) << "for " << pair; + + // check shape + buf.assign((ndim == 0LL) ? 1 : ndim, DEFAULT_VAL); + EXPECT_GR_SUCCESS(grunstable_ratequery_prop( + pack.my_rates(), pair.id, GRUNSTABLE_QPROP_SHAPE, buf.data())) + << "for " << pair; + if (ndim == 0LL) { + EXPECT_EQ(buf[0], DEFAULT_VAL) + << "the buffer passed to grunstable_ratequery_prop was unexpectedly " + << "modified while querying the shape for the rate " << pair + << ". It shouldn't be modified since ndim=0."; + } else { + EXPECT_THAT(buf, testing::Each(testing::Gt(0))) + << "buf holds the shape queried for " << pair; + } + + long long maxitemsize = DEFAULT_VAL; + EXPECT_GR_SUCCESS(grunstable_ratequery_prop( + pack.my_rates(), pair.id, GRUNSTABLE_QPROP_MAXITEMSIZE, &maxitemsize)) + << "for " << pair; + EXPECT_EQ(maxitemsize, sizeof(double)) << "for " << pair; + } +} + +/// summarizes details about rate properties +struct RateProperties { + std::vector shape; + std::size_t maxitemsize; + + bool operator==(const RateProperties& other) const { + return maxitemsize == other.maxitemsize && shape == other.shape; + } + + /// teach googletest how to print this type + friend void PrintTo(const RateProperties& props, std::ostream* os) { + *os << "{shape={"; + for (std::size_t i = 0; i < props.shape.size(); i++) { + if (i != 0) { + *os << ", "; + } + *os << props.shape[i]; + } + *os << "}, maxitemsize=" << props.maxitemsize << "}"; + } +}; + +/// construct a RateProperties instance for the specified rate +/// +/// returns an empty optional if any there are any issues +std::optional try_query_RateProperties( + chemistry_data_storage* my_rates, grunstable_rateid_type rateid) { + long long ndim = -1LL; + std::vector shape; + if (grunstable_ratequery_prop(my_rates, rateid, GRUNSTABLE_QPROP_NDIM, + &ndim) != GR_SUCCESS) { + return std::nullopt; + } else if (ndim != 0LL) { + if (ndim < 0LL) { // <- sanity check! + return std::nullopt; + } + shape.assign(ndim, 0LL); + if (grunstable_ratequery_prop(my_rates, rateid, GRUNSTABLE_QPROP_SHAPE, + shape.data()) != GR_SUCCESS) { + return std::nullopt; + } + } + + // sanity check! confirm that no shape elements are non-positive + if (std::count_if(shape.begin(), shape.end(), + [](long long x) { return x <= 0; }) > 0) { + return std::nullopt; // sanity check failed! + } + + long long maxitemsize = -1LL; + if (grunstable_ratequery_prop(my_rates, rateid, GRUNSTABLE_QPROP_MAXITEMSIZE, + &maxitemsize) != GR_SUCCESS) { + return std::nullopt; + } + if (maxitemsize <= 0LL) { + return std::nullopt; + } + + return {RateProperties{shape, static_cast(maxitemsize)}}; +} + using grtest::ChemPreset; using grtest::FullConfPreset; using grtest::InitialUnitPreset; @@ -100,3 +268,93 @@ INSTANTIATE_TEST_SUITE_P( FullConfPreset{ChemPreset::primchem3, InitialUnitPreset::simple_z0}, FullConfPreset{ChemPreset::primchem4_dustspecies3, InitialUnitPreset::simple_z0})); + +// now, we are going to check on some well-established rates +// -> this may not be the most maintainable test (we may want to re-evaluate it +// in the future) + +enum RateKind { scalar_f64, simple_1d_rate, k13dd }; + +static RateProperties RateProperties_from_RateKind( + const chemistry_data* my_chemistry, RateKind kind) { + switch (kind) { + case RateKind::scalar_f64: { + std::vector shape = {}; // <-- intentionally empty + return RateProperties{shape, sizeof(double)}; + } + case RateKind::simple_1d_rate: { + std::vector shape = {my_chemistry->NumberOfTemperatureBins}; + return RateProperties{shape, sizeof(double)}; + } + case RateKind::k13dd: { + std::vector shape = {my_chemistry->NumberOfTemperatureBins * + 14}; + return RateProperties{shape, sizeof(double)}; + } + } + GR_INTERNAL_UNREACHABLE_ERROR() +} + +/// returns a map between known rate names and the rate kind +std::map known_rates() { + std::map out; + // radiative rates: + for (int i = 24; i < 32; i++) { + out.insert({"k" + std::to_string(i), RateKind::scalar_f64}); + } + + // standard collisional rates + for (int i = 1; i < 24; i++) { + out.insert({"k" + std::to_string(i), RateKind::simple_1d_rate}); + } + for (int i = 50; i < 58; i++) { + out.insert({"k" + std::to_string(i), RateKind::simple_1d_rate}); + } + for (int i = 125; i < 154; i++) { + out.insert({"k" + std::to_string(i), RateKind::simple_1d_rate}); + } + + // metal chemistry rates + for (int i = 11; i < 55; i++) { + out.insert({"kz" + std::to_string(i), RateKind::simple_1d_rate}); + } + + out.insert({"k13dd", RateKind::k13dd}); + + return out; +} + +using KnownRateQueryTest = + grtest::ConfigPresetFixture; + +TEST_F(KnownRateQueryTest, CheckProperties) { + const std::map known_rate_map = known_rates(); + + // iterate over every known {parameter-name, key-id} pair + for (const grtest::NameIdPair pair : grtest::RateQueryRange(pack)) { + // check if the current rateid is in known_rate_map + std::map::const_iterator search = + known_rate_map.find(pair.name); + if (search == known_rate_map.end()) { + continue; // the rateid is **NOT** in known_rate_map + } + // construct the expected properties + RateProperties expected_props = + RateProperties_from_RateKind(pack.my_chemistry(), search->second); + + // load the actual props + std::optional maybe_actual_props = + try_query_RateProperties(pack.my_rates(), pair.id); + if (!maybe_actual_props.has_value()) { + GTEST_FAIL() + << "something went wrong while trying to lookup the properties for " + << "the " << pair << " rate."; + } + RateProperties actual_props = maybe_actual_props.value(); + + EXPECT_EQ(expected_props, actual_props) + << "this mismatch in the queried properties is for the " << pair + << " rate."; + } +} From 08841f77b758cf9e52254066db9d63aec2d2cc46 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 19 Dec 2025 19:12:12 -0500 Subject: [PATCH 30/40] introduced a setter and getter function --- src/clib/ratequery.cpp | 67 +++++++++++++++++++++++++++++++ src/include/grackle.h | 56 +++++++++++++++++++++++--- tests/unit/test_api_ratequery.cpp | 62 ++++++++++++++++++++++++++++ 3 files changed, 180 insertions(+), 5 deletions(-) diff --git a/src/clib/ratequery.cpp b/src/clib/ratequery.cpp index 5c54f74cb..294b731ed 100644 --- a/src/clib/ratequery.cpp +++ b/src/clib/ratequery.cpp @@ -147,6 +147,9 @@ static Entry get_MiscRxn_Entry(chemistry_data_storage* my_rates, int i) { grackle::impl::ratequery::Registry grackle::impl::ratequery::new_Registry( const chemistry_data& my_chemistry) { + if (my_chemistry.primordial_chemistry == 0) { + return Registry{0, nullptr}; + } EntryProps props_LogTLinInterp = mk_invalid_EntryProps(); props_LogTLinInterp.ndim = 1; props_LogTLinInterp.shape[0] = my_chemistry.NumberOfTemperatureBins; @@ -232,6 +235,37 @@ static ratequery_rslt_ query_Entry(chemistry_data_storage* my_rates, return invalid_rslt_(); } +/* +/// this only exists for debugging purposes +static void show_Entry(const Entry* entry) { + std::printf( + "{.data=%p, .name=\"%s\", .props={.ndim=%d, .shape={", + reinterpret_cast(entry->data), entry->name, entry->props.ndim); + // we should strongly consider factoring out logic for printing arrays + for (int i = 0; i < entry->props.ndim; i++) { + if (i > 0) { + std::printf(", "); + } + std::printf("%d", entry->props.shape[i]); + } + std::printf("}}\n"); +} +*/ + +/// compute the number of items in an Entry described by @p props +static long long get_n_items(EntryProps props) { + GR_INTERNAL_REQUIRE(props.ndim >= 0, "sanity check!"); + + if (props.ndim == 0) { + return 1LL; // a scalar always consists of 1 item + } + long long n_items = 1LL; + for (int i = 0; i < props.ndim; i++) { + n_items *= static_cast(props.shape[i]); + } + return n_items; +} + } // namespace grackle::impl::ratequery // here we implement the public API @@ -258,6 +292,39 @@ extern "C" grunstable_rateid_type grunstable_ratequery_id( return rate_q::UNDEFINED_RATE_ID_; } +extern "C" int grunstable_ratequery_get_f64(chemistry_data_storage* my_rates, + grunstable_rateid_type rate_id, + double* buf) { + namespace rate_q = grackle::impl::ratequery; + rate_q::Entry entry = rate_q::query_Entry(my_rates, rate_id, true).entry; + + if (entry.data == nullptr) { // in this case, the query failed + return GR_FAIL; + } + + long long n_items = rate_q::get_n_items(entry.props); + for (long long i = 0; i < n_items; i++) { + buf[i] = entry.data[i]; + } + return GR_SUCCESS; +} + +extern "C" int grunstable_ratequery_set_f64(chemistry_data_storage* my_rates, + grunstable_rateid_type rate_id, + const double* buf) { + namespace rate_q = grackle::impl::ratequery; + rate_q::Entry entry = rate_q::query_Entry(my_rates, rate_id, true).entry; + if (entry.data == nullptr) { // in this case, the query failed + return GR_FAIL; + } + + long long n_items = rate_q::get_n_items(entry.props); + for (long long i = 0; i < n_items; i++) { + entry.data[i] = buf[i]; + } + return GR_SUCCESS; +} + extern "C" double* grunstable_ratequery_get_ptr( chemistry_data_storage* my_rates, grunstable_rateid_type rate_id) { namespace rate_q = grackle::impl::ratequery; diff --git a/src/include/grackle.h b/src/include/grackle.h index a599a15d5..5f513a184 100644 --- a/src/include/grackle.h +++ b/src/include/grackle.h @@ -198,11 +198,7 @@ int gr_initialize_field_data(grackle_field_data *my_fields); /// > - we should probably update all of the function signatures so that a /// > pointer to chemistry_data_storage* is passed as an argument to each /// > of the functions. -/// > 2. Should we prevent direct access to the underlying data pointers? -/// > Instead we would provide getter and setters. We discuss this further -/// > down below (in the relevant function's API). This is important for -/// > different backends. -/// > 3. We should generally consider whether the API is consistent enough with +/// > 2. We should generally consider whether the API is consistent enough with /// > APIs for accessing other data structures. /// > - currently the dynamic parameter API is grackle's only other /// > data-access API, but we could imagine creating an API for the fields @@ -234,6 +230,56 @@ typedef long long grunstable_rateid_type; grunstable_rateid_type grunstable_ratequery_id( const chemistry_data_storage* my_rates, const char* name); +/// Copy data associated with the @p rate_id in @p my_rates into buf +/// +/// The behavior is currently undefined if: +/// - the @p my_rates argument is a NULL pointer +/// - the @p my_rates argument isn't configured to use the specified rate +/// - the @p buf argument is NULL +/// - the @p buf isn't large enough to store the copied rates +/// +/// @param[in] my_rates The object from which data is retrieved +/// @param[in] rate_id The id of the rate for which the data is retrieved +/// @param[out] buf The buffer that the function writes to +/// +/// @return Returns GR_SUCCESS if successful. If an invalid @p rate_id is +/// specified, this returns a different value +/// +/// > [!note] +/// > Before stablizing, we should consider whether we want to add an argument +/// > that specifies the supplied size of `buf` (and provide well-defined +/// > behavior when `buf` is too small +int grunstable_ratequery_get_f64( + chemistry_data_storage* my_rates, grunstable_rateid_type rate_id, + double* buf +); + + +/// Overwrite the data associated with @p rate_id in @p my_rates with the +/// values provided by @p buf +/// +/// The behavior is currently undefined if: +/// - the @p my_rates argument is a NULL pointer +/// - the @p my_rates argument isn't configured to use the specified rate +/// - the @p buf argument is NULL +/// - the @p buf isn't large enough to store the copied rates +/// +/// @param[out] my_rates The object from which data is retrieved +/// @param[in] rate_id The id of the rate for which the data is retrieved +/// @param[in] buf The buffer that the function writes to +/// +/// @return Returns GR_SUCCESS if successful. If an invalid @p rate_id is +/// specified, this returns a different value +/// +/// > [!note] +/// > Before stablizing, we should consider whether we want to add an argument +/// > that specifies the supplied size of `buf` (and provide well-defined +/// > behavior when `buf` is too small +int grunstable_ratequery_set_f64( + chemistry_data_storage* my_rates, grunstable_rateid_type rate_id, + const double* buf +); + /// Access the pointer associated with the rateid from myrates /// /// The behavior is **NOT** currently defined if the `my_rates` struct isn't diff --git a/tests/unit/test_api_ratequery.cpp b/tests/unit/test_api_ratequery.cpp index 9a0b58f02..e7b6c5672 100644 --- a/tests/unit/test_api_ratequery.cpp +++ b/tests/unit/test_api_ratequery.cpp @@ -203,6 +203,14 @@ struct RateProperties { return maxitemsize == other.maxitemsize && shape == other.shape; } + long long n_items() const { + long long n_items = 1LL; + for (std::size_t i = 0; i < shape.size(); i++) { + n_items *= shape[i]; + } + return n_items; // this is correct even for scalars + } + /// teach googletest how to print this type friend void PrintTo(const RateProperties& props, std::ostream* os) { *os << "{shape={"; @@ -255,6 +263,57 @@ std::optional try_query_RateProperties( return {RateProperties{shape, static_cast(maxitemsize)}}; } +// returns a value that differs from the input +static double remap_value(double in) { + if (in == 0.0 || !std::isfinite(in)) { + return 1.0; + } + return -in; +} + +TEST_P(ParametrizedRateQueryTest, SetAndGet) { + std::vector initial_buf; + std::vector post_update_buf; + + // iterate over every known (rate-name, rate-id) pair + for (const grtest::NameIdPair pair : grtest::RateQueryRange(pack)) { + // get the properties associated with the current rate + std::optional maybe_props = + try_query_RateProperties(pack.my_rates(), pair.id); + if (!maybe_props.has_value()) { + GTEST_FAIL() + << "something went wrong while trying to lookup the properties for " + << "the " << pair << " rate."; + } + RateProperties props = maybe_props.value(); + long long n_items = props.n_items(); + + // load in data associated with the current rate + initial_buf.assign(n_items, NAN); + ASSERT_GR_SUCCESS(grunstable_ratequery_get_f64(pack.my_rates(), pair.id, + initial_buf.data())) + << "for " << pair; + + // overwrite each entry with a different value + for (long long i = 0; i < n_items; i++) { + initial_buf[i] = remap_value(initial_buf[i]); + } + + // write the new values back to my_rates + EXPECT_GR_SUCCESS(grunstable_ratequery_set_f64(pack.my_rates(), pair.id, + initial_buf.data())) + << "for " << pair; + + // finally, lets check that the values actually got updated + post_update_buf.assign(n_items, NAN); + EXPECT_GR_SUCCESS(grunstable_ratequery_get_f64(pack.my_rates(), pair.id, + post_update_buf.data())) + << "for " << pair; + + EXPECT_EQ(initial_buf, post_update_buf) << "for " << pair; + } +} + using grtest::ChemPreset; using grtest::FullConfPreset; using grtest::InitialUnitPreset; @@ -357,4 +416,7 @@ TEST_F(KnownRateQueryTest, CheckProperties) { << "this mismatch in the queried properties is for the " << pair << " rate."; } + + // it might be useful to repeat the tests using another chemistry_data + // instance with a different NumberOfTemperatureBins value } From c75aacfdbc7f0820a37298e99948ba7c0629d558 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 20 Dec 2025 10:54:58 -0500 Subject: [PATCH 31/40] gracklepy: implement wrappers that use getter/setter --- src/python/gracklepy/grackle_defs.pxd | 10 ++ src/python/gracklepy/grackle_wrapper.pyx | 116 ++++++++++++++++++++++- 2 files changed, 125 insertions(+), 1 deletion(-) diff --git a/src/python/gracklepy/grackle_defs.pxd b/src/python/gracklepy/grackle_defs.pxd index a8e6b761c..81de18dd8 100644 --- a/src/python/gracklepy/grackle_defs.pxd +++ b/src/python/gracklepy/grackle_defs.pxd @@ -303,6 +303,16 @@ cdef extern from "grackle.h": const c_chemistry_data_storage* my_rates, const char* name) + int grunstable_ratequery_get_f64( + c_chemistry_data_storage* my_rates, + grunstable_rateid_type rate_id, + double* buf) + + int grunstable_ratequery_set_f64( + c_chemistry_data_storage* my_rates, + grunstable_rateid_type rate_id, + const double* buf) + double* grunstable_ratequery_get_ptr( c_chemistry_data_storage* my_rates, grunstable_rateid_type rate_id) diff --git a/src/python/gracklepy/grackle_wrapper.pyx b/src/python/gracklepy/grackle_wrapper.pyx index 772ccfb36..0e2e12ea6 100644 --- a/src/python/gracklepy/grackle_wrapper.pyx +++ b/src/python/gracklepy/grackle_wrapper.pyx @@ -903,6 +903,18 @@ cdef class _wrapped_c_chemistry_data: return out +def _portable_reshape(arr: np.ndarray, shape: tuple[int, ...]) -> np.ndarray: + """ + Reshape a numpy array (raises an error if a copy can't be avoided) + """ + if np.__version__.startswith("1.") or np.__version__.startswith("2.0."): + out = arr.view() + out.shape = shape + return out + else: + return arr.reshape(shape, order="C", copy=False) + + cdef class _rate_mapping_access: # This class is used internally by the chemistry_data extension class to # wrap its chemistry_data_storage ptr and provide access to the stored @@ -929,6 +941,9 @@ cdef class _rate_mapping_access: @staticmethod cdef _rate_mapping_access from_ptr(c_chemistry_data_storage *ptr): + """ + Construct a _rate_mapping_access instance from the provided pointer + """ cdef _rate_mapping_access out = _rate_mapping_access.__new__( _rate_mapping_access ) @@ -937,9 +952,17 @@ cdef class _rate_mapping_access: @property def _name_rateid_map(self): + """returns a mapping between rate names and rate id""" + if self._cached_name_rateid_map is not None: return self._cached_name_rateid_map + if self._ptr is NULL: + raise RuntimeError( + "this instance hasn't been configured with a pointer for it " + "access retrieve data from" + ) + cdef dict out = {} cdef const char* rate_name cdef grunstable_rateid_type rate_id @@ -952,7 +975,10 @@ cdef class _rate_mapping_access: out[rate_name.decode('UTF-8')] = int(rate_id) i+=1 - def _try_get_shape(self, rate_id): + def _try_get_shape(self, rate_id: int) -> tuple[int, ...]: + """ + try to query the shape associated with rate_id + """ cdef long long buf[7] cdef int ret = grunstable_ratequery_prop( self._ptr, rate_id, GRUNSTABLE_QPROP_NDIM, &buf[0] @@ -976,6 +1002,94 @@ cdef class _rate_mapping_access: "the query for shape failed after query for ndim succeeded") return tuple(int(buf[i]) for i in range(ndim)) + def _get_rate(self, key: str): + """ + Returns a numpy array that holds a copy of grackle's internal data + associated with `key` + + Parameters + ---------- + key: str + The name of the quantity to access + + Returns + ------- + np.ndarray or float + The retrieved value + """ + + # lookup the rate_id and shape of the rates + rate_id = self._name_rateid_map[key] + shape = self._try_get_shape(rate_id) + + # allocate the memory that grackle will write data to + nelements = 1 if shape == () else np.prod(shape) + out = np.empty(shape=(nelements,), dtype=np.float64) + + # create a memoryview of out (so we can access the underlying pointer) + cdef double[:] memview = out + + if grunstable_ratequery_get_f64(self._ptr, rate_id, &memview[0]) != GR_SUCCESS: + raise RuntimeError( + "Something went wrong while retrieving the data associated with the " + f"\"{key}\" key" + ) + if shape == (): + return float(out[0]) + + if shape != out.shape: + out = _portable_reshape(out, shape=shape) + # we set the WRITABLE flag to False so that people don't mistakenly believe + # that by modifying the array in place that they are updating Grackle's + # internal buffers + out.setflags(write=False) + return out + + def _set_rate(self, key, val): + """ + Copies value(s) into the Grackle solver's internal buffer associated + with `key` + + Parameters + ---------- + key: str + The name of the quantity to modify + val: array_like + The values to be written + """ + # lookup the rate_id and shape of the rates + rate_id = self._name_rateid_map[key] + shape = self._try_get_shape(rate_id) + + # validate that val meets expectations and then coerce to `buf` a 1D numpy + # array that we can feed to the C function + if np.shape(val) != shape: + if shape == (): + raise ValueError(f"The \"{key}\" key expects a scalar") + raise ValueError(f"The \"{key}\" expects a value with shape, {shape}") + elif shape == (): + coerced = float(val) + buf = np.array([coerced]) + else: + coerced = np.asanyarray(val, dtype=np.float64, order="C") + if hasattr(coerced, "unit") or hasattr(coerced, "units"): + # val is probably an astropy.Quantity or unyt.unyt_array instance + raise ValueError( + f"'{type(self).__name__}' can't handle numpy.ndarray subclasses " + "that attach unit information" + ) + buf = _portable_reshape(coerced, shape = (coerced.size,)) + + # create a memoryview of out (so we can access the underlying pointer) + cdef const double[:] memview = buf + + if grunstable_ratequery_set_f64(self._ptr, rate_id, &memview[0]) != GR_SUCCESS: + raise RuntimeError( + "Something went wrong while writing data associated with the " + f"\"{key}\" key" + ) + + def _access_rate(self, key, val): # determine whether the rate needs to be updated update_rate = (val is not _NOSETVAL) From 088187305f508066c2dbda3572e2ab42c85cb676 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 20 Dec 2025 10:58:25 -0500 Subject: [PATCH 32/40] gracklepy: purge uses of grunstable_ratequery_get_ptr --- src/python/gracklepy/grackle_defs.pxd | 4 -- src/python/gracklepy/grackle_wrapper.pyx | 48 ++---------------------- 2 files changed, 3 insertions(+), 49 deletions(-) diff --git a/src/python/gracklepy/grackle_defs.pxd b/src/python/gracklepy/grackle_defs.pxd index 81de18dd8..9a8017345 100644 --- a/src/python/gracklepy/grackle_defs.pxd +++ b/src/python/gracklepy/grackle_defs.pxd @@ -313,10 +313,6 @@ cdef extern from "grackle.h": grunstable_rateid_type rate_id, const double* buf) - double* grunstable_ratequery_get_ptr( - c_chemistry_data_storage* my_rates, - grunstable_rateid_type rate_id) - cdef enum grunstable_ratequery_prop_kind: GRUNSTABLE_QPROP_NDIM GRUNSTABLE_QPROP_SHAPE diff --git a/src/python/gracklepy/grackle_wrapper.pyx b/src/python/gracklepy/grackle_wrapper.pyx index 0e2e12ea6..342817cae 100644 --- a/src/python/gracklepy/grackle_wrapper.pyx +++ b/src/python/gracklepy/grackle_wrapper.pyx @@ -1089,61 +1089,19 @@ cdef class _rate_mapping_access: f"\"{key}\" key" ) - - def _access_rate(self, key, val): - # determine whether the rate needs to be updated - update_rate = (val is not _NOSETVAL) - - rate_id = self._name_rateid_map[key] # will raise KeyError if not known - - if self._ptr is NULL: - raise RuntimeError( - "this instance hasn't been configured with a pointer for it " - "access retrieve data from" - ) - - # retrieve the pointer - cdef double* rate_ptr = grunstable_ratequery_get_ptr(self._ptr, rate_id) - - # lookup the shape of the rates - shape = self._try_get_shape(rate_id) - - # predeclare a memoryview to use with 1d arrays - cdef double[:] memview - - if shape == (): # handle the scalar case! - if update_rate: - rate_ptr[0] = val # cast performs type check - return float(rate_ptr[0]) - elif len(shape) == 1: - if update_rate: - raise RuntimeError( - f"You cannot assign a value to the {key!r} rate.\n\n" - "If you are looking to modify the rate's values, you " - f"should retrieve the {key!r} rate's value (a numpy " - "array), and modify the values in place" - ) - size = shape[0] - memview = (rate_ptr) - return np.asarray(memview) - else: - raise RuntimeError( - "no support is in place for higher dimensional arrays" - ) - def get(self, key, default=None, /): """ Retrieve the value associated with key, if key is known. Otherwise, return the default. """ try: - return self._access_rate(key, _NOSETVAL) + return self._get_rate(key) except: return default - def __getitem__(self, key): return self._access_rate(key, _NOSETVAL) + def __getitem__(self, key): return self._get_rate(key) - def __setitem__(self, key, value): self._access_rate(key, value) + def __setitem__(self, key, value): self._set_rate(key, value) def __iter__(self): return iter(self._name_rateid_map) From 144db1fa4240eceb47e469e56f3d4439c6f1e451 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 20 Dec 2025 11:34:05 -0500 Subject: [PATCH 33/40] remove remaining references to grunstable_ratequery_get_ptr --- src/clib/ratequery.cpp | 7 ------- src/include/grackle.h | 32 ------------------------------- tests/unit/test_api_ratequery.cpp | 6 ------ 3 files changed, 45 deletions(-) diff --git a/src/clib/ratequery.cpp b/src/clib/ratequery.cpp index 294b731ed..746fba471 100644 --- a/src/clib/ratequery.cpp +++ b/src/clib/ratequery.cpp @@ -325,13 +325,6 @@ extern "C" int grunstable_ratequery_set_f64(chemistry_data_storage* my_rates, return GR_SUCCESS; } -extern "C" double* grunstable_ratequery_get_ptr( - chemistry_data_storage* my_rates, grunstable_rateid_type rate_id) { - namespace rate_q = grackle::impl::ratequery; - - return rate_q::query_Entry(my_rates, rate_id, true).entry.data; -} - extern "C" int grunstable_ratequery_prop( const chemistry_data_storage* my_rates, grunstable_rateid_type rate_id, enum grunstable_ratequery_prop_kind prop_kind, long long* ptr) { diff --git a/src/include/grackle.h b/src/include/grackle.h index 5f513a184..34993b8ad 100644 --- a/src/include/grackle.h +++ b/src/include/grackle.h @@ -280,38 +280,6 @@ int grunstable_ratequery_set_f64( const double* buf ); -/// Access the pointer associated with the rateid from myrates -/// -/// The behavior is **NOT** currently defined if the `my_rates` struct isn't -/// configured to use the specified rate. -/// -/// @return The pointer to the accessed data (whether the rate data corresponds -/// to an array or scalar). If an invalid `rate_id` is specified, this -/// returns NULL. -/// -/// > [!note] -/// > Before stablizing, we should consider whether we actually want this -/// > function. It may be better to replace it with a getter (that copies the -/// > rate data to the supplied buffer) and a setter (that copies the provided -/// > data out of the provided buffer). This may be appealing for a number of -/// > reasons: -/// > 1. When we add GPU support, the rate data may live permanently on the GPU -/// > (or some data may live on the GPU while other data lives on the CPU). -/// > With dedicated setters/getters, we could always require that the -/// > provided buffer data is on the CPU. -/// > 2. We have the option to conditionally disable the setter. For example, -/// > if we choose to support custom rates, I assume we will want to specify -/// > all custom choices as part of initialization (for simplicity) and then -/// > we may want to deprecate/remove the setter. (One could also imagine, -/// > disabling the setter when using GPUs). -/// > 3. It eliminates a certain class of memory-lifetime bugs (People could -/// > try use the pointer returned by the incarnation of this function after -/// > destroying Grackle. This problem can't happen with the proposed -/// > getter/setter) -double* grunstable_ratequery_get_ptr( - chemistry_data_storage* my_rates, grunstable_rateid_type rate_id -); - /// Describe Rate-Query Property Types /// /// > [!note] diff --git a/tests/unit/test_api_ratequery.cpp b/tests/unit/test_api_ratequery.cpp index e7b6c5672..69c03b336 100644 --- a/tests/unit/test_api_ratequery.cpp +++ b/tests/unit/test_api_ratequery.cpp @@ -62,12 +62,6 @@ TEST_F(SimpleRateQueryTest, InvalidNameQuery) { EXPECT_EQ(rateid, get_invalid_rateid(pack)); } -TEST_F(SimpleRateQueryTest, PtrInvalidRateId) { - double* ptr = - grunstable_ratequery_get_ptr(pack.my_rates(), get_invalid_rateid(pack)); - EXPECT_EQ(ptr, nullptr); -} - // most of the remaining tests (and future planned tests) involve iterating // through all rates made available via the ratequery interface. To make the // tests themselves as easy to read as possible, we implate a C++-style From 5d6fb08a83aa7e4b1ed9ffe564939770d774de2f Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 13 Dec 2025 10:25:42 -0500 Subject: [PATCH 34/40] Rename `RecipeEntrySet` -> `EntrySet` and track `id_offset` separately --- src/clib/ratequery.cpp | 81 +++++++++++++++++++++--------------------- src/clib/ratequery.hpp | 25 +++++++++---- 2 files changed, 59 insertions(+), 47 deletions(-) diff --git a/src/clib/ratequery.cpp b/src/clib/ratequery.cpp index 746fba471..14825adc5 100644 --- a/src/clib/ratequery.cpp +++ b/src/clib/ratequery.cpp @@ -148,8 +148,9 @@ static Entry get_MiscRxn_Entry(chemistry_data_storage* my_rates, int i) { grackle::impl::ratequery::Registry grackle::impl::ratequery::new_Registry( const chemistry_data& my_chemistry) { if (my_chemistry.primordial_chemistry == 0) { - return Registry{0, nullptr}; + return Registry{0, 0, nullptr, nullptr}; } + // step 1: define several common EntryProps EntryProps props_LogTLinInterp = mk_invalid_EntryProps(); props_LogTLinInterp.ndim = 1; props_LogTLinInterp.shape[0] = my_chemistry.NumberOfTemperatureBins; @@ -162,24 +163,33 @@ grackle::impl::ratequery::Registry grackle::impl::ratequery::new_Registry( EntryProps props_scalar = mk_invalid_EntryProps(); props_scalar.ndim = 0; - const RecipeEntrySet standard_sets[] = { - {1000, CollisionalRxnLUT::NUM_ENTRIES, &get_CollisionalRxn_Entry, + // step 2: define standard entry recipies + const EntrySet standard_recipies[] = { + {CollisionalRxnLUT::NUM_ENTRIES, &get_CollisionalRxn_Entry, props_LogTLinInterp}, - {1999, 1, &get_k13dd_Entry, props_k13dd}, - {2000, MiscRxn_NRATES, &get_MiscRxn_Entry, props_scalar}}; - - int len = static_cast(sizeof(standard_sets) / sizeof(RecipeEntrySet)); - RecipeEntrySet* sets = new RecipeEntrySet[len]; - for (int i = 0; i < len; i++) { - sets[i] = standard_sets[i]; + {1, &get_k13dd_Entry, props_k13dd}, + {MiscRxn_NRATES, &get_MiscRxn_Entry, props_scalar}}; + + int n_sets = static_cast(sizeof(standard_recipies) / sizeof(EntrySet)); + + // step 3: actually set up Registry + EntrySet* sets = new EntrySet[n_sets]; + int* id_offsets = new int[n_sets]; + int tot_entry_count = 0; + for (int i = 0; i < n_sets; i++) { + id_offsets[i] = tot_entry_count; + sets[i] = standard_recipies[i]; + tot_entry_count += standard_recipies[i].len; } - return Registry{len, sets}; + return Registry{tot_entry_count, n_sets, id_offsets, sets}; } void grackle::impl::ratequery::drop_Registry( grackle::impl::ratequery::Registry* ptr) { if (ptr->sets != nullptr) { + delete[] ptr->id_offsets; + ptr->id_offsets = nullptr; delete[] ptr->sets; ptr->sets = nullptr; } @@ -204,33 +214,26 @@ static const Registry* get_registry(const chemistry_data_storage* my_rates) { } /// internal function to search for the rate description i -/// -/// We interpret i as rate_id, when use_rate_id is true. Otherwise, we just -/// look for the ith rate description (we introduce an artificial distinction -/// between the 2 cases because we want to reserve the right to be able to -/// change the relationship if it becomes convenient in the future) static ratequery_rslt_ query_Entry(chemistry_data_storage* my_rates, - long long i, bool use_rate_id) { + long long i) { const Registry* registry = get_registry(my_rates); - if (registry == nullptr) { + if (registry == nullptr || i >= registry->n_entries) { return invalid_rslt_(); } - int total_len = 0; // <- we increment this as we go through the rates - - for (int set_idx = 0; set_idx < registry->len; set_idx++) { - const struct RecipeEntrySet cur_set = registry->sets[set_idx]; + for (int set_idx = 0; set_idx < registry->n_sets; set_idx++) { + const struct EntrySet cur_set = registry->sets[set_idx]; + int cur_id_offset = registry->id_offsets[set_idx]; - const long long tmp = (use_rate_id) ? i - cur_set.id_offset : i - total_len; + const long long tmp = i - static_cast(cur_id_offset); if ((tmp >= 0) && (tmp < cur_set.len)) { ratequery_rslt_ out; - out.rate_id = tmp + cur_set.id_offset; - out.entry = cur_set.fn(my_rates, tmp); + out.rate_id = i; + out.entry = cur_set.recipe_fn(my_rates, tmp); out.entry.props = cur_set.common_props; return out; } - total_len += cur_set.len; } return invalid_rslt_(); } @@ -280,12 +283,13 @@ extern "C" grunstable_rateid_type grunstable_ratequery_id( return rate_q::UNDEFINED_RATE_ID_; } - for (int set_idx = 0; set_idx < registry->len; set_idx++) { - const rate_q::RecipeEntrySet set = registry->sets[set_idx]; - for (int i = 0; i < set.len; i++) { - rate_q::Entry entry = set.fn(nullptr, i); + for (int set_idx = 0; set_idx < registry->n_sets; set_idx++) { + const rate_q::EntrySet set = registry->sets[set_idx]; + int set_len = set.len; + for (int i = 0; i < set_len; i++) { + rate_q::Entry entry = set.recipe_fn(nullptr, i); if (std::strcmp(name, entry.name) == 0) { - return set.id_offset + i; + return registry->id_offsets[set_idx] + i; } } } @@ -296,7 +300,7 @@ extern "C" int grunstable_ratequery_get_f64(chemistry_data_storage* my_rates, grunstable_rateid_type rate_id, double* buf) { namespace rate_q = grackle::impl::ratequery; - rate_q::Entry entry = rate_q::query_Entry(my_rates, rate_id, true).entry; + rate_q::Entry entry = rate_q::query_Entry(my_rates, rate_id).entry; if (entry.data == nullptr) { // in this case, the query failed return GR_FAIL; @@ -313,7 +317,7 @@ extern "C" int grunstable_ratequery_set_f64(chemistry_data_storage* my_rates, grunstable_rateid_type rate_id, const double* buf) { namespace rate_q = grackle::impl::ratequery; - rate_q::Entry entry = rate_q::query_Entry(my_rates, rate_id, true).entry; + rate_q::Entry entry = rate_q::query_Entry(my_rates, rate_id).entry; if (entry.data == nullptr) { // in this case, the query failed return GR_FAIL; } @@ -333,7 +337,7 @@ extern "C" int grunstable_ratequery_prop( // short-term hack! (it's bad practice to "cast away the const") rate_q::Entry entry = rate_q::query_Entry(const_cast(my_rates), - rate_id, true) + rate_id) .entry; const rate_q::EntryProps& props = entry.props; @@ -369,7 +373,7 @@ extern "C" const char* grunstable_ith_rate( const long long sanitized_i = (i < LLONG_MAX) ? (long long)i : -1; // short-term hack! (it's bad practice to "cast away the const") rate_q::ratequery_rslt_ tmp = rate_q::query_Entry( - const_cast(my_rates), sanitized_i, false); + const_cast(my_rates), sanitized_i); if (out_rate_id != nullptr) { *out_rate_id = tmp.rate_id; } @@ -380,10 +384,5 @@ extern "C" unsigned long long grunstable_ratequery_nrates( const chemistry_data_storage* my_rates) { namespace rate_q = grackle::impl::ratequery; const rate_q::Registry* registry = my_rates->opaque_storage->registry; - - unsigned long long out = 0; - for (int i = 0; i < registry->len; i++) { - out += static_cast(registry->sets[i].len); - } - return out; + return registry->n_entries; } diff --git a/src/clib/ratequery.hpp b/src/clib/ratequery.hpp index 55b01c27f..2477f9919 100644 --- a/src/clib/ratequery.hpp +++ b/src/clib/ratequery.hpp @@ -97,18 +97,31 @@ inline Entry new_Entry(double* rate, const char* name) { /// satisfies `0 <= index <= (N-1)` typedef Entry fetch_Entry_recipe_fn(chemistry_data_storage*, int); -/// Describes the set of entries that can be accessed through a given recipe -struct RecipeEntrySet { - int id_offset; +/// Describes the set of entries that can be access through a given recipe +struct EntrySet { + /// number of entries in the current set int len; - fetch_Entry_recipe_fn* fn; + + /// a function pointer that can be used to access entries through a recipe + fetch_Entry_recipe_fn* recipe_fn; + + /// properties used by all entries accessed through a recipe + /// + /// In more detail, an entry returned by `recipe_fn` has its `props` member + /// overwritten by this value EntryProps common_props; }; /// Describes a registry of queryable entries struct Registry { - int len; - RecipeEntrySet* sets; + /// number of entries + int n_entries; + /// number of contained EntrySets + int n_sets; + /// stores the minimum rate_id for each EntrySet + int* id_offsets; + /// stores sets of entries + EntrySet* sets; }; /// construct a new registry From 5b7ed15ca8bd1d17029045a90f6c1b4febe142cb Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 20 Dec 2025 14:24:44 -0500 Subject: [PATCH 35/40] initial implementation of RegBuilder --- src/clib/ratequery.cpp | 83 ++++++++++++++++++++++++++++++++ src/clib/ratequery.hpp | 105 +++++++++++++++++++++++++++++++++++------ 2 files changed, 174 insertions(+), 14 deletions(-) diff --git a/src/clib/ratequery.cpp b/src/clib/ratequery.cpp index 14825adc5..3908f9225 100644 --- a/src/clib/ratequery.cpp +++ b/src/clib/ratequery.cpp @@ -85,6 +85,7 @@ static Entry get_CollisionalRxn_Entry(chemistry_data_storage* my_rates, int i) { return MKENTRY_STANDARD_KCOL_(my_rates, NAME, CollisionalRxnLUT::NAME); \ } #include "collisional_rxn_rate_members.def" + #undef ENTRY default: { return mk_invalid_Entry(); @@ -143,8 +144,90 @@ static Entry get_MiscRxn_Entry(chemistry_data_storage* my_rates, int i) { } } +/// resets the instance to the initial (empty) state. +/// +/// the skip_dealloc argument will only be true when the data is transferred +/// to a Registry +static void RegBuilder_reset_to_empty(RegBuilder* ptr, bool skip_dealloc) { + if ((ptr->capacity > 0) && !skip_dealloc) { + delete[] ptr->sets; + } + (*ptr) = new_RegBuilder(); +} + +static int RegBuilder_recipe_(RegBuilder* ptr, fetch_Entry_recipe_fn* recipe_fn, + int n_entries, EntryProps common_props) { + if (recipe_fn == nullptr) { + return GrPrintAndReturnErr("recipe_fn is a nullptr"); + } else if (n_entries <= 0) { + return GrPrintAndReturnErr("n_entries is not positive"); + } else if (!EntryProps_is_valid(common_props)) { + return GrPrintAndReturnErr("common_props isn't valid"); + } + + if (ptr->capacity == 0) { + ptr->capacity = 5; + ptr->sets = new EntrySet[ptr->capacity]; + } else if (ptr->len == ptr->capacity) { + // consider making this resizable in the future... + return GrPrintAndReturnErr("out of capacity"); + } + + ptr->sets[ptr->len++] = EntrySet{n_entries, recipe_fn, common_props}; + return GR_SUCCESS; +} + } // namespace grackle::impl::ratequery +void grackle::impl::ratequery::drop_RegBuilder(RegBuilder* ptr) { + RegBuilder_reset_to_empty(ptr, false); +} + +namespace grackle::impl::ratequery {} // namespace grackle::impl::ratequery + +int grackle::impl::ratequery::RegBuilder_recipe_scalar( + RegBuilder* ptr, int n_entries, fetch_Entry_recipe_fn* recipe_fn) { + EntryProps common_props = mk_invalid_EntryProps(); + common_props.ndim = 0; + return RegBuilder_recipe_(ptr, recipe_fn, n_entries, common_props); +} + +int grackle::impl::ratequery::RegBuilder_recipe_1d( + RegBuilder* ptr, int n_entries, fetch_Entry_recipe_fn* recipe_fn, + int common_len) { + EntryProps common_props = mk_invalid_EntryProps(); + common_props.ndim = 1; + common_props.shape[0] = common_len; + return RegBuilder_recipe_(ptr, recipe_fn, n_entries, common_props); +} + +/// build a new Registry. +/// +/// In the process, the current Registry is consumed; it's effectively reset to +/// the state immediately after it was initialized. (This lets us avoid +/// reallocating lots of memory) +grackle::impl::ratequery::Registry +grackle::impl::ratequery::RegBuilder_consume_and_build(RegBuilder* ptr) { + int n_sets = ptr->len; + if (n_sets == 0) { + drop_RegBuilder(ptr); + return Registry{0, n_sets, nullptr, nullptr}; + } else { + // set up id_offsets and determine the total number of entries + int* id_offsets = new int[n_sets]; + int tot_entry_count = 0; + for (int i = 0; i < n_sets; i++) { + id_offsets[i] = tot_entry_count; + tot_entry_count += ptr->sets[i].len; + } + Registry out{tot_entry_count, n_sets, id_offsets, ptr->sets}; + // reset to ptr to initial state (but don't deallocate since the ptr was + // transferred to out + RegBuilder_reset_to_empty(ptr, true); + return out; + } +} + grackle::impl::ratequery::Registry grackle::impl::ratequery::new_Registry( const chemistry_data& my_chemistry) { if (my_chemistry.primordial_chemistry == 0) { diff --git a/src/clib/ratequery.hpp b/src/clib/ratequery.hpp index 2477f9919..2c264839b 100644 --- a/src/clib/ratequery.hpp +++ b/src/clib/ratequery.hpp @@ -97,20 +97,8 @@ inline Entry new_Entry(double* rate, const char* name) { /// satisfies `0 <= index <= (N-1)` typedef Entry fetch_Entry_recipe_fn(chemistry_data_storage*, int); -/// Describes the set of entries that can be access through a given recipe -struct EntrySet { - /// number of entries in the current set - int len; - - /// a function pointer that can be used to access entries through a recipe - fetch_Entry_recipe_fn* recipe_fn; - - /// properties used by all entries accessed through a recipe - /// - /// In more detail, an entry returned by `recipe_fn` has its `props` member - /// overwritten by this value - EntryProps common_props; -}; +// temporary forward declaration +struct EntrySet; /// Describes a registry of queryable entries struct Registry { @@ -130,6 +118,95 @@ Registry new_Registry(const chemistry_data&); /// deallocate the contents of a registry void drop_Registry(Registry* ptr); +/// An interface for gradually configuring a Registry +/// +/// @par Context within the codebase +/// This is intended to be an ephemeral object that only lives during within +/// the function that initializes a Grackle solver from user-specified +/// parameters +/// - the instance is created at the start of this function +/// - a pointer to the instance is passed to various initialization functions. +/// These functions can use the instance to register entries that will be +/// accessible in the resulting Registry +/// - if all configuration has gone well, this instance will be used to +/// initialize the Registry +/// - the instance is **always** destroyed when it is time to exit the solver +/// initialization function +/// +/// @par Motivation +/// There are 2 main sources of motivation +/// 1. It lets us locate a "recipe-routine" for accessing data next to the +/// routines that initialize the same data. This makes sense from a +/// code-organization perspective. Moreover, if we conditionally initialize +/// data, it will be easier to ensure that recipies won't try to provide +/// access to the uninitialized data +/// 2. this will make it easier for us to handle data that Grackle only +/// initializes for the sake of supporting user queries (at the moment, +/// this is hypothetical) +/// +/// @important +/// Other parts of grackle should refrain from directly accessing the internals +/// of this function (i.e. they should only use the associated methods) +struct RegBuilder { + int capacity; + int len; + EntrySet* sets; +}; + +/// initialize a new instance +inline RegBuilder new_RegBuilder() { return {0, 0, nullptr}; } + +/// deallocates all storage within a RegBuilder instance +void drop_RegBuilder(RegBuilder* ptr); + +/// register a recipe for accessing scalar values +/// +/// @param[inout] ptr The RegBuilder that will be updated +/// @param[in] n_entries The number of entries accessible through the recipe +/// @param[in] recipe_fn The recipe being registered +/// @param[in] common_props The properties shared by each Entry in the recipe +/// +/// @returns GR_SUCCESS if successful, otherwise returns a different value +int RegBuilder_recipe_scalar(RegBuilder* ptr, int n_entries, + fetch_Entry_recipe_fn* recipe_fn); + +/// register a recipe for accessing 1D arrays +/// +/// @param[inout] ptr The RegBuilder that will be updated +/// @param[in] n_entries The number of entries accessible through the recipe +/// @param[in] recipe_fn The recipe being registered +/// @param[in] common_len The length shared by each 1D array accessible +/// through this recipe. +/// +/// @returns GR_SUCCESS if successful, otherwise returns a different value +int RegBuilder_recipe_1d(RegBuilder* ptr, int n_entries, + fetch_Entry_recipe_fn* recipe_fn, int common_len); + +/// build a new Registry. +/// +/// In the process, the current Registry is consumed; it's effectively reset to +/// the state immediately after it was initialized. (This lets us avoid +/// reallocating lots of memory) +/// +/// @note +/// For safety, the caller should still plan to call drop_RegBuilder +Registry RegBuilder_consume_and_build(RegBuilder* ptr); + +/// Describes the set of entries that can be access through a given recipe +struct EntrySet { + /// number of entries in the current set + int len; + + /// a function pointer that can be used to access entries through a recipe + fetch_Entry_recipe_fn* recipe_fn; + + /// properties used by all entries accessed through a recipe + /// + /// In more detail, an entry returned by `recipe_fn` has its `props` member + /// overwritten by this value + EntryProps common_props; +}; + /** @}*/ // end of group } // namespace grackle::impl::ratequery From 7baec78f8c15f0d6b82c349d135605765dd10d8d Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 20 Dec 2025 15:24:47 -0500 Subject: [PATCH 36/40] use RegistryBuilder within RegistryConstructor --- src/clib/ratequery.cpp | 48 ++++++++++++------------------------------ 1 file changed, 14 insertions(+), 34 deletions(-) diff --git a/src/clib/ratequery.cpp b/src/clib/ratequery.cpp index 3908f9225..0444e0f37 100644 --- a/src/clib/ratequery.cpp +++ b/src/clib/ratequery.cpp @@ -230,42 +230,22 @@ grackle::impl::ratequery::RegBuilder_consume_and_build(RegBuilder* ptr) { grackle::impl::ratequery::Registry grackle::impl::ratequery::new_Registry( const chemistry_data& my_chemistry) { - if (my_chemistry.primordial_chemistry == 0) { - return Registry{0, 0, nullptr, nullptr}; - } - // step 1: define several common EntryProps - EntryProps props_LogTLinInterp = mk_invalid_EntryProps(); - props_LogTLinInterp.ndim = 1; - props_LogTLinInterp.shape[0] = my_chemistry.NumberOfTemperatureBins; - - // maybe k13dd should be considered multi-dimensional? - EntryProps props_k13dd = mk_invalid_EntryProps(); - props_k13dd.ndim = 1; - props_k13dd.shape[0] = my_chemistry.NumberOfTemperatureBins * 14; - - EntryProps props_scalar = mk_invalid_EntryProps(); - props_scalar.ndim = 0; - - // step 2: define standard entry recipies - const EntrySet standard_recipies[] = { - {CollisionalRxnLUT::NUM_ENTRIES, &get_CollisionalRxn_Entry, - props_LogTLinInterp}, - {1, &get_k13dd_Entry, props_k13dd}, - {MiscRxn_NRATES, &get_MiscRxn_Entry, props_scalar}}; - - int n_sets = static_cast(sizeof(standard_recipies) / sizeof(EntrySet)); - - // step 3: actually set up Registry - EntrySet* sets = new EntrySet[n_sets]; - int* id_offsets = new int[n_sets]; - int tot_entry_count = 0; - for (int i = 0; i < n_sets; i++) { - id_offsets[i] = tot_entry_count; - sets[i] = standard_recipies[i]; - tot_entry_count += standard_recipies[i].len; + RegBuilder reg_builder = new_RegBuilder(); + if (my_chemistry.primordial_chemistry != 0) { + RegBuilder_recipe_1d(®_builder, CollisionalRxnLUT::NUM_ENTRIES, + &get_CollisionalRxn_Entry, + my_chemistry.NumberOfTemperatureBins); + + // maybe k13dd should be considered multi-dimensional? + RegBuilder_recipe_1d(®_builder, 1, &get_k13dd_Entry, + my_chemistry.NumberOfTemperatureBins * 14); + + RegBuilder_recipe_scalar(®_builder, MiscRxn_NRATES, &get_MiscRxn_Entry); } - return Registry{tot_entry_count, n_sets, id_offsets, sets}; + Registry out = RegBuilder_consume_and_build(®_builder); + drop_RegBuilder(®_builder); + return out; } void grackle::impl::ratequery::drop_Registry( From c5a7231cbe1e0509f5eec801970ed8d6e262b95e Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 20 Dec 2025 16:30:56 -0500 Subject: [PATCH 37/40] Remove new_Registry --- src/clib/initialize_chemistry_data.cpp | 29 +++++++++++++++--- src/clib/ratequery.cpp | 41 ++++++++++++-------------- src/clib/ratequery.hpp | 13 ++++++-- 3 files changed, 54 insertions(+), 29 deletions(-) diff --git a/src/clib/initialize_chemistry_data.cpp b/src/clib/initialize_chemistry_data.cpp index 95900c440..ec3eb2674 100644 --- a/src/clib/initialize_chemistry_data.cpp +++ b/src/clib/initialize_chemistry_data.cpp @@ -230,9 +230,15 @@ static void initialize_empty_chemistry_data_storage_struct(chemistry_data_storag my_rates->opaque_storage = NULL; } -extern "C" int local_initialize_chemistry_data(chemistry_data *my_chemistry, - chemistry_data_storage *my_rates, - code_units *my_units) +/// core logic of local_initialize_chemistry_data_ +/// +/// @note +/// This has been separated from local_initialize_chemistry_data to ensure that +/// any memory allocations tracked by reg_builder can be appropriately freed +/// (this is somewhat unavoidable in C++ without destructors) +static int local_initialize_chemistry_data_( + chemistry_data *my_chemistry, chemistry_data_storage *my_rates, + code_units *my_units, grackle::impl::ratequery::RegBuilder* reg_builder) { /* Better safe than sorry: Initialize everything to NULL/0 */ @@ -459,8 +465,9 @@ extern "C" int local_initialize_chemistry_data(chemistry_data *my_chemistry, my_rates->initial_units = *my_units; // initialize the registry + grackle::impl::ratequery::RegBuilder_misc_recipies(reg_builder, my_chemistry); my_rates->opaque_storage->registry = new grackle::impl::ratequery::Registry( - grackle::impl::ratequery::new_Registry(*my_chemistry) + grackle::impl::ratequery::RegBuilder_consume_and_build(reg_builder) ); if (grackle_verbose) { @@ -510,6 +517,20 @@ extern "C" int local_initialize_chemistry_data(chemistry_data *my_chemistry, return GR_SUCCESS; } + +extern "C" int local_initialize_chemistry_data(chemistry_data *my_chemistry, + chemistry_data_storage *my_rates, + code_units *my_units) +{ + namespace rate_q = grackle::impl::ratequery; + rate_q::RegBuilder reg_builder = rate_q::new_RegBuilder(); + + int out = local_initialize_chemistry_data_(my_chemistry, my_rates, my_units, + ®_builder); + rate_q::drop_RegBuilder(®_builder); + return out; +} + extern "C" int initialize_chemistry_data(code_units *my_units) { if (local_initialize_chemistry_data(grackle_data, &grackle_rates, diff --git a/src/clib/ratequery.cpp b/src/clib/ratequery.cpp index 0444e0f37..e8df8fbd7 100644 --- a/src/clib/ratequery.cpp +++ b/src/clib/ratequery.cpp @@ -41,7 +41,6 @@ // namespace grackle::impl::ratequery { - // we have reserved the right to change this value at any time enum { UNDEFINED_RATE_ID_ = 0 }; @@ -127,7 +126,7 @@ static Entry get_MiscRxn_Entry(chemistry_data_storage* my_rates, int i) { return MKENTRY_SCALAR_(my_rates, k25); case MiscRxn_k26: return MKENTRY_SCALAR_(my_rates, k26); - // Radiative rates for 9-species + // Radiative rates for 9-species case MiscRxn_k27: return MKENTRY_SCALAR_(my_rates, k27); case MiscRxn_k28: @@ -143,6 +142,24 @@ static Entry get_MiscRxn_Entry(chemistry_data_storage* my_rates, int i) { } } } +} // namespace grackle::impl::ratequery + +void grackle::impl::ratequery::RegBuilder_misc_recipies( + RegBuilder* ptr, const chemistry_data* my_chemistry) { + if (my_chemistry->primordial_chemistry != 0) { + RegBuilder_recipe_1d(ptr, CollisionalRxnLUT::NUM_ENTRIES, + &get_CollisionalRxn_Entry, + my_chemistry->NumberOfTemperatureBins); + + // maybe k13dd should be considered multi-dimensional? + RegBuilder_recipe_1d(ptr, 1, &get_k13dd_Entry, + my_chemistry->NumberOfTemperatureBins * 14); + + RegBuilder_recipe_scalar(ptr, MiscRxn_NRATES, &get_MiscRxn_Entry); + } +} + +namespace grackle::impl::ratequery { /// resets the instance to the initial (empty) state. /// @@ -228,26 +245,6 @@ grackle::impl::ratequery::RegBuilder_consume_and_build(RegBuilder* ptr) { } } -grackle::impl::ratequery::Registry grackle::impl::ratequery::new_Registry( - const chemistry_data& my_chemistry) { - RegBuilder reg_builder = new_RegBuilder(); - if (my_chemistry.primordial_chemistry != 0) { - RegBuilder_recipe_1d(®_builder, CollisionalRxnLUT::NUM_ENTRIES, - &get_CollisionalRxn_Entry, - my_chemistry.NumberOfTemperatureBins); - - // maybe k13dd should be considered multi-dimensional? - RegBuilder_recipe_1d(®_builder, 1, &get_k13dd_Entry, - my_chemistry.NumberOfTemperatureBins * 14); - - RegBuilder_recipe_scalar(®_builder, MiscRxn_NRATES, &get_MiscRxn_Entry); - } - - Registry out = RegBuilder_consume_and_build(®_builder); - drop_RegBuilder(®_builder); - return out; -} - void grackle::impl::ratequery::drop_Registry( grackle::impl::ratequery::Registry* ptr) { if (ptr->sets != nullptr) { diff --git a/src/clib/ratequery.hpp b/src/clib/ratequery.hpp index 2c264839b..ce3150646 100644 --- a/src/clib/ratequery.hpp +++ b/src/clib/ratequery.hpp @@ -101,6 +101,8 @@ typedef Entry fetch_Entry_recipe_fn(chemistry_data_storage*, int); struct EntrySet; /// Describes a registry of queryable entries +/// +/// @note This is constructed from a RegBuilder struct Registry { /// number of entries int n_entries; @@ -112,9 +114,6 @@ struct Registry { EntrySet* sets; }; -/// construct a new registry -Registry new_Registry(const chemistry_data&); - /// deallocate the contents of a registry void drop_Registry(Registry* ptr); @@ -182,6 +181,14 @@ int RegBuilder_recipe_scalar(RegBuilder* ptr, int n_entries, int RegBuilder_recipe_1d(RegBuilder* ptr, int n_entries, fetch_Entry_recipe_fn* recipe_fn, int common_len); +/// registers miscellaneous recipes +/// +/// @note +/// This is a hack until we can figure out a better spot to put definitions of +/// some miscellaneous rates +void RegBuilder_misc_recipies(RegBuilder* ptr, + const chemistry_data* my_chemistry); + /// build a new Registry. /// /// In the process, the current Registry is consumed; it's effectively reset to From de89e2b56692e92e02eec733784955e1ed89d5d3 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 20 Dec 2025 20:16:21 -0500 Subject: [PATCH 38/40] relocate and simplify get_CollisionalRxn_Entry --- src/clib/initialize_chemistry_data.cpp | 3 +- src/clib/initialize_rates.cpp | 59 +++++++++++++++++++++++++- src/clib/initialize_rates.hpp | 4 +- src/clib/ratequery.cpp | 49 ++------------------- src/clib/ratequery.hpp | 1 - 5 files changed, 66 insertions(+), 50 deletions(-) diff --git a/src/clib/initialize_chemistry_data.cpp b/src/clib/initialize_chemistry_data.cpp index ec3eb2674..f19669228 100644 --- a/src/clib/initialize_chemistry_data.cpp +++ b/src/clib/initialize_chemistry_data.cpp @@ -426,7 +426,8 @@ static int local_initialize_chemistry_data_( // Compute rate tables. if (grackle::impl::initialize_rates(my_chemistry, my_rates, my_units, - co_length_units, co_density_units) + co_length_units, co_density_units, + reg_builder) != GR_SUCCESS) { fprintf(stderr, "Error in initialize_rates.\n"); return GR_FAIL; diff --git a/src/clib/initialize_rates.cpp b/src/clib/initialize_rates.cpp index 18909b011..d33b13ee6 100644 --- a/src/clib/initialize_rates.cpp +++ b/src/clib/initialize_rates.cpp @@ -385,12 +385,60 @@ int init_kcol_rate_tables( return GR_SUCCESS; } + +/// a function that encodes the algorithm for looking up the `i`th pointer +/// collisional reaction rate from an instance of `chemistry_data_storage`. +/// +/// This is intended to be registered with RegBuilder in order to provide +/// access to the various collisional reaction rates. +/// +/// @param my_rates The object from which the rate Entry is loaded +/// @param i the index of the queried rate +grackle::impl::ratequery::Entry get_CollisionalRxn_Entry + (chemistry_data_storage* my_rates, int i) { + // sanity check! (this shouldn't actually happen) + if ((my_rates == nullptr) || (my_rates->opaque_storage == nullptr) || + (my_rates->opaque_storage->kcol_rate_tables == nullptr)) { + return grackle::impl::ratequery::mk_invalid_Entry(); + } + + // import new_Entry into the current scope (so we don't need the full name) + using ::grackle::impl::ratequery::new_Entry; + + double** data = my_rates->opaque_storage->kcol_rate_tables->data; + + // this implementation leverages the following properties of the + // CollisionalRxnLUT enum: + // - each entry of collisional_rxn_rate_members.def has a corresponding + // enumeration-constant + // - the very first enumeration constant has a value of 0 (since a value + // wasn't explicitly specified) + // - the value of each other enumeration constants is 1 larger than the value + // of the previous value (if a value isn't explicitly specified) + // - CollisionalRxnLUT::NUM_ENTRIES specifies the number of other enumeration + // constants (excluding CollisionalRxnLUT::NUM_ENTRIES) in the enum + switch (i) { +#define TO_STR(s) #s +#define ENTRY(NAME) \ + case CollisionalRxnLUT::NAME: { return new_Entry(data[i], TO_STR(NAME)); } +#include "collisional_rxn_rate_members.def" + +#undef ENTRY +#undef TO_STR + default: { + return grackle::impl::ratequery::mk_invalid_Entry(); + } + } +} + + } // anonymous namespace //Definition of the initialise_rates function. int grackle::impl::initialize_rates( chemistry_data *my_chemistry, chemistry_data_storage *my_rates, - code_units *my_units, double co_length_unit, double co_density_unit) + code_units *my_units, double co_length_unit, double co_density_unit, + ratequery::RegBuilder* reg_builder) { // TODO: we REALLY need to do an error check that // my_chemistry->NumberOfTemperatureBins >= 2 @@ -493,6 +541,15 @@ int grackle::impl::initialize_rates( return GR_FAIL; } + // register the recipe for looking up the "standard" collisional rates + if (grackle::impl::ratequery::RegBuilder_recipe_1d( + reg_builder, CollisionalRxnLUT::NUM_ENTRIES, + &get_CollisionalRxn_Entry, my_chemistry->NumberOfTemperatureBins + ) != GR_SUCCESS) { + return GrPrintAndReturnErr("error registering standard collisional " + "reaction rates"); + } + //--------Calculate coefficients for density-dependent collisional H2 dissociation rate-------- // // idt = 0 calculates coefficients for direct collisional dissociation idt = 1 diff --git a/src/clib/initialize_rates.hpp b/src/clib/initialize_rates.hpp index 0a004895c..99e2e9b21 100644 --- a/src/clib/initialize_rates.hpp +++ b/src/clib/initialize_rates.hpp @@ -14,12 +14,14 @@ #define INITIALIZE_RATES_HPP #include "grackle.h" +#include "ratequery.hpp" namespace grackle::impl { int initialize_rates(chemistry_data* my_chemistry, chemistry_data_storage* my_rates, code_units* my_units, - double co_length_unit, double co_density_unit); + double co_length_unit, double co_density_unit, + ratequery::RegBuilder* reg_builder); } // namespace grackle::impl diff --git a/src/clib/ratequery.cpp b/src/clib/ratequery.cpp index e8df8fbd7..433867c92 100644 --- a/src/clib/ratequery.cpp +++ b/src/clib/ratequery.cpp @@ -46,51 +46,10 @@ enum { UNDEFINED_RATE_ID_ = 0 }; // introduce some basic machinery to help us implement dynamic lookup of rates -static Entry new_Entry_standard_kcol_(chemistry_data_storage* my_rates, - const char* name, int index) { - if ((my_rates == nullptr) || (my_rates->opaque_storage == nullptr) || - (my_rates->opaque_storage->kcol_rate_tables == nullptr)) { - return new_Entry(nullptr, name); - } else { - return new_Entry(my_rates->opaque_storage->kcol_rate_tables->data[index], - name); - } -} - #define MKENTRY_(PTR, NAME, FAMILY) \ new_Entry(((PTR) == nullptr) ? nullptr : (PTR)->NAME, #NAME, FAMILY) #define MKENTRY_SCALAR_(PTR, NAME) \ new_Entry(((PTR) == nullptr) ? nullptr : &((PTR)->NAME), #NAME) -#define MKENTRY_STANDARD_KCOL_(PTR, NAME, INDEX) \ - new_Entry_standard_kcol_(PTR, #NAME, INDEX) - -// Create machinery to lookup Standard-Form Collisional Reaction Rates -// ------------------------------------------------------------------- -// see the next section for other macros - -// this fn leverages the following properties of the CollisionalRxnLUT enum: -// - each entry of collisional_rxn_rate_members.def has a corresponding -// enumeration-constant -// - the very first enumeration constant has a value of 0 (since a value wasn't -// explicitly specified) -// - the value of each other enumeration constants is 1 larger than the value -// of the previous value (if a value isn't explicitly specified) -// - CollisionalRxnLUT::NUM_ENTRIES specifies the number of other enumeration -// constants (excluding CollisionalRxnLUT::NUM_ENTRIES) in the enum -static Entry get_CollisionalRxn_Entry(chemistry_data_storage* my_rates, int i) { - switch (i) { -#define ENTRY(NAME) \ - case CollisionalRxnLUT::NAME: { \ - return MKENTRY_STANDARD_KCOL_(my_rates, NAME, CollisionalRxnLUT::NAME); \ - } -#include "collisional_rxn_rate_members.def" - -#undef ENTRY - default: { - return mk_invalid_Entry(); - } - } -} static Entry get_k13dd_Entry(chemistry_data_storage* my_rates, int i) { if (i == 0) { @@ -147,10 +106,6 @@ static Entry get_MiscRxn_Entry(chemistry_data_storage* my_rates, int i) { void grackle::impl::ratequery::RegBuilder_misc_recipies( RegBuilder* ptr, const chemistry_data* my_chemistry) { if (my_chemistry->primordial_chemistry != 0) { - RegBuilder_recipe_1d(ptr, CollisionalRxnLUT::NUM_ENTRIES, - &get_CollisionalRxn_Entry, - my_chemistry->NumberOfTemperatureBins); - // maybe k13dd should be considered multi-dimensional? RegBuilder_recipe_1d(ptr, 1, &get_k13dd_Entry, my_chemistry->NumberOfTemperatureBins * 14); @@ -347,7 +302,9 @@ extern "C" grunstable_rateid_type grunstable_ratequery_id( const rate_q::EntrySet set = registry->sets[set_idx]; int set_len = set.len; for (int i = 0; i < set_len; i++) { - rate_q::Entry entry = set.recipe_fn(nullptr, i); + // short-term hack! (it's bad practice to "cast away the const") + rate_q::Entry entry = + set.recipe_fn(const_cast(my_rates), i); if (std::strcmp(name, entry.name) == 0) { return registry->id_offsets[set_idx] + i; } diff --git a/src/clib/ratequery.hpp b/src/clib/ratequery.hpp index ce3150646..0397405c9 100644 --- a/src/clib/ratequery.hpp +++ b/src/clib/ratequery.hpp @@ -163,7 +163,6 @@ void drop_RegBuilder(RegBuilder* ptr); /// @param[inout] ptr The RegBuilder that will be updated /// @param[in] n_entries The number of entries accessible through the recipe /// @param[in] recipe_fn The recipe being registered -/// @param[in] common_props The properties shared by each Entry in the recipe /// /// @returns GR_SUCCESS if successful, otherwise returns a different value int RegBuilder_recipe_scalar(RegBuilder* ptr, int n_entries, From ef5b1be6f7c30cf1c8c925362d540d1f8a33478a Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 20 Dec 2025 20:36:21 -0500 Subject: [PATCH 39/40] assorted cleanup --- src/clib/initialize_chemistry_data.cpp | 7 +++- src/clib/initialize_rates.cpp | 27 +++++++++++++++ src/clib/ratequery.cpp | 46 +++++++++----------------- src/clib/ratequery.hpp | 4 +-- 4 files changed, 51 insertions(+), 33 deletions(-) diff --git a/src/clib/initialize_chemistry_data.cpp b/src/clib/initialize_chemistry_data.cpp index f19669228..0a196abe2 100644 --- a/src/clib/initialize_chemistry_data.cpp +++ b/src/clib/initialize_chemistry_data.cpp @@ -29,6 +29,7 @@ #include "opaque_storage.hpp" // gr_opaque_storage #include "phys_constants.h" #include "ratequery.hpp" +#include "status_reporting.h" #ifdef _OPENMP #include @@ -466,7 +467,11 @@ static int local_initialize_chemistry_data_( my_rates->initial_units = *my_units; // initialize the registry - grackle::impl::ratequery::RegBuilder_misc_recipies(reg_builder, my_chemistry); + if (grackle::impl::ratequery::RegBuilder_misc_recipies(reg_builder, + my_chemistry) + != GR_SUCCESS){ + return GrPrintAndReturnErr("error in RegBuilder_misc_recipies"); + } my_rates->opaque_storage->registry = new grackle::impl::ratequery::Registry( grackle::impl::ratequery::RegBuilder_consume_and_build(reg_builder) ); diff --git a/src/clib/initialize_rates.cpp b/src/clib/initialize_rates.cpp index d33b13ee6..a5198b5d4 100644 --- a/src/clib/initialize_rates.cpp +++ b/src/clib/initialize_rates.cpp @@ -432,6 +432,25 @@ grackle::impl::ratequery::Entry get_CollisionalRxn_Entry } +/// a function that encodes the algorithm for looking up the k13dd +/// collisional reaction rate from an instance of `chemistry_data_storage`. +/// +/// This is intended to be registered with RegBuilder in order to provide +/// access to the various collisional reaction rates. +/// +/// @param my_rates The object from which the rate Entry is loaded +/// @param i the index of the queried rate +grackle::impl::ratequery::Entry get_k13dd_Entry( + chemistry_data_storage* my_rates, int i) { + if (i == 0) { + double* ptr = (my_rates == nullptr) ? nullptr : my_rates->k13dd; + return grackle::impl::ratequery::new_Entry(ptr, "k13dd"); + } else { + return grackle::impl::ratequery::mk_invalid_Entry(); + } +} + + } // anonymous namespace //Definition of the initialise_rates function. @@ -565,6 +584,14 @@ int grackle::impl::initialize_rates( // coeff7(idt=0, TbinFinal), coeff1(idt=1, Tbin1), ..., coeff7(idt=1, TbinFinal)} add_k13dd_reaction_rate(&my_rates->k13dd, kUnit, my_chemistry); + // maybe k13dd should be considered multi-dimensional? + if (grackle::impl::ratequery::RegBuilder_recipe_1d( + reg_builder, 1, &get_k13dd_Entry, + my_chemistry->NumberOfTemperatureBins * 14) != GR_SUCCESS) { + return GrPrintAndReturnErr("error registering k13dd rate"); + } + + //H2 formation on dust grains requires loop over the dust temperature. add_h2dust_reaction_rate(&my_rates->h2dust, kUnit, my_chemistry); diff --git a/src/clib/ratequery.cpp b/src/clib/ratequery.cpp index 433867c92..73a02a3d9 100644 --- a/src/clib/ratequery.cpp +++ b/src/clib/ratequery.cpp @@ -44,24 +44,10 @@ namespace grackle::impl::ratequery { // we have reserved the right to change this value at any time enum { UNDEFINED_RATE_ID_ = 0 }; -// introduce some basic machinery to help us implement dynamic lookup of rates - -#define MKENTRY_(PTR, NAME, FAMILY) \ - new_Entry(((PTR) == nullptr) ? nullptr : (PTR)->NAME, #NAME, FAMILY) -#define MKENTRY_SCALAR_(PTR, NAME) \ - new_Entry(((PTR) == nullptr) ? nullptr : &((PTR)->NAME), #NAME) - -static Entry get_k13dd_Entry(chemistry_data_storage* my_rates, int i) { - if (i == 0) { - double* ptr = (my_rates == nullptr) ? nullptr : my_rates->k13dd; - return new_Entry(ptr, "k13dd"); - } else { - return mk_invalid_Entry(); - } -} - // Create machinery to lookup Other Miscellaneous Rates // ---------------------------------------------------- +// -> ideally this should get relocated to a more sensible location (but not +// sure where that is... enum MiscRxnRateKind_ { MiscRxn_k24, @@ -77,25 +63,28 @@ enum MiscRxnRateKind_ { }; static Entry get_MiscRxn_Entry(chemistry_data_storage* my_rates, int i) { + if (my_rates == nullptr) { // <- shouldn't come up + return mk_invalid_Entry(); + } switch (i) { // Radiative rates for 6-species (for external field): case MiscRxn_k24: - return MKENTRY_SCALAR_(my_rates, k24); + return new_Entry(&my_rates->k24, "k24"); case MiscRxn_k25: - return MKENTRY_SCALAR_(my_rates, k25); + return new_Entry(&my_rates->k25, "k25"); case MiscRxn_k26: - return MKENTRY_SCALAR_(my_rates, k26); + return new_Entry(&my_rates->k26, "k26"); // Radiative rates for 9-species case MiscRxn_k27: - return MKENTRY_SCALAR_(my_rates, k27); + return new_Entry(&my_rates->k27, "k27"); case MiscRxn_k28: - return MKENTRY_SCALAR_(my_rates, k28); + return new_Entry(&my_rates->k28, "k28"); case MiscRxn_k29: - return MKENTRY_SCALAR_(my_rates, k29); + return new_Entry(&my_rates->k29, "k29"); case MiscRxn_k30: - return MKENTRY_SCALAR_(my_rates, k30); + return new_Entry(&my_rates->k30, "k30"); case MiscRxn_k31: - return MKENTRY_SCALAR_(my_rates, k31); + return new_Entry(&my_rates->k31, "k31"); default: { return mk_invalid_Entry(); } @@ -103,15 +92,12 @@ static Entry get_MiscRxn_Entry(chemistry_data_storage* my_rates, int i) { } } // namespace grackle::impl::ratequery -void grackle::impl::ratequery::RegBuilder_misc_recipies( +int grackle::impl::ratequery::RegBuilder_misc_recipies( RegBuilder* ptr, const chemistry_data* my_chemistry) { if (my_chemistry->primordial_chemistry != 0) { - // maybe k13dd should be considered multi-dimensional? - RegBuilder_recipe_1d(ptr, 1, &get_k13dd_Entry, - my_chemistry->NumberOfTemperatureBins * 14); - - RegBuilder_recipe_scalar(ptr, MiscRxn_NRATES, &get_MiscRxn_Entry); + return RegBuilder_recipe_scalar(ptr, MiscRxn_NRATES, &get_MiscRxn_Entry); } + return GR_SUCCESS; } namespace grackle::impl::ratequery { diff --git a/src/clib/ratequery.hpp b/src/clib/ratequery.hpp index 0397405c9..a5f3d7f55 100644 --- a/src/clib/ratequery.hpp +++ b/src/clib/ratequery.hpp @@ -185,8 +185,8 @@ int RegBuilder_recipe_1d(RegBuilder* ptr, int n_entries, /// @note /// This is a hack until we can figure out a better spot to put definitions of /// some miscellaneous rates -void RegBuilder_misc_recipies(RegBuilder* ptr, - const chemistry_data* my_chemistry); +int RegBuilder_misc_recipies(RegBuilder* ptr, + const chemistry_data* my_chemistry); /// build a new Registry. /// From a2665b21c0bc8f357ffb4fadaf84827acb841dea Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sun, 28 Dec 2025 13:39:32 -0500 Subject: [PATCH 40/40] address a typo in a docstring --- src/clib/ratequery.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/clib/ratequery.hpp b/src/clib/ratequery.hpp index a5f3d7f55..972102203 100644 --- a/src/clib/ratequery.hpp +++ b/src/clib/ratequery.hpp @@ -54,7 +54,7 @@ namespace grackle::impl::ratequery { /// initialization, we could consider making construction of the registry (or /// non-essential registry-entries) a runtime parameter. /// -/// ASIDE: The current design is very much prioritizes doing something easy +/// ASIDE: The current design very much prioritizes doing something easy /// over runtime performance. /** @{ */