From 21c4d292fed4268f340db9154cfef279f9f94924 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 22 Jan 2026 09:55:14 -0500 Subject: [PATCH 01/29] Shift definition GrackleCtxPack to its own file --- tests/grtestutils/CMakeLists.txt | 1 + tests/grtestutils/GrackleCtxPack.hpp | 100 ++++++++++++++++++++++ tests/grtestutils/googletest/fixtures.hpp | 3 +- tests/grtestutils/iterator_adaptor.hpp | 2 +- tests/grtestutils/preset.hpp | 80 ----------------- 5 files changed, 104 insertions(+), 82 deletions(-) create mode 100644 tests/grtestutils/GrackleCtxPack.hpp diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index 27bf11d17..3915849f9 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 + GrackleCtxPack.hpp iterator_adaptor.hpp os.hpp os.cpp preset.hpp preset.cpp diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/GrackleCtxPack.hpp new file mode 100644 index 000000000..bce9abbca --- /dev/null +++ b/tests/grtestutils/GrackleCtxPack.hpp @@ -0,0 +1,100 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Declare the GrackleCtxType +/// +//===----------------------------------------------------------------------===// +#ifndef GRTESTUTILS_GRACKLECTXPACK_HPP +#define GRTESTUTILS_GRACKLECTXPACK_HPP + +#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 // GRTESTUTILS_GRACKLECTXPACK_HPP diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index 05f1ccb97..fc72c4a3a 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -6,7 +6,7 @@ //===----------------------------------------------------------------------===// /// /// @file -/// Define machinery for creating GoogleTest Fixutures to help test Grackle's +/// Define machinery for creating GoogleTest Fixtures to help test Grackle's /// C API. /// //===----------------------------------------------------------------------===// @@ -18,6 +18,7 @@ // 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 "../GrackleCtxPack.hpp" #include "../preset.hpp" namespace grtest { diff --git a/tests/grtestutils/iterator_adaptor.hpp b/tests/grtestutils/iterator_adaptor.hpp index ad84a3559..c992b9490 100644 --- a/tests/grtestutils/iterator_adaptor.hpp +++ b/tests/grtestutils/iterator_adaptor.hpp @@ -16,7 +16,7 @@ #include #include "grackle.h" -#include "preset.hpp" +#include "GrackleCtxPack.hpp" namespace grtest { diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp index 919429a09..b1278d175 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -63,86 +63,6 @@ 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 From 2da8d074ddfee8d03d70a7c4f4085217a9762c2a Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 22 Jan 2026 13:28:10 -0500 Subject: [PATCH 02/29] add wrappers to set chemistry_data params --- tests/grtestutils/CMakeLists.txt | 2 +- tests/grtestutils/GrackleCtxPack.cpp | 58 +++++++++++++++++++ tests/grtestutils/GrackleCtxPack.hpp | 86 ++++++++++++++++++++++++++++ 3 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 tests/grtestutils/GrackleCtxPack.cpp diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index 3915849f9..586f164e1 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -80,7 +80,7 @@ add_library(grtestutils # files outside of the googletest subdirectory # -> these shouldn't include headers from the googletest subdirectory cmd.hpp cmd.cpp - GrackleCtxPack.hpp + GrackleCtxPack.hpp GrackleCtxPack.cpp iterator_adaptor.hpp os.hpp os.cpp preset.hpp preset.cpp diff --git a/tests/grtestutils/GrackleCtxPack.cpp b/tests/grtestutils/GrackleCtxPack.cpp new file mode 100644 index 000000000..beed5be30 --- /dev/null +++ b/tests/grtestutils/GrackleCtxPack.cpp @@ -0,0 +1,58 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Implement logic pertaining to GrackleCtxPack +/// +//===----------------------------------------------------------------------===// + +#include "GrackleCtxPack.hpp" +#include "status_reporting.h" + +#include + +namespace grtest::param_detail { + +static bool set_str_(chemistry_data& my_chem, const std::string& name, + const char* val, std::size_t sz_with_nul, + StrAllocTracker* str_allocs) { + // NOTE: we should NOT directly modify characters held by field_ptr + const char** dest = const_cast( + local_chemistry_data_access_string(&my_chem, name.c_str())); + if (dest == nullptr) { + return false; // field is either not known or not a string + } + + // the following acts as a compiler hint (to suppress a warning) + GR_INTERNAL_REQUIRE((val == nullptr) != (sz_with_nul == 0), "compiler-hint"); + + if (str_allocs != nullptr) { + // (if applicable) allocate a new buffer and deallocate the old buffer + char* new_alloc = str_allocs->alloc_buf_and_free_old(sz_with_nul, *dest); + if (sz_with_nul > 0) { + std::memcpy(new_alloc, val, sz_with_nul - 1); + new_alloc[sz_with_nul - 1] = '\0'; + } + (*dest) = new_alloc; + } else { + (*dest) = val; + } + return true; +} + +bool set_str(chemistry_data& my_chem, const std::string& name, const char* val, + StrAllocTracker* str_allocs) { + std::size_t sz_with_nul = (val == nullptr) ? 0 : std::strlen(val) + 1; + return set_str_(my_chem, name, val, sz_with_nul, str_allocs); +} + +bool set_str(chemistry_data& my_chem, const std::string& name, + const std::string val, StrAllocTracker* str_allocs) { + return set_str_(my_chem, name, val.data(), val.size() + 1, str_allocs); +} + +} // namespace grtest::param_detail \ No newline at end of file diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/GrackleCtxPack.hpp index bce9abbca..db8b58f39 100644 --- a/tests/grtestutils/GrackleCtxPack.hpp +++ b/tests/grtestutils/GrackleCtxPack.hpp @@ -13,8 +13,94 @@ #define GRTESTUTILS_GRACKLECTXPACK_HPP #include "preset.hpp" +#include // std::remove_if +#include // std::unique_ptr namespace grtest { + +namespace param_detail { + +/// Tracks allocations of string parameters stored by a @ref chemistry_data +/// +/// The basic premise is that when you construct a @ref chemistry_data instance, +/// you would also construct a StrAllocTracker instance that has the same +/// lifetime as @ref chemistry_data. And then you would pass both instances to +/// @ref set_str when you want to store a string parameter +/// +/// @par Implementation Notes +/// * We explicitly avoid tracking the allocations within this type using +/// std::string because SSO (small string optimization) could cause weird +/// bugs in the future during refactoring if we aren't super careful +/// +/// * We assume max number of strings is small (i.e. <=5). If the number grows, +/// use std::unordered_set instead of std::vector +class StrAllocTracker { + /// manages the lifetime of string allocations + std::vector> bufs_; + +public: + /// allocate a new buffer and try to free the old buffer + /// + /// @param tot_len Total length of new buffer (including the nul terminator) + /// @param old The old buffer that we are trying to replace + char* alloc_buf_and_free_old(std::size_t tot_len, const char* old) { + // erase the entry in bufs_ holding old (this also frees memory). If old + // isn't found, it's a nullptr or we assume that it's a string literal + for (std::size_t i = 0; i < bufs_.size(); i++) { + if (bufs_[i].get() == old) { + bufs_.erase(bufs_.begin() + i); + break; + } + } + + if (tot_len == 0) { + return nullptr; + } + const std::unique_ptr& p = bufs_.emplace_back(new char[tot_len]); + return p.get(); + } +}; + +/// Tries to set a string parameter tracked by @p my_param +/// +/// @param[in,out] my_chem Tracks various Grackle parameters +/// @param[in] name The name of the parameter getting updated +/// @param[in] val The value of the parameter +/// @param[in] str_allocs Tracks allocations of the strings held by @p my_chem +/// @returns true if successful and `false` if there was an error (e.g. @p name +/// isn't a known parameter or is a parameter that doesn't expect a string) +/// +/// @note +/// Oridinarily, this function will update @p str_allocs to hold a copy of +/// @p val and @p my_chem will be updated to store a pointer to that copy. When +/// @p str_allocs is a nullptr, @p my_chem is directly updated to track the data +/// referenced by @p val. +bool set_str(chemistry_data& my_chem, const std::string& name, const char* val, + StrAllocTracker* str_allocs); +bool set_str(chemistry_data& my_chem, const std::string& name, + const std::string& val, StrAllocTracker* str_allocs); + +inline bool set_int(chemistry_data& my_chem, const std::string& name, int val) { + int* dest = local_chemistry_data_access_int(&my_chem, name.c_str()); + if (dest == nullptr) { + return false; + } + (*dest) = val; + return true; +} + +inline bool set_double(chemistry_data& my_chem, const std::string& name, + double val) { + double* dest = local_chemistry_data_access_double(&my_chem, name.c_str()); + if (dest == nullptr) { + return false; + } + (*dest) = val; + return true; +} + +} // namespace param_detail + /// Tracks the group of Grackle objects needed for executing API functions /// /// The primary motivation for this object's existence is making sure that From 774b3057d9e1875af682c3d56601db0435097f30 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 22 Jan 2026 15:11:29 -0500 Subject: [PATCH 03/29] grtestutils: chemistry_data parameter machinery --- tests/grtestutils/CMakeLists.txt | 3 +- tests/grtestutils/GrackleCtxPack.cpp | 58 --------- tests/grtestutils/GrackleCtxPack.hpp | 37 ------ tests/grtestutils/param.cpp | 179 +++++++++++++++++++++++++++ tests/grtestutils/param.hpp | 171 +++++++++++++++++++++++++ 5 files changed, 352 insertions(+), 96 deletions(-) delete mode 100644 tests/grtestutils/GrackleCtxPack.cpp create mode 100644 tests/grtestutils/param.cpp create mode 100644 tests/grtestutils/param.hpp diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index 586f164e1..bf12b77f4 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -80,9 +80,10 @@ add_library(grtestutils # files outside of the googletest subdirectory # -> these shouldn't include headers from the googletest subdirectory cmd.hpp cmd.cpp - GrackleCtxPack.hpp GrackleCtxPack.cpp + GrackleCtxPack.hpp iterator_adaptor.hpp os.hpp os.cpp + param.hpp param.cpp preset.hpp preset.cpp utils.hpp utils.cpp diff --git a/tests/grtestutils/GrackleCtxPack.cpp b/tests/grtestutils/GrackleCtxPack.cpp deleted file mode 100644 index beed5be30..000000000 --- a/tests/grtestutils/GrackleCtxPack.cpp +++ /dev/null @@ -1,58 +0,0 @@ -//===----------------------------------------------------------------------===// -// -// See the LICENSE file for license and copyright information -// SPDX-License-Identifier: NCSA AND BSD-3-Clause -// -//===----------------------------------------------------------------------===// -/// -/// @file -/// Implement logic pertaining to GrackleCtxPack -/// -//===----------------------------------------------------------------------===// - -#include "GrackleCtxPack.hpp" -#include "status_reporting.h" - -#include - -namespace grtest::param_detail { - -static bool set_str_(chemistry_data& my_chem, const std::string& name, - const char* val, std::size_t sz_with_nul, - StrAllocTracker* str_allocs) { - // NOTE: we should NOT directly modify characters held by field_ptr - const char** dest = const_cast( - local_chemistry_data_access_string(&my_chem, name.c_str())); - if (dest == nullptr) { - return false; // field is either not known or not a string - } - - // the following acts as a compiler hint (to suppress a warning) - GR_INTERNAL_REQUIRE((val == nullptr) != (sz_with_nul == 0), "compiler-hint"); - - if (str_allocs != nullptr) { - // (if applicable) allocate a new buffer and deallocate the old buffer - char* new_alloc = str_allocs->alloc_buf_and_free_old(sz_with_nul, *dest); - if (sz_with_nul > 0) { - std::memcpy(new_alloc, val, sz_with_nul - 1); - new_alloc[sz_with_nul - 1] = '\0'; - } - (*dest) = new_alloc; - } else { - (*dest) = val; - } - return true; -} - -bool set_str(chemistry_data& my_chem, const std::string& name, const char* val, - StrAllocTracker* str_allocs) { - std::size_t sz_with_nul = (val == nullptr) ? 0 : std::strlen(val) + 1; - return set_str_(my_chem, name, val, sz_with_nul, str_allocs); -} - -bool set_str(chemistry_data& my_chem, const std::string& name, - const std::string val, StrAllocTracker* str_allocs) { - return set_str_(my_chem, name, val.data(), val.size() + 1, str_allocs); -} - -} // namespace grtest::param_detail \ No newline at end of file diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/GrackleCtxPack.hpp index db8b58f39..2fca9d7de 100644 --- a/tests/grtestutils/GrackleCtxPack.hpp +++ b/tests/grtestutils/GrackleCtxPack.hpp @@ -61,43 +61,6 @@ class StrAllocTracker { } }; -/// Tries to set a string parameter tracked by @p my_param -/// -/// @param[in,out] my_chem Tracks various Grackle parameters -/// @param[in] name The name of the parameter getting updated -/// @param[in] val The value of the parameter -/// @param[in] str_allocs Tracks allocations of the strings held by @p my_chem -/// @returns true if successful and `false` if there was an error (e.g. @p name -/// isn't a known parameter or is a parameter that doesn't expect a string) -/// -/// @note -/// Oridinarily, this function will update @p str_allocs to hold a copy of -/// @p val and @p my_chem will be updated to store a pointer to that copy. When -/// @p str_allocs is a nullptr, @p my_chem is directly updated to track the data -/// referenced by @p val. -bool set_str(chemistry_data& my_chem, const std::string& name, const char* val, - StrAllocTracker* str_allocs); -bool set_str(chemistry_data& my_chem, const std::string& name, - const std::string& val, StrAllocTracker* str_allocs); - -inline bool set_int(chemistry_data& my_chem, const std::string& name, int val) { - int* dest = local_chemistry_data_access_int(&my_chem, name.c_str()); - if (dest == nullptr) { - return false; - } - (*dest) = val; - return true; -} - -inline bool set_double(chemistry_data& my_chem, const std::string& name, - double val) { - double* dest = local_chemistry_data_access_double(&my_chem, name.c_str()); - if (dest == nullptr) { - return false; - } - (*dest) = val; - return true; -} } // namespace param_detail diff --git a/tests/grtestutils/param.cpp b/tests/grtestutils/param.cpp new file mode 100644 index 000000000..054262152 --- /dev/null +++ b/tests/grtestutils/param.cpp @@ -0,0 +1,179 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Logic pertaining to ParamVal +/// +//===----------------------------------------------------------------------===// +#include "status_reporting.h" + +#include "param.hpp" +#include "GrackleCtxPack.hpp" // param_detail::StrAllocTracker +#include // std::strlen +#include // std::ostream +#include // std::false_type, std::is_same_v, std::decay_t + +namespace { // stuff inside an anonymous namespace is local to this file +// this is used to report a compile-time error in the lambda function that +// we use in std::visit (this is standard practice) +template +constexpr std::false_type always_false_{}; +} // anonymous namespace + +namespace grtest { + +std::string ParamVal::to_string() const { + std::string tmp; + // this lambda function is passed a reference to the value within this->val_ + auto get_string_repr_ = [&tmp](const auto& v) -> void { + using T = std::decay_t; + if constexpr (std::is_same_v || std::is_same_v) { + tmp = std::to_string(v); + } else if constexpr (std::is_same_v) { + tmp.reserve(v.size() + 2); + tmp += '"'; + tmp += v; + tmp += '"'; + } else if constexpr (std::is_same_v) { + tmp = "nullptr"; + } else { + static_assert(always_false_, "encountered unhandled type"); + } + }; + std::visit(get_string_repr_, this->val_); + + std::string out; + out.reserve(tmp.size() + 10); + out = "ParamVal("; + out += tmp; + out += ")"; + return out; +} + +void PrintTo(const ParamVal& p, std::ostream* os) { *os << p.to_string(); } + +bool ParamVal::is_equal(int val) const { + return std::holds_alternative(val_) && val == std::get(val_); +} + +bool ParamVal::is_equal(double val) const { + return std::holds_alternative(val_) && val == std::get(val_); +} + +bool ParamVal::is_equal(std::string_view val) const { + return std::holds_alternative(val_) && + val == std::get(val_); +} + +bool ParamVal::is_equal(const char* val) const { + if (val == nullptr) { + return std::holds_alternative(val_); + } + return is_equal(std::string_view(val)); +} + +namespace { // stuff inside an anonymous namespace is local to this file + +bool set_str_(chemistry_data& my_chem, const std::string& name, const char* val, + std::size_t sz_with_nul, + param_detail::StrAllocTracker* str_allocs) { + // NOTE: we should NOT directly modify characters held by field_ptr + const char** dest = const_cast( + local_chemistry_data_access_string(&my_chem, name.c_str())); + if (dest == nullptr) { + return false; // field is either not known or not a string + } + + // the following acts as a compiler hint (to suppress a warning) + GR_INTERNAL_REQUIRE((val == nullptr) != (sz_with_nul > 0), "compiler-hint"); + + if (str_allocs != nullptr) { + // (if applicable) allocate a new buffer and deallocate the old buffer + char* new_alloc = str_allocs->alloc_buf_and_free_old(sz_with_nul, *dest); + if (sz_with_nul > 0) { + std::memcpy(new_alloc, val, sz_with_nul - 1); + new_alloc[sz_with_nul - 1] = '\0'; + } + (*dest) = new_alloc; + } else { + (*dest) = val; + } + return true; +} + +/// Tries to set a string parameter tracked by @p val +/// +/// @param[in,out] my_chem Tracks various Grackle parameters +/// @param[in] name The name of the parameter getting updated +/// @param[in] val The value of the parameter +/// @param[in] str_allocs Tracks allocations of the strings held by @p my_chem +/// @returns true if successful and `false` if there was an error (e.g. @p name +/// isn't a known parameter or is a parameter that doesn't expect a string) +/// +/// @note +/// Ordinarily, this function will update @p str_allocs to hold a copy of +/// @p val and @p my_chem will be updated to store a pointer to that copy. When +/// @p str_allocs is a nullptr, @p my_chem is directly updated to track the data +/// referenced by @p val. +bool set_str(chemistry_data& my_chem, const std::string& name, const char* val, + param_detail::StrAllocTracker* str_allocs) { + std::size_t sz_with_nul = (val == nullptr) ? 0 : std::strlen(val) + 1; + return set_str_(my_chem, name, val, sz_with_nul, str_allocs); +} + +bool set_str(chemistry_data& my_chem, const std::string& name, + const std::string& val, + param_detail::StrAllocTracker* str_allocs) { + return set_str_(my_chem, name, val.data(), val.size() + 1, str_allocs); +} + +bool set_int(chemistry_data& my_chem, const std::string& name, int val) { + int* dest = local_chemistry_data_access_int(&my_chem, name.c_str()); + if (dest == nullptr) { + return false; + } + (*dest) = val; + return true; +} + +bool set_double(chemistry_data& my_chem, const std::string& name, double val) { + double* dest = local_chemistry_data_access_double(&my_chem, name.c_str()); + if (dest == nullptr) { + return false; + } + (*dest) = val; + return true; +} + +} // anonymous namespace + +bool set_param(chemistry_data& my_chem, const std::string& name, + const ParamVal& par_val, + param_detail::StrAllocTracker* str_allocs) { + bool success = false; + + // this lambda function is passed a reference to the value within par_val.val_ + auto set_val = [&success, &my_chem, &name, + str_allocs](const auto& v) -> void { + using T = std::decay_t; + if constexpr (std::is_same_v) { + success = set_int(my_chem, name, v); + } else if constexpr (std::is_same_v) { + success = set_double(my_chem, name, v); + } else if constexpr (std::is_same_v || + std::is_same_v) { + success = set_str(my_chem, name, v, str_allocs); + } else { + static_assert(always_false_, "encountered unhandled type"); + } + }; + + std::visit(set_val, par_val.val_); + return success; +} + +} // namespace grtest \ No newline at end of file diff --git a/tests/grtestutils/param.hpp b/tests/grtestutils/param.hpp new file mode 100644 index 000000000..7ea9465be --- /dev/null +++ b/tests/grtestutils/param.hpp @@ -0,0 +1,171 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// +/// +//===----------------------------------------------------------------------===// +#ifndef GRTESTUTILS_PARAM_HPP +#define GRTESTUTILS_PARAM_HPP + +#include "grackle.h" +#include +#include +#include +#include +#include + +namespace grtest { + +namespace param_detail { +// a forward declaration +class StrAllocTracker; +} // namespace param_detail + +/// Used to hold a chemistry_data parameter value (of arbitrary type) +/// +/// This is primarily intended to help construct a chemistry_data instance +class ParamVal { + friend bool set_param(chemistry_data& my_chem, const std::string& name, + const ParamVal& par_val, + param_detail::StrAllocTracker* str_allocs); + + using val_type = std::variant; + + // private attribute + val_type val_; + + static val_type coerce_c_string_(const char* s) { + return (s == nullptr) ? val_type(std::in_place_type) + : val_type(std::in_place_type, s); + } + +public: + // it isn't possible to have an empty parameter value + ParamVal() = delete; + + explicit ParamVal(int val) : val_(val) {} + explicit ParamVal(double val) : val_(val) {} + explicit ParamVal(std::string val) : val_(val) {} + explicit ParamVal(std::string_view val) : val_(std::string(val)) {} + explicit ParamVal(const char* val) : val_(ParamVal::coerce_c_string_(val)) {} + + /// return a string-representation of `this` + std::string to_string() const; + + /// teach googletest how to print the value + friend void PrintTo(const ParamVal& p, std::ostream* os); + + bool operator==(const ParamVal& other) const { return val_ == other.val_; } + bool operator!=(const ParamVal& other) const { return val_ != other.val_; } + + // all the following methods are primarily intended for debugging purposes + // (they are not essential to the core functionality) + + /// Try to access a pointer the contained value (intended for debugging) + /// + /// For example, `param_val.try_get()`, tries to access the value as an + /// integer + template + std::optional try_get() const { + T* ptr = std::get_if(&val_); + return (ptr == nullptr) ? std::nullopt : std::optional{*ptr}; + } + + /// Returns whether `this` holds an empty string + bool is_empty_string() const { + return std::holds_alternative(val_); + } + + /*!{*/ + /// Checks equivalence with the specified value + bool is_equal(int val) const; + bool is_equal(double val) const; + bool is_equal(std::string_view val) const; + bool is_equal(const char* val) const; + /*!}*/ +}; + +/// Use this whenever you want to make a sequence of key-value pairs +using ParamPair = std::pair; + +/// Nicely format a @ref ParamPair as a string +inline std::string to_string(const ParamPair& pair) { + std::string str_val = pair.second.to_string(); + std::string out; + out.reserve(pair.first.size() + str_val.size() + 6); + out += "{\""; + out += pair.first; + out += "\", "; + out += str_val; + out += '}'; + return out; +} + +/// Tries to update @p my_chem to hold the value specified by @p par_val +/// +/// @param[in,out] my_chem Tracks various Grackle parameters +/// @param[in] name The name of the parameter getting updated +/// @param[in] par_val The value of the parameter +/// @param[in] str_allocs Tracks allocations of the strings held by @p my_chem +/// @returns true if successful and `false` if there was an error (e.g. @p name +/// isn't a known parameter or is a parameter that doesn't expect a string) +/// +/// @note +/// When @p par_val holds a non-null string and @p name is a known parameter +/// holding a string: +/// - this function ordinarily updates @p str_allocs to hold a copy of +/// @p par_val and @p my_chem will be updated to store a pointer to that copy. +/// - When @p str_allocs is a nullptr, @p my_chem is directly updated to track +/// the data contained within @p par_val. +bool set_param(chemistry_data& my_chem, const std::string& name, + const ParamVal& par_val, + param_detail::StrAllocTracker* str_allocs); + +/// Tries to update @p my_chem to hold the value specified by @p par_val +/// +/// This is designed work with various standard library container types holding +/// key-value parameter name pairs. For concreteness, suppose we have a variable +/// called `my_params` with one of the following types: +/// - `std::vector>` (this is equivalent to +/// `std::vector`) +/// - `std::map` +/// - `std::unordered_map` +/// +/// For each of the cases, you would invoke +/// @code{C++} +/// set_params(my_params.cbegin(), my_params.cend(), my_chem, str_allocs); +/// @endcode +/// +/// @param[in] first, last the pair of iterators defining the range of parameter +/// name-value pairs that will be used to update my_chem. +/// @param[in,out] my_chem Tracks various Grackle parameters +/// @param[in] str_allocs Tracks allocations of the strings held by @p my_chem +/// @returns An empty optional if successful. Otherwise, it returns the name +/// of the parameter that couldn't be updated. +/// +/// @tparam It The type of the iterator. When str_allocs is a `nullptr` this +/// must be a ForwardIterator (i.e. after incrementing the iterator, +/// previous values must remain valid). Otherwise, this can also be an +/// InputIterator +template +std::optional set_params( + It first, It last, chemistry_data& my_chem, + param_detail::StrAllocTracker* str_allocs) { + for (It it = first; it != last; ++it) { + const std::string& name = it->first; + const ParamVal& val = it->second; + if (!set_param(my_chem, name, val, str_allocs)) { + return std::optional(name); + } + } + return std::nullopt; // we succeeded +} + +} // namespace grtest + +#endif // GRTESTUTILS_PARAM_HPP From 1ea8dd8cfb132c869f1c0fe076b55d5288047ad8 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 24 Jan 2026 14:23:40 -0500 Subject: [PATCH 04/29] grtestutils: refactor presets to use ParamVal --- tests/grtestutils/GrackleCtxPack.hpp | 46 +++++++++++++------ tests/grtestutils/preset.cpp | 69 +++++++++++++++------------- tests/grtestutils/preset.hpp | 10 ++-- tests/grtestutils/utils.cpp | 22 +++++---- tests/grtestutils/utils.hpp | 20 +++----- 5 files changed, 96 insertions(+), 71 deletions(-) diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/GrackleCtxPack.hpp index 2fca9d7de..601f2a12f 100644 --- a/tests/grtestutils/GrackleCtxPack.hpp +++ b/tests/grtestutils/GrackleCtxPack.hpp @@ -61,7 +61,6 @@ class StrAllocTracker { } }; - } // namespace param_detail /// Tracks the group of Grackle objects needed for executing API functions @@ -79,6 +78,8 @@ class StrAllocTracker { class GrackleCtxPack { /// units used for initializing chemistry_data code_units initial_units_; + /// tracks string allocations for @ref my_chemistry_ + param_detail::StrAllocTracker str_allocs_; /// the fully initialized chemistry_data instance std::unique_ptr my_chemistry_; /// the fully initialized chemistry_data_storage_instance @@ -115,30 +116,49 @@ class GrackleCtxPack { /// 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) { + auto erroneous_exit = [&status](InitStatus new_status) { if (status != nullptr) { - *status = tmp; + *status = new_status; } - return GrackleCtxPack(); // return an unitialized instance + return GrackleCtxPack(); + }; + + // allocate chemistry_data and set the defaults + std::unique_ptr my_chem(new chemistry_data); + if (local_initialize_chemistry_parameters(my_chem.get()) != GR_SUCCESS) { + return erroneous_exit(InitStatus::generic_fail); } + // lookup the parameters associated with the preset + std::pair, InitStatus> chem_preset_rslt = + get_chem_preset_vals_(preset.chemistry); + if (chem_preset_rslt.second != InitStatus::success) { + return erroneous_exit(chem_preset_rslt.second); + } + const std::vector& params = chem_preset_rslt.first; + + // update my_chem with values from the preset + param_detail::StrAllocTracker str_allocs; + std::optional bad_param = + set_params(params.begin(), params.end(), *my_chem, &str_allocs); + if (bad_param.has_value()) { + // todo: we probably want to tell the caller about the bad parameter + return erroneous_exit(InitStatus::generic_fail); + } + + // set up chemistry_data_storage 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(), + if (local_initialize_chemistry_data(my_chem.get(), my_rates.get(), &initial_unit) != GR_SUCCESS) { - if (status != nullptr) { - *status = InitStatus::generic_fail; - } - return GrackleCtxPack(); // return an unitialized instance + return erroneous_exit(InitStatus::generic_fail); } GrackleCtxPack out; out.initial_units_ = initial_unit; - out.my_chemistry_ = std::move(my_chemistry); + out.str_allocs_ = std::move(str_allocs); + out.my_chemistry_ = std::move(my_chem); out.my_rates_ = std::move(my_rates); return out; } diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/preset.cpp index fedf0f96d..4ccfc8a0f 100644 --- a/tests/grtestutils/preset.cpp +++ b/tests/grtestutils/preset.cpp @@ -10,12 +10,15 @@ /// //===----------------------------------------------------------------------===// +#include "./param.hpp" #include "./preset.hpp" #include "./utils.hpp" #include "grackle.h" #include "status_reporting.h" // GR_INTERNAL_UNREACHABLE_ERROR +#include + namespace grtest { static std::string to_string(const grtest::ChemPreset& preset) { switch (preset) { @@ -34,50 +37,52 @@ static std::string to_string(const grtest::ChemPreset& preset) { GR_INTERNAL_UNREACHABLE_ERROR(); } -InitStatus setup_chemistry_data_from_preset(chemistry_data* my_chem, - ChemPreset preset) { - if (local_initialize_chemistry_parameters(my_chem) != GR_SUCCESS) { - return InitStatus::generic_fail; - } +std::pair, InitStatus> get_chem_preset_vals_( + ChemPreset preset) { + std::vector> v = { + {"use_grackle", ParamVal(1)}, // chemistry on + {"use_isrf_field", ParamVal(1)}, + {"with_radiative_cooling", ParamVal(1)}, // cooling on + {"metal_cooling", ParamVal(1)}, // metal cooling on + {"UVbackground", ParamVal(1)}, // UV background on + }; - if (!set_standard_datafile(*my_chem, "CloudyData_UVB=HM2012.h5")) { - return InitStatus::standard_datafile_notfound; + std::optional maybe_datafile = + get_standard_datafile("CloudyData_UVB=HM2012.h5"); + if (!maybe_datafile.has_value()) { + return {{}, InitStatus::standard_datafile_notfound}; + } else { + v.emplace_back("grackle_data_file", maybe_datafile.value()); } - 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; + v.emplace_back("primordial_chemistry", 0); + v.emplace_back("dust_chemistry", 0); + return {v, InitStatus::success}; } case ChemPreset::primchem1: { - my_chem->primordial_chemistry = 1; - my_chem->dust_chemistry = 1; - return InitStatus::success; + v.emplace_back("primordial_chemistry", 1); + v.emplace_back("dust_chemistry", 1); + return {v, InitStatus::success}; } case ChemPreset::primchem2: { - my_chem->primordial_chemistry = 2; - my_chem->dust_chemistry = 1; - return InitStatus::success; + v.emplace_back("primordial_chemistry", 2); + v.emplace_back("dust_chemistry", 1); + return {v, InitStatus::success}; } case ChemPreset::primchem3: { - my_chem->primordial_chemistry = 3; - my_chem->dust_chemistry = 1; - return InitStatus::success; + v.emplace_back("primordial_chemistry", 3); + v.emplace_back("dust_chemistry", 1); + return {v, 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 = 3; - my_chem->use_dust_density_field = 1; - return InitStatus::success; + v.emplace_back("primordial_chemistry", 4); + v.emplace_back("dust_chemistry", 1); + v.emplace_back("metal_chemistry", 1); + v.emplace_back("dust_species", 3); + v.emplace_back("use_dust_density_field", 1); + return {v, InitStatus::success}; } } GR_INTERNAL_UNREACHABLE_ERROR(); @@ -117,4 +122,4 @@ void PrintTo(const grtest::FullConfPreset& preset, std::ostream* os) { << to_string(preset.unit) << '}'; } -} // namespace grtest \ No newline at end of file +} // namespace grtest diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp index b1278d175..8e5b96478 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -13,9 +13,9 @@ #define GRTESTUTILS_PRESET_HPP #include "grackle.h" -#include -#include -#include +#include "./param.hpp" +#include +#include namespace grtest { @@ -39,8 +39,8 @@ enum class ChemPreset { }; /// override the settings of my_chem based on the specified preset -InitStatus setup_chemistry_data_from_preset(chemistry_data* my_chem, - ChemPreset preset); +std::pair, InitStatus> get_chem_preset_vals_( + ChemPreset preset); /// Preset for constructing the code_unit instance used for initializing the /// Grackle Solver diff --git a/tests/grtestutils/utils.cpp b/tests/grtestutils/utils.cpp index d4781249f..3665911f4 100644 --- a/tests/grtestutils/utils.cpp +++ b/tests/grtestutils/utils.cpp @@ -22,12 +22,11 @@ static const char* const standard_data_files[N_STANDARD_DATAFILES] = { stringify(GR_DATADIR) "/cloudy_metals_2008_3D.h5" }; -bool grtest::set_standard_datafile( - chemistry_data& my_chemistry, const char* datafile -) { +/// returns the string-literal for the standard data file +static const char* standard_datafile_literal_(const char* datafile) { - if (datafile==NULL) { - return false; // we should probably abort the program with an error + if (datafile==nullptr) { + return nullptr; } // we get the number of characters in the prefix-path @@ -38,9 +37,16 @@ bool grtest::set_standard_datafile( for (int i = 0; i < N_STANDARD_DATAFILES; i++){ if (std::strcmp(datafile, standard_data_files[i]+prefix_len) == 0) { - my_chemistry.grackle_data_file = standard_data_files[i]; - return true; + return standard_data_files[i]; } } - return false; + return nullptr; } + +std::optional grtest::get_standard_datafile(const char* datafile) { + const char* tmp = standard_datafile_literal_(datafile); + if (tmp==nullptr) { + return std::nullopt; + } + return std::optional(tmp); +} \ No newline at end of file diff --git a/tests/grtestutils/utils.hpp b/tests/grtestutils/utils.hpp index 72e4e77fa..b0fc12bcc 100644 --- a/tests/grtestutils/utils.hpp +++ b/tests/grtestutils/utils.hpp @@ -3,25 +3,19 @@ #ifndef GRTEST_UTILS_HPP #define GRTEST_UTILS_HPP -#include #include +#include +#include +#include + namespace grtest { -/// this function records the desired standard datafile within the -/// chemistry_data struct. It deals with the minutia of making sure that +/// this function returns the desired standard datafile. It deals with the minutia of making sure that /// grackle can find the standardized data-file /// -/// @returns true if successful or false if unsuccessful -/// -/// @note -/// For the sake of forward compatability (we will probably change the -/// implementation if we merge PRs 235, 237, and 246) you should: -/// - only pass string literals (or the addresses of string-literals) as the -/// this function's datafile arg -/// - AND never deallocate my_chemistry.datafile after calling this function -/// (there won't be a memory leak) -bool set_standard_datafile(chemistry_data& my_chemistry, const char* datafile); +/// @returns An empty optional if unsuccessful +std::optional get_standard_datafile(const char* datafile); } From 61c7fcf2d42e913e788faf37165cb3055b2c4724 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 23 Jan 2026 08:50:39 -0500 Subject: [PATCH 05/29] make it possible to more concisely initialize a vector of key-value pairs --- tests/grtestutils/param.hpp | 49 ++++++++++++++++++++++++++++++++++++ tests/grtestutils/preset.cpp | 14 +++++------ 2 files changed, 56 insertions(+), 7 deletions(-) diff --git a/tests/grtestutils/param.hpp b/tests/grtestutils/param.hpp index 7ea9465be..e3c5ee7be 100644 --- a/tests/grtestutils/param.hpp +++ b/tests/grtestutils/param.hpp @@ -13,11 +13,13 @@ #define GRTESTUTILS_PARAM_HPP #include "grackle.h" +#include #include #include #include #include #include +#include namespace grtest { @@ -106,6 +108,53 @@ inline std::string to_string(const ParamPair& pair) { return out; } +namespace param_detail { + +/// this **ONLY** exists to help implement @ref make_ParamPair_vec +struct ParamPairInit { + ParamPair pair; + + template + ParamPairInit(const char* k, V&& v) : pair(k, ParamVal(std::forward(v))) {} +}; + +} // namespace param_detail + +/// A convenience function for easier construction of a vector of Param pairs +/// +/// This function lets you write code like +/// @code{C++} +/// std::vector v = make_ParamPair_vec({ +/// {"use_grackle", 1}, +/// {"with_radiative_cooling", 1}, +/// {"metal_cooling", 1}, +/// {"UVbackground", 1}, +/// {"primordial_chemistry", 2} +/// }); +/// @endcode +/// +/// We could definitely get rid of this function, but then we would need to +/// rewrite the above snippet as: +/// @code{C++} +/// std::vector v = { +/// {"use_grackle", ParamVal(1)}, +/// {"with_radiative_cooling", ParamVal(1)}, +/// {"metal_cooling", ParamVal(1)}, +/// {"UVbackground", ParamVal(1)}, +/// {"primordial_chemistry", ParamVal(2)} +/// }; +inline std::vector make_ParamPair_vec( + std::initializer_list l) { + // if we were a little more clever, we might be able to get rid of an + // intermediate allocation when constructing ParamPairInit... + std::vector v; + v.reserve(l.size()); + for (const param_detail::ParamPairInit& p : l) { + v.push_back(p.pair); + } + return v; +} + /// Tries to update @p my_chem to hold the value specified by @p par_val /// /// @param[in,out] my_chem Tracks various Grackle parameters diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/preset.cpp index 4ccfc8a0f..bd19e0761 100644 --- a/tests/grtestutils/preset.cpp +++ b/tests/grtestutils/preset.cpp @@ -39,13 +39,13 @@ static std::string to_string(const grtest::ChemPreset& preset) { std::pair, InitStatus> get_chem_preset_vals_( ChemPreset preset) { - std::vector> v = { - {"use_grackle", ParamVal(1)}, // chemistry on - {"use_isrf_field", ParamVal(1)}, - {"with_radiative_cooling", ParamVal(1)}, // cooling on - {"metal_cooling", ParamVal(1)}, // metal cooling on - {"UVbackground", ParamVal(1)}, // UV background on - }; + std::vector> v = make_ParamPair_vec({ + {"use_grackle", 1}, // chemistry on + {"use_isrf_field", 1}, + {"with_radiative_cooling", 1}, // cooling on + {"metal_cooling", 1}, // metal cooling on + {"UVbackground", 1}, // UV background on + }); std::optional maybe_datafile = get_standard_datafile("CloudyData_UVB=HM2012.h5"); From af25b64fc519546375093d3dab3474b4fad5e53d Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 23 Jan 2026 09:45:18 -0500 Subject: [PATCH 06/29] rename FullConfPreset->SimpleConfPreset --- tests/grtestutils/GrackleCtxPack.hpp | 2 +- tests/grtestutils/googletest/fixtures.hpp | 4 ++-- tests/grtestutils/preset.cpp | 2 +- tests/grtestutils/preset.hpp | 6 +++--- tests/unit/test_api_ratequery.cpp | 14 +++++++------- tests/unit/test_ghost_zone.cpp | 10 +++++----- 6 files changed, 19 insertions(+), 19 deletions(-) diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/GrackleCtxPack.hpp index 601f2a12f..0d26d6487 100644 --- a/tests/grtestutils/GrackleCtxPack.hpp +++ b/tests/grtestutils/GrackleCtxPack.hpp @@ -114,7 +114,7 @@ class GrackleCtxPack { chemistry_data_storage* my_rates() { return this->my_rates_.get(); } /// create an initialized instance from a preset - static GrackleCtxPack create(const FullConfPreset& preset, + static GrackleCtxPack create(const SimpleConfPreset& preset, InitStatus* status) { auto erroneous_exit = [&status](InitStatus new_status) { if (status != nullptr) { diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index fc72c4a3a..6b8edacd7 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -44,7 +44,7 @@ class ConfigPresetFixture : public testing::Test { // called immediately after the constructor (but before the test-case) grtest::InitStatus status; - pack = GrackleCtxPack::create(FullConfPreset{chem_preset, unit_preset}, + pack = GrackleCtxPack::create(SimpleConfPreset{chem_preset, unit_preset}, &status); if (!pack.is_initialized()) { if (status == InitStatus::standard_datafile_notfound) { @@ -74,7 +74,7 @@ class ConfigPresetFixture : public testing::Test { /// this class, OR /// 2. make a subclass, named `MyFeatureTest`, of this class class ParametrizedConfigPresetFixture - : public testing::TestWithParam { + : public testing::TestWithParam { protected: void SetUp() override { // called immediately after the constructor (but before the test-case) diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/preset.cpp index bd19e0761..2ff206dd1 100644 --- a/tests/grtestutils/preset.cpp +++ b/tests/grtestutils/preset.cpp @@ -117,7 +117,7 @@ code_units setup_initial_unit(InitialUnitPreset preset) { GR_INTERNAL_UNREACHABLE_ERROR(); } -void PrintTo(const grtest::FullConfPreset& preset, std::ostream* os) { +void PrintTo(const grtest::SimpleConfPreset& 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 index 8e5b96478..a9156252a 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -55,13 +55,13 @@ enum class InitialUnitPreset { code_units setup_initial_unit(InitialUnitPreset preset); /// Represents the preset for creating a GrackleCtxPack -struct FullConfPreset { +struct SimpleConfPreset { ChemPreset chemistry; InitialUnitPreset unit; }; -// teach googletest how to print FullConfPreset -void PrintTo(const FullConfPreset& preset, std::ostream* os); +// teach googletest how to print SimpleConfPreset +void PrintTo(const SimpleConfPreset& preset, std::ostream* os); } // namespace grtest diff --git a/tests/unit/test_api_ratequery.cpp b/tests/unit/test_api_ratequery.cpp index 02d0882bd..f163f192a 100644 --- a/tests/unit/test_api_ratequery.cpp +++ b/tests/unit/test_api_ratequery.cpp @@ -78,15 +78,15 @@ TEST_P(ParametrizedRateQueryTest, ConsistentIDs) { } using grtest::ChemPreset; -using grtest::FullConfPreset; using grtest::InitialUnitPreset; +using grtest::SimpleConfPreset; 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})); + SimpleConfPreset{ChemPreset::primchem0, InitialUnitPreset::simple_z0}, + SimpleConfPreset{ChemPreset::primchem1, InitialUnitPreset::simple_z0}, + SimpleConfPreset{ChemPreset::primchem2, InitialUnitPreset::simple_z0}, + SimpleConfPreset{ChemPreset::primchem3, InitialUnitPreset::simple_z0}, + SimpleConfPreset{ChemPreset::primchem4_dustspecies3, + InitialUnitPreset::simple_z0})); diff --git a/tests/unit/test_ghost_zone.cpp b/tests/unit/test_ghost_zone.cpp index 16628721f..317b055b7 100644 --- a/tests/unit/test_ghost_zone.cpp +++ b/tests/unit/test_ghost_zone.cpp @@ -342,15 +342,15 @@ TEST_P(APIGhostZoneTest, GridZoneStartEnd) { } -using grtest::FullConfPreset; +using grtest::SimpleConfPreset; using grtest::ChemPreset; using grtest::InitialUnitPreset; INSTANTIATE_TEST_SUITE_P( /* 1st arg is intentionally empty */, APIGhostZoneTest, ::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}) + SimpleConfPreset{ChemPreset::primchem0, InitialUnitPreset::simple_z0}, + SimpleConfPreset{ChemPreset::primchem1, InitialUnitPreset::simple_z0}, + SimpleConfPreset{ChemPreset::primchem2, InitialUnitPreset::simple_z0}, + SimpleConfPreset{ChemPreset::primchem3, InitialUnitPreset::simple_z0}) ); From 151cc5f6f93f00fd5c65a30583087739685e22de Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 23 Jan 2026 16:29:16 -0500 Subject: [PATCH 07/29] grtestutils: introduce Status type --- tests/grtestutils/CMakeLists.txt | 1 + tests/grtestutils/GrackleCtxPack.hpp | 43 ++++++------ tests/grtestutils/googletest/fixtures.hpp | 39 +++++------ tests/grtestutils/param.hpp | 10 +-- tests/grtestutils/preset.cpp | 14 ++-- tests/grtestutils/preset.hpp | 10 +-- tests/grtestutils/status.cpp | 68 +++++++++++++++++++ tests/grtestutils/status.hpp | 83 +++++++++++++++++++++++ 8 files changed, 203 insertions(+), 65 deletions(-) create mode 100644 tests/grtestutils/status.cpp create mode 100644 tests/grtestutils/status.hpp diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index bf12b77f4..cda5b1e78 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -85,6 +85,7 @@ add_library(grtestutils os.hpp os.cpp param.hpp param.cpp preset.hpp preset.cpp + status.hpp status.cpp utils.hpp utils.cpp # files in the googletest subdirectory (reminder: should just be headers!) diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/GrackleCtxPack.hpp index 0d26d6487..172be18b0 100644 --- a/tests/grtestutils/GrackleCtxPack.hpp +++ b/tests/grtestutils/GrackleCtxPack.hpp @@ -12,7 +12,8 @@ #ifndef GRTESTUTILS_GRACKLECTXPACK_HPP #define GRTESTUTILS_GRACKLECTXPACK_HPP -#include "preset.hpp" +#include "./preset.hpp" +#include "./status.hpp" #include // std::remove_if #include // std::unique_ptr @@ -114,36 +115,29 @@ class GrackleCtxPack { chemistry_data_storage* my_rates() { return this->my_rates_.get(); } /// create an initialized instance from a preset - static GrackleCtxPack create(const SimpleConfPreset& preset, - InitStatus* status) { - auto erroneous_exit = [&status](InitStatus new_status) { - if (status != nullptr) { - *status = new_status; - } - return GrackleCtxPack(); - }; - + static std::pair create( + const SimpleConfPreset& preset) { // allocate chemistry_data and set the defaults std::unique_ptr my_chem(new chemistry_data); if (local_initialize_chemistry_parameters(my_chem.get()) != GR_SUCCESS) { - return erroneous_exit(InitStatus::generic_fail); + return {GrackleCtxPack(), + error::Adhoc("initialize_chemistry_parameters failed")}; } // lookup the parameters associated with the preset - std::pair, InitStatus> chem_preset_rslt = + std::pair, Status> chem_preset_rslt = get_chem_preset_vals_(preset.chemistry); - if (chem_preset_rslt.second != InitStatus::success) { - return erroneous_exit(chem_preset_rslt.second); + if (!chem_preset_rslt.second.is_ok()) { + return {GrackleCtxPack(), chem_preset_rslt.second}; } const std::vector& params = chem_preset_rslt.first; // update my_chem with values from the preset param_detail::StrAllocTracker str_allocs; - std::optional bad_param = + Status status = set_params(params.begin(), params.end(), *my_chem, &str_allocs); - if (bad_param.has_value()) { - // todo: we probably want to tell the caller about the bad parameter - return erroneous_exit(InitStatus::generic_fail); + if (!status.is_ok()) { + return {GrackleCtxPack(), status}; } // set up chemistry_data_storage @@ -152,14 +146,15 @@ class GrackleCtxPack { new chemistry_data_storage); if (local_initialize_chemistry_data(my_chem.get(), my_rates.get(), &initial_unit) != GR_SUCCESS) { - return erroneous_exit(InitStatus::generic_fail); + return {GrackleCtxPack(), + error::Adhoc("initialize_chemistry_data failed")}; } - GrackleCtxPack out; - out.initial_units_ = initial_unit; - out.str_allocs_ = std::move(str_allocs); - out.my_chemistry_ = std::move(my_chem); - out.my_rates_ = std::move(my_rates); + std::pair out{GrackleCtxPack(), OkStatus()}; + out.first.initial_units_ = initial_unit; + out.first.str_allocs_ = std::move(str_allocs); + out.first.my_chemistry_ = std::move(my_chem); + out.first.my_rates_ = std::move(my_rates); return out; } }; diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index 6b8edacd7..00e5270f0 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -20,6 +20,9 @@ #include "../GrackleCtxPack.hpp" #include "../preset.hpp" +#include "../status.hpp" + +#include // std::move namespace grtest { @@ -43,17 +46,14 @@ class ConfigPresetFixture : public testing::Test { void SetUp() override { // called immediately after the constructor (but before the test-case) - grtest::InitStatus status; - pack = GrackleCtxPack::create(SimpleConfPreset{chem_preset, unit_preset}, - &status); - if (!pack.is_initialized()) { - if (status == InitStatus::standard_datafile_notfound) { - GTEST_SKIP() - << "something went before initialization while searching for a " - << "standard datafile"; - } else { - FAIL() << "Error in initialize_chemistry_data."; - } + std::pair tmp = + GrackleCtxPack::create(SimpleConfPreset{chem_preset, unit_preset}); + if (tmp.second.is_ok()) { + pack = std::move(tmp.first); + } else if (tmp.second.is_missing_std_file()) { + GTEST_SKIP() << tmp.second.to_string(); + } else { + FAIL() << tmp.second.to_string(); } } @@ -78,16 +78,13 @@ class ParametrizedConfigPresetFixture 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::standard_datafile_notfound) { - GTEST_SKIP() - << "something went before initialization while searching for a " - << "standard datafile"; - } else { - FAIL() << "Error in initialize_chemistry_data."; - } + std::pair tmp = GrackleCtxPack::create(GetParam()); + if (tmp.second.is_ok()) { + pack = std::move(tmp.first); + } else if (tmp.second.is_missing_std_file()) { + GTEST_SKIP() << tmp.second.to_string(); + } else { + FAIL() << tmp.second.to_string(); } } diff --git a/tests/grtestutils/param.hpp b/tests/grtestutils/param.hpp index e3c5ee7be..50af101f1 100644 --- a/tests/grtestutils/param.hpp +++ b/tests/grtestutils/param.hpp @@ -13,6 +13,7 @@ #define GRTESTUTILS_PARAM_HPP #include "grackle.h" +#include "./status.hpp" #include #include #include @@ -202,17 +203,16 @@ bool set_param(chemistry_data& my_chem, const std::string& name, /// previous values must remain valid). Otherwise, this can also be an /// InputIterator template -std::optional set_params( - It first, It last, chemistry_data& my_chem, - param_detail::StrAllocTracker* str_allocs) { +Status set_params(It first, It last, chemistry_data& my_chem, + param_detail::StrAllocTracker* str_allocs) { for (It it = first; it != last; ++it) { const std::string& name = it->first; const ParamVal& val = it->second; if (!set_param(my_chem, name, val, str_allocs)) { - return std::optional(name); + return error::Param(name); } } - return std::nullopt; // we succeeded + return OkStatus(); } } // namespace grtest diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/preset.cpp index 2ff206dd1..0152bc0b3 100644 --- a/tests/grtestutils/preset.cpp +++ b/tests/grtestutils/preset.cpp @@ -37,7 +37,7 @@ static std::string to_string(const grtest::ChemPreset& preset) { GR_INTERNAL_UNREACHABLE_ERROR(); } -std::pair, InitStatus> get_chem_preset_vals_( +std::pair, Status> get_chem_preset_vals_( ChemPreset preset) { std::vector> v = make_ParamPair_vec({ {"use_grackle", 1}, // chemistry on @@ -50,7 +50,7 @@ std::pair, InitStatus> get_chem_preset_vals_( std::optional maybe_datafile = get_standard_datafile("CloudyData_UVB=HM2012.h5"); if (!maybe_datafile.has_value()) { - return {{}, InitStatus::standard_datafile_notfound}; + return {{}, error::MissingStdFile()}; } else { v.emplace_back("grackle_data_file", maybe_datafile.value()); } @@ -59,22 +59,22 @@ std::pair, InitStatus> get_chem_preset_vals_( case ChemPreset::primchem0: { v.emplace_back("primordial_chemistry", 0); v.emplace_back("dust_chemistry", 0); - return {v, InitStatus::success}; + return {v, OkStatus()}; } case ChemPreset::primchem1: { v.emplace_back("primordial_chemistry", 1); v.emplace_back("dust_chemistry", 1); - return {v, InitStatus::success}; + return {v, OkStatus()}; } case ChemPreset::primchem2: { v.emplace_back("primordial_chemistry", 2); v.emplace_back("dust_chemistry", 1); - return {v, InitStatus::success}; + return {v, OkStatus()}; } case ChemPreset::primchem3: { v.emplace_back("primordial_chemistry", 3); v.emplace_back("dust_chemistry", 1); - return {v, InitStatus::success}; + return {v, OkStatus()}; } case ChemPreset::primchem4_dustspecies3: { v.emplace_back("primordial_chemistry", 4); @@ -82,7 +82,7 @@ std::pair, InitStatus> get_chem_preset_vals_( v.emplace_back("metal_chemistry", 1); v.emplace_back("dust_species", 3); v.emplace_back("use_dust_density_field", 1); - return {v, InitStatus::success}; + return {v, OkStatus()}; } } GR_INTERNAL_UNREACHABLE_ERROR(); diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp index a9156252a..808f3c52e 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -14,18 +14,12 @@ #include "grackle.h" #include "./param.hpp" +#include "./status.hpp" #include #include namespace grtest { -/// this only exists so that we can determine the reason that a test fails -enum class InitStatus { - success, - generic_fail, - standard_datafile_notfound, -}; - /// represents different presets for initializing chemistry_data /// /// @note @@ -39,7 +33,7 @@ enum class ChemPreset { }; /// override the settings of my_chem based on the specified preset -std::pair, InitStatus> get_chem_preset_vals_( +std::pair, Status> get_chem_preset_vals_( ChemPreset preset); /// Preset for constructing the code_unit instance used for initializing the diff --git a/tests/grtestutils/status.cpp b/tests/grtestutils/status.cpp new file mode 100644 index 000000000..081461723 --- /dev/null +++ b/tests/grtestutils/status.cpp @@ -0,0 +1,68 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Define the Status class +/// +//===----------------------------------------------------------------------===// +#include "status.hpp" + +#include "status_reporting.h" // GR_INTERNAL_UNREACHABLE_ERROR +#include // std::exchange + +namespace grtest { + +namespace status_detail { + +/// This mirrors the internal structure of Status. +struct TmpStatus { + StatusKind kind; + std::string s; +}; + +} // namespace status_detail + +Status::Status(status_detail::TmpStatus&& tmp) + : kind(tmp.kind), s(std::exchange(tmp.s, std::string())) {} + +Status error::MissingStdFile() { + return Status(status_detail::TmpStatus{ + status_detail::StatusKind::MISSING_STD_FILE, ""}); +} + +Status error::Param(std::string param_name) { + return Status(status_detail::TmpStatus{status_detail::StatusKind::PARAM, + std::move(param_name)}); +} + +Status error::Adhoc(std::string message) { + return Status(status_detail::TmpStatus{status_detail::StatusKind::ADHOC, + std::move(message)}); +} + +std::string Status::to_string() const { + switch (kind) { + case status_detail::StatusKind::OK: + return "OkStatus"; + case status_detail::StatusKind::MISSING_STD_FILE: { + return ( + "something went wrong with the test harness routine for looking up a " + "standard datafile"); + } + case status_detail::StatusKind::PARAM: { + return ( + "the parameter, \"" + this->s + + "\" isn't known to Grackle, or was associated with the wrong type"); + } + case status_detail::StatusKind::ADHOC: + return s; + default: + GR_INTERNAL_UNREACHABLE_ERROR(); + } +} + +} // namespace grtest \ No newline at end of file diff --git a/tests/grtestutils/status.hpp b/tests/grtestutils/status.hpp new file mode 100644 index 000000000..0e4ddee7b --- /dev/null +++ b/tests/grtestutils/status.hpp @@ -0,0 +1,83 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Declare the Status class +/// +//===----------------------------------------------------------------------===// +#ifndef GRTESTUTILS_STATUS_HPP +#define GRTESTUTILS_STATUS_HPP + +#include + +namespace grtest { + +namespace status_detail { + +struct TmpStatus; // forward declaration + +enum class StatusKind { OK, MISSING_STD_FILE, PARAM, ADHOC }; + +} // namespace status_detail + +/// An instances can be used to denote context when returning from functions +/// +/// @par Aside +/// Frankly, I would prefer if we used std::expected (or a simplified backport) +/// and made this only represent errors, but we can always refactor later +class Status { + /// tracks the kind of status + status_detail::StatusKind kind; + + /// for certain kinds of statuses, this provides extra context + std::string s; + +public: + Status() : kind(status_detail::StatusKind::OK), s() {} + + /// this constructor is used by factory functions + explicit Status(status_detail::TmpStatus&& tmp); + + /// checks if the status is ok + bool is_ok() const { return kind == status_detail::StatusKind::OK; } + + /// checks if the status is an error + bool is_err() const { return !is_ok(); } + + /// coerce to a string representation + std::string to_string() const; + + /// Return true when `this` indicates that a standard datafile can't be found + bool is_missing_std_file() const { + return kind == status_detail::StatusKind::MISSING_STD_FILE; + } +}; + +/// construct an Ok status +inline Status OkStatus() { return Status(); } + +/// This namespace holds factory functions for @ref Status instances that +/// denote errors +namespace error { + +/// Indicates that standard datafiles are missing +Status MissingStdFile(); + +/// There was an error setting the specified parameter name. +/// +/// This means that the parameter wasn't known or the associated value had the +/// wrong type +Status Param(std::string param_name); + +/// Specifies a generic string error message +Status Adhoc(std::string message); + +} // namespace error + +} // namespace grtest + +#endif // GRTESTUTILS_STATUS_HPP From 26139f24d6fd1628f7318cdb94aeeeb6cd55386f Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 23 Jan 2026 17:11:35 -0500 Subject: [PATCH 08/29] relocate some logic and address compiler warning --- tests/grtestutils/CMakeLists.txt | 8 ++-- tests/grtestutils/GrackleCtxPack.cpp | 58 ++++++++++++++++++++++++++++ tests/grtestutils/GrackleCtxPack.hpp | 47 ++-------------------- 3 files changed, 66 insertions(+), 47 deletions(-) create mode 100644 tests/grtestutils/GrackleCtxPack.cpp diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index cda5b1e78..3cd85dc2b 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -80,12 +80,12 @@ add_library(grtestutils # files outside of the googletest subdirectory # -> these shouldn't include headers from the googletest subdirectory cmd.hpp cmd.cpp - GrackleCtxPack.hpp + GrackleCtxPack.cpp GrackleCtxPack.hpp iterator_adaptor.hpp os.hpp os.cpp - param.hpp param.cpp - preset.hpp preset.cpp - status.hpp status.cpp + param.cpp param.hpp + preset.cpp preset.hpp + status.cpp status.hpp utils.hpp utils.cpp # files in the googletest subdirectory (reminder: should just be headers!) diff --git a/tests/grtestutils/GrackleCtxPack.cpp b/tests/grtestutils/GrackleCtxPack.cpp new file mode 100644 index 000000000..308fb91e5 --- /dev/null +++ b/tests/grtestutils/GrackleCtxPack.cpp @@ -0,0 +1,58 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Help implement GrackleCtxPack +/// +//===----------------------------------------------------------------------===// + +#include "GrackleCtxPack.hpp" + +namespace grtest { + +std::pair GrackleCtxPack::create( + const SimpleConfPreset& preset) { + // allocate chemistry_data and set the defaults + std::unique_ptr my_chem(new chemistry_data); + if (local_initialize_chemistry_parameters(my_chem.get()) != GR_SUCCESS) { + return {GrackleCtxPack(), + error::Adhoc("initialize_chemistry_parameters failed")}; + } + + // lookup the parameters associated with the preset + std::pair, Status> chem_preset_rslt = + get_chem_preset_vals_(preset.chemistry); + if (!chem_preset_rslt.second.is_ok()) { + return {GrackleCtxPack(), chem_preset_rslt.second}; + } + const std::vector& params = chem_preset_rslt.first; + + // update my_chem with values from the preset + param_detail::StrAllocTracker str_allocs; + Status status = + set_params(params.begin(), params.end(), *my_chem, &str_allocs); + if (!status.is_ok()) { + return {GrackleCtxPack(), status}; + } + + // set up chemistry_data_storage + code_units initial_unit = setup_initial_unit(preset.unit); + std::unique_ptr my_rates(new chemistry_data_storage); + if (local_initialize_chemistry_data(my_chem.get(), my_rates.get(), + &initial_unit) != GR_SUCCESS) { + return {GrackleCtxPack(), error::Adhoc("initialize_chemistry_data failed")}; + } + + std::pair out{GrackleCtxPack(), OkStatus()}; + out.first.initial_units_ = initial_unit; + out.first.str_allocs_ = std::move(str_allocs); + out.first.my_chemistry_ = std::move(my_chem); + out.first.my_rates_ = std::move(my_rates); + return out; +} + +} // namespace grtest diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/GrackleCtxPack.hpp index 172be18b0..f086ad636 100644 --- a/tests/grtestutils/GrackleCtxPack.hpp +++ b/tests/grtestutils/GrackleCtxPack.hpp @@ -88,13 +88,14 @@ class GrackleCtxPack { public: /// Construct an uninitialized instance - GrackleCtxPack() : my_chemistry_(nullptr), my_rates_(nullptr) {} + GrackleCtxPack() + : initial_units_{}, 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 + // -> we could re-enable them if we wanted to be able to add to clone the // types (or if we wanted to internally use std::shared_ptr GrackleCtxPack(const GrackleCtxPack&) = delete; GrackleCtxPack& operator=(const GrackleCtxPack&) = delete; @@ -116,47 +117,7 @@ class GrackleCtxPack { /// create an initialized instance from a preset static std::pair create( - const SimpleConfPreset& preset) { - // allocate chemistry_data and set the defaults - std::unique_ptr my_chem(new chemistry_data); - if (local_initialize_chemistry_parameters(my_chem.get()) != GR_SUCCESS) { - return {GrackleCtxPack(), - error::Adhoc("initialize_chemistry_parameters failed")}; - } - - // lookup the parameters associated with the preset - std::pair, Status> chem_preset_rslt = - get_chem_preset_vals_(preset.chemistry); - if (!chem_preset_rslt.second.is_ok()) { - return {GrackleCtxPack(), chem_preset_rslt.second}; - } - const std::vector& params = chem_preset_rslt.first; - - // update my_chem with values from the preset - param_detail::StrAllocTracker str_allocs; - Status status = - set_params(params.begin(), params.end(), *my_chem, &str_allocs); - if (!status.is_ok()) { - return {GrackleCtxPack(), status}; - } - - // set up chemistry_data_storage - code_units initial_unit = setup_initial_unit(preset.unit); - std::unique_ptr my_rates( - new chemistry_data_storage); - if (local_initialize_chemistry_data(my_chem.get(), my_rates.get(), - &initial_unit) != GR_SUCCESS) { - return {GrackleCtxPack(), - error::Adhoc("initialize_chemistry_data failed")}; - } - - std::pair out{GrackleCtxPack(), OkStatus()}; - out.first.initial_units_ = initial_unit; - out.first.str_allocs_ = std::move(str_allocs); - out.first.my_chemistry_ = std::move(my_chem); - out.first.my_rates_ = std::move(my_rates); - return out; - } + const SimpleConfPreset& preset); }; } // namespace grtest From 9f248fbff3c701fe09db15d897202cc854043083 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 24 Jan 2026 19:16:30 -0500 Subject: [PATCH 09/29] Replace SimpleConfPreset with ParamConf --- tests/grtestutils/GrackleCtxPack.cpp | 24 +++- tests/grtestutils/GrackleCtxPack.hpp | 5 +- tests/grtestutils/googletest/fixtures.hpp | 7 +- tests/grtestutils/param.cpp | 5 +- tests/grtestutils/param.hpp | 9 +- tests/grtestutils/preset.cpp | 31 ++++- tests/grtestutils/preset.hpp | 148 +++++++++++++++++++++- tests/unit/test_api_ratequery.cpp | 22 ++-- tests/unit/test_ghost_zone.cpp | 15 ++- 9 files changed, 222 insertions(+), 44 deletions(-) diff --git a/tests/grtestutils/GrackleCtxPack.cpp b/tests/grtestutils/GrackleCtxPack.cpp index 308fb91e5..487bc96b7 100644 --- a/tests/grtestutils/GrackleCtxPack.cpp +++ b/tests/grtestutils/GrackleCtxPack.cpp @@ -10,12 +10,13 @@ /// //===----------------------------------------------------------------------===// -#include "GrackleCtxPack.hpp" +#include "./GrackleCtxPack.hpp" +#include "./preset.hpp" namespace grtest { std::pair GrackleCtxPack::create( - const SimpleConfPreset& preset) { + const ParamConf& param_conf) { // allocate chemistry_data and set the defaults std::unique_ptr my_chem(new chemistry_data); if (local_initialize_chemistry_parameters(my_chem.get()) != GR_SUCCESS) { @@ -25,22 +26,31 @@ std::pair GrackleCtxPack::create( // lookup the parameters associated with the preset std::pair, Status> chem_preset_rslt = - get_chem_preset_vals_(preset.chemistry); - if (!chem_preset_rslt.second.is_ok()) { + get_chem_preset_vals_(param_conf.chem_preset()); + if (chem_preset_rslt.second.is_err()) { return {GrackleCtxPack(), chem_preset_rslt.second}; } const std::vector& params = chem_preset_rslt.first; - // update my_chem with values from the preset + // initialize my_chem's string storage param_detail::StrAllocTracker str_allocs; + + // update my_chem with values from the preset Status status = set_params(params.begin(), params.end(), *my_chem, &str_allocs); - if (!status.is_ok()) { + if (status.is_err()) { + return {GrackleCtxPack(), status}; + } + + // update my_chem with values from parameter overrides (if there are any) + const std::vector& po_vec = param_conf.param_overrides(); + status = set_params(po_vec.begin(), po_vec.end(), *my_chem, &str_allocs); + if (status.is_err()) { return {GrackleCtxPack(), status}; } // set up chemistry_data_storage - code_units initial_unit = setup_initial_unit(preset.unit); + code_units initial_unit = setup_initial_unit(param_conf.unit_preset()); std::unique_ptr my_rates(new chemistry_data_storage); if (local_initialize_chemistry_data(my_chem.get(), my_rates.get(), &initial_unit) != GR_SUCCESS) { diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/GrackleCtxPack.hpp index f086ad636..02cbef659 100644 --- a/tests/grtestutils/GrackleCtxPack.hpp +++ b/tests/grtestutils/GrackleCtxPack.hpp @@ -115,9 +115,8 @@ class GrackleCtxPack { 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 std::pair create( - const SimpleConfPreset& preset); + /// create an initialized instance + static std::pair create(const ParamConf& param_conf); }; } // namespace grtest diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index 00e5270f0..56ebd140d 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -45,9 +45,8 @@ class ConfigPresetFixture : public testing::Test { protected: void SetUp() override { // called immediately after the constructor (but before the test-case) - - std::pair tmp = - GrackleCtxPack::create(SimpleConfPreset{chem_preset, unit_preset}); + ParamConf conf = ParamConf::SimplePreset(chem_preset, unit_preset); + std::pair tmp = GrackleCtxPack::create(conf); if (tmp.second.is_ok()) { pack = std::move(tmp.first); } else if (tmp.second.is_missing_std_file()) { @@ -74,7 +73,7 @@ class ConfigPresetFixture : public testing::Test { /// this class, OR /// 2. make a subclass, named `MyFeatureTest`, of this class class ParametrizedConfigPresetFixture - : public testing::TestWithParam { + : public testing::TestWithParam { protected: void SetUp() override { // called immediately after the constructor (but before the test-case) diff --git a/tests/grtestutils/param.cpp b/tests/grtestutils/param.cpp index 054262152..7feb0de1a 100644 --- a/tests/grtestutils/param.cpp +++ b/tests/grtestutils/param.cpp @@ -26,7 +26,7 @@ constexpr std::false_type always_false_{}; namespace grtest { -std::string ParamVal::to_string() const { +std::string ParamVal::to_string(bool unwrap) const { std::string tmp; // this lambda function is passed a reference to the value within this->val_ auto get_string_repr_ = [&tmp](const auto& v) -> void { @@ -46,6 +46,9 @@ std::string ParamVal::to_string() const { }; std::visit(get_string_repr_, this->val_); + if (unwrap) { + return tmp; + } std::string out; out.reserve(tmp.size() + 10); out = "ParamVal("; diff --git a/tests/grtestutils/param.hpp b/tests/grtestutils/param.hpp index 50af101f1..a45881743 100644 --- a/tests/grtestutils/param.hpp +++ b/tests/grtestutils/param.hpp @@ -58,7 +58,10 @@ class ParamVal { explicit ParamVal(const char* val) : val_(ParamVal::coerce_c_string_(val)) {} /// return a string-representation of `this` - std::string to_string() const; + /// + /// @param[in] unwrap When true, we just print the inner value. Otherwise, + /// the value is written as `ParamVal()` + std::string to_string(bool unwrap = false) const; /// teach googletest how to print the value friend void PrintTo(const ParamVal& p, std::ostream* os); @@ -97,8 +100,8 @@ class ParamVal { using ParamPair = std::pair; /// Nicely format a @ref ParamPair as a string -inline std::string to_string(const ParamPair& pair) { - std::string str_val = pair.second.to_string(); +inline std::string to_string(const ParamPair& pair, bool unwrap_val = false) { + std::string str_val = pair.second.to_string(unwrap_val); std::string out; out.reserve(pair.first.size() + str_val.size() + 6); out += "{\""; diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/preset.cpp index 0152bc0b3..07bca4a06 100644 --- a/tests/grtestutils/preset.cpp +++ b/tests/grtestutils/preset.cpp @@ -18,10 +18,13 @@ #include "status_reporting.h" // GR_INTERNAL_UNREACHABLE_ERROR #include +#include namespace grtest { -static std::string to_string(const grtest::ChemPreset& preset) { +std::string to_string(const grtest::ChemPreset& preset) { switch (preset) { + case grtest::ChemPreset::DEFAULT: + return ""; case grtest::ChemPreset::primchem0: return "pc=0"; case grtest::ChemPreset::primchem1: @@ -37,8 +40,16 @@ static std::string to_string(const grtest::ChemPreset& preset) { GR_INTERNAL_UNREACHABLE_ERROR(); } +void PrintTo(const grtest::ChemPreset& chem_preset, std::ostream* os) { + *os << to_string(chem_preset); +} + std::pair, Status> get_chem_preset_vals_( ChemPreset preset) { + if (preset == ChemPreset::DEFAULT) { + return {{}, OkStatus()}; + } + std::vector> v = make_ParamPair_vec({ {"use_grackle", 1}, // chemistry on {"use_isrf_field", 1}, @@ -56,6 +67,10 @@ std::pair, Status> get_chem_preset_vals_( } switch (preset) { + case ChemPreset::DEFAULT: { + // we should have returned before now + GR_INTERNAL_UNREACHABLE_ERROR(); + } case ChemPreset::primchem0: { v.emplace_back("primordial_chemistry", 0); v.emplace_back("dust_chemistry", 0); @@ -88,7 +103,7 @@ std::pair, Status> get_chem_preset_vals_( GR_INTERNAL_UNREACHABLE_ERROR(); } -static std::string to_string(const InitialUnitPreset& preset) { +std::string to_string(const InitialUnitPreset& preset) { switch (preset) { case grtest::InitialUnitPreset::simple_z0: return "simpleUnit-z=0"; @@ -117,9 +132,15 @@ code_units setup_initial_unit(InitialUnitPreset preset) { GR_INTERNAL_UNREACHABLE_ERROR(); } -void PrintTo(const grtest::SimpleConfPreset& preset, std::ostream* os) { - *os << "Preset{" << to_string(preset.chemistry) << ',' - << to_string(preset.unit) << '}'; +void PrintTo(const ParamConf& param_conf, std::ostream* os) { + param_conf.print_descr(os, true); +} + +std::string ParamConf::stringify(bool short_summary, + const std::string& common_indent) const { + std::ostringstream s; + this->print_descr(&s, short_summary, common_indent); + return s.str(); } } // namespace grtest diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/preset.hpp index 808f3c52e..23de0937f 100644 --- a/tests/grtestutils/preset.hpp +++ b/tests/grtestutils/preset.hpp @@ -14,7 +14,6 @@ #include "grackle.h" #include "./param.hpp" -#include "./status.hpp" #include #include @@ -25,6 +24,7 @@ namespace grtest { /// @note /// In the future, we probably want to add more enum class ChemPreset { + DEFAULT, primchem0, primchem1, primchem2, @@ -32,6 +32,10 @@ enum class ChemPreset { primchem4_dustspecies3, }; +void PrintTo(const ChemPreset& chem_preset, std::ostream* os); + +std::string to_string(const ChemPreset& preset); + /// override the settings of my_chem based on the specified preset std::pair, Status> get_chem_preset_vals_( ChemPreset preset); @@ -45,17 +49,147 @@ enum class InitialUnitPreset { simple_z0, // <- no cosmology, z=0 }; +std::string to_string(const InitialUnitPreset& preset); + /// 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 SimpleConfPreset { - ChemPreset chemistry; - InitialUnitPreset unit; +/// Represents a configuration for creating a GrackleCtxPack with overrides +/// +/// @note +/// This type is fleshed out a class, rather than just being a simpler +/// struct-like type for the sole purpose of providing nice stringification +/// operations +class ParamConf { + // this whole class is a little clunky... + + /// the chemistry preset + ChemPreset c_preset_; + /// the initial unit preset + InitialUnitPreset u_preset_; + + // A few thoughts about overrides_: + // 1. it's probably better if it's some kind of mapping type + // 2. long term, we may want overrides to be able to override options from + // u_preset_ + + /// the parameter overrides + std::vector overrides_; + + // it's only possible to default construct through a factory method + ParamConf() = default; + + bool is_overridden_(std::string_view name) const { + for (const ParamPair& pair : overrides_) { + if (name == pair.first) { + return true; + } + } + return false; + } + +public: + /// Represents a ParamConfig that is simply built from presets + static ParamConf SimplePreset(ChemPreset chemistry, InitialUnitPreset unit) { + ParamConf out; + out.c_preset_ = chemistry; + out.u_preset_ = unit; + return out; + } + + ParamConf(const std::optional& chemistry, InitialUnitPreset unit, + const std::vector& param_overrides) + : c_preset_(chemistry.value_or(ChemPreset::DEFAULT)), + u_preset_{unit}, + overrides_(param_overrides) {} + + ParamConf(const std::optional& chemistry, InitialUnitPreset unit, + std::vector&& param_overrides) + : c_preset_(chemistry.value_or(ChemPreset::DEFAULT)), + u_preset_{unit}, + overrides_(std::move(param_overrides)) {} + + ChemPreset chem_preset() const { return c_preset_; } + const std::vector& param_overrides() const { return overrides_; } + InitialUnitPreset unit_preset() const { return u_preset_; } + + /// teach Googletest to print the simple description + friend void PrintTo(const ParamConf& param_conf, std::ostream* os); + + /// this is used to help describe the parameters used in a test result message + /// + /// @note + /// This accepts a template parameter Stream because we want this to work + /// with Googletest's `::testing::AssertionResult` type. While that type + /// is probably a subclass of `std::ostream`, that's not explicitly stated in + /// the documentation (and I don't want things to break) + template + void print_descr(Sink* s, bool short_descr, + const std::string& common_indent = "") const; + + /// prints a simple description + std::string stringify(bool short_summary, + const std::string& common_indent = "") const; }; -// teach googletest how to print SimpleConfPreset -void PrintTo(const SimpleConfPreset& preset, std::ostream* os); +template +void ParamConf::print_descr(Sink* s, bool short_descr, + const std::string& common_indent) const { + std::string c_p_str = to_string(this->c_preset_); + std::string u_p_str = to_string(this->u_preset_); + + std::size_t n_override = this->overrides_.size(); + + if (short_descr) { + if (n_override == 0) { + *s << common_indent << "Preset{"; + *s << c_p_str << ',' << to_string(this->u_preset_) << '}'; + } else { + *s << common_indent << "ParamConf("; + *s << c_p_str << ",<+" << n_override << "overrides>," << u_p_str << ')'; + } + return; + } + + // the rest of this function is dedicated to providing a detailed summary + std::string level1_indent = common_indent + " "; + std::string level2_indent = common_indent + " "; + + *s << common_indent << "ParamConf{\n"; + *s << level1_indent << "ChemPreset: " << c_p_str << ",\n"; + + // Show non-overridden parameters corresponding to chem preset + if (this->c_preset_ != ChemPreset::DEFAULT) { + *s << level1_indent << "Non-Overridden Preset Params: "; + std::pair, Status> tmp = + get_chem_preset_vals_(this->c_preset_); + if (tmp.second.is_err()) { + *s << "\n"; + } else { + // record the pair isn't overridden + *s << "{\n"; + for (const ParamPair& pair : tmp.first) { + if (!this->is_overridden_(pair.first)) { + *s << level2_indent << to_string(pair, true) << ",\n"; + } + } + *s << level1_indent << "},\n"; + } + } + + // handle details about the overrides + *s << level1_indent << "Parameter Overrides: {\n"; + for (const ParamPair& pair : this->overrides_) { + *s << level2_indent << to_string(pair, true) << ",\n"; + } + *s << level1_indent << "},\n"; + + // finally show the unit-preset + *s << level1_indent << "Unit Preset: " << u_p_str << '\n'; + + // close the brace + *s << common_indent << "}\n"; +} } // namespace grtest diff --git a/tests/unit/test_api_ratequery.cpp b/tests/unit/test_api_ratequery.cpp index f163f192a..07ef9ee85 100644 --- a/tests/unit/test_api_ratequery.cpp +++ b/tests/unit/test_api_ratequery.cpp @@ -79,14 +79,20 @@ TEST_P(ParametrizedRateQueryTest, ConsistentIDs) { using grtest::ChemPreset; using grtest::InitialUnitPreset; -using grtest::SimpleConfPreset; +using grtest::ParamConf; + +static const ParamConf my_presets_[] = { + ParamConf::SimplePreset(ChemPreset::primchem0, + InitialUnitPreset::simple_z0), + ParamConf::SimplePreset(ChemPreset::primchem1, + InitialUnitPreset::simple_z0), + ParamConf::SimplePreset(ChemPreset::primchem2, + InitialUnitPreset::simple_z0), + ParamConf::SimplePreset(ChemPreset::primchem3, + InitialUnitPreset::simple_z0), + ParamConf::SimplePreset(ChemPreset::primchem4_dustspecies3, + InitialUnitPreset::simple_z0)}; INSTANTIATE_TEST_SUITE_P( /* 1st arg is intentionally empty */, ParametrizedRateQueryTest, - ::testing::Values( - SimpleConfPreset{ChemPreset::primchem0, InitialUnitPreset::simple_z0}, - SimpleConfPreset{ChemPreset::primchem1, InitialUnitPreset::simple_z0}, - SimpleConfPreset{ChemPreset::primchem2, InitialUnitPreset::simple_z0}, - SimpleConfPreset{ChemPreset::primchem3, InitialUnitPreset::simple_z0}, - SimpleConfPreset{ChemPreset::primchem4_dustspecies3, - InitialUnitPreset::simple_z0})); + ::testing::ValuesIn(my_presets_)); diff --git a/tests/unit/test_ghost_zone.cpp b/tests/unit/test_ghost_zone.cpp index 317b055b7..e06f5f6b9 100644 --- a/tests/unit/test_ghost_zone.cpp +++ b/tests/unit/test_ghost_zone.cpp @@ -342,15 +342,18 @@ TEST_P(APIGhostZoneTest, GridZoneStartEnd) { } -using grtest::SimpleConfPreset; +using grtest::ParamConf; using grtest::ChemPreset; using grtest::InitialUnitPreset; +static const ParamConf my_presets_[] = { + ParamConf::SimplePreset(ChemPreset::primchem0, InitialUnitPreset::simple_z0), + ParamConf::SimplePreset(ChemPreset::primchem1, InitialUnitPreset::simple_z0), + ParamConf::SimplePreset(ChemPreset::primchem2, InitialUnitPreset::simple_z0), + ParamConf::SimplePreset(ChemPreset::primchem3, InitialUnitPreset::simple_z0) +}; + INSTANTIATE_TEST_SUITE_P( /* 1st arg is intentionally empty */, APIGhostZoneTest, - ::testing::Values( - SimpleConfPreset{ChemPreset::primchem0, InitialUnitPreset::simple_z0}, - SimpleConfPreset{ChemPreset::primchem1, InitialUnitPreset::simple_z0}, - SimpleConfPreset{ChemPreset::primchem2, InitialUnitPreset::simple_z0}, - SimpleConfPreset{ChemPreset::primchem3, InitialUnitPreset::simple_z0}) + ::testing::ValuesIn(my_presets_) ); From 9e4c7dd5599ddd72a608ec473faf9231eb162832 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 24 Jan 2026 19:31:40 -0500 Subject: [PATCH 10/29] introduce GRTest_MAKE_CTX_PACK --- tests/grtestutils/googletest/fixtures.hpp | 93 +++++++++++++++++++---- 1 file changed, 77 insertions(+), 16 deletions(-) diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index 56ebd140d..db7685d53 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -24,6 +24,80 @@ #include // std::move +// C Pre-Processor weirdness requires an additional level of indirection +// if we want B to be passed __LINE__ and actually expand to a line number +#define GRTest_CONCAT_TOKEN_HELPER_(a, b) a##b +#define GRTest_CONCAT_TOKEN_(a, b) GRTest_CONCAT_TOKEN_HELPER_(a, b) + +/// Construct a GrackleCtxPack with googletest error checking +/// +/// In more detail, this calls `GrackleCtxPack::create(conf)`, performs +/// associated error-checking and then stores the result in `lhs`. This is +/// roughly analogous to +/// \code{C++} +/// RsltPairT rslt_pair = ::grtest::GrackleCtxPack::create(conf); +/// if (!rslt_pair.second.is_err()) { +/// /* handle the error with Googletest machinery */ +/// } +/// lhs = std::move(rslt_pair.first); +/// \endcode +/// In the above pseudocode, 2 simplifications were made: +/// - `RsltPairT` replaced std::pair<::grtest::GrackleCtxPack, ::grtest::Status> +/// - the variable name `rslt_pair` replaces a more unique variable name +/// +/// @param[in, out] lhs If construction of the pack succeeds, then this will +/// be on the left hand side of the final assignment expression +/// @param[in] conf Expands to eSimpleConfPreset +/// +/// @par Motivation +/// This exists so that one-off test-cases don't need to create a full-blown +/// fixture to check a given configuration (they can use this macro instead) +/// +/// @warning +/// Because the error-checking logic may trigger skipping of a test or a test +/// failure, this **MUST** be called within the body of a test-case or in the +/// `SetUp()`/`TearDown()` method of a fixture. In other words, **DO NOT** call +/// this in a helper function. For more context, see: +/// https://google.github.io/googletest/advanced.html#assertion-placement +/// +/// @par Broader Thoughts +/// I really don't like this. But, it currently seems like the most pragmatic +/// solution for now... +/// - Part of the issue is that I have ambitions to reuse the GrackleCtxPack +/// for benchmarking and possibly fuzz/property testing and I didn't want to +/// pigeonhole ourselves... +/// - Now that error-handling of `GrackleCtxPack::create` has improved, and we +/// can provide a detailed description as to why construction failed (without +/// writing much extra code), I'm starting to think that we should replace +/// this with a simple function that either returns a fully-constructed +/// GrackleCtxPack or aborts the program +/// - notably, this choice means that this gets rid of test-skipping +/// functionallity. +/// - I think the fact that this solution would abort the program rather than +/// gracefully reporting a problem is probably ok... Since error-reporting +/// is more robust, it would probably be straight-forward to write +/// alternative functions with specialized handling on a case-by-case basis +/// (e.g. for tests that check Grackle's behavior for deliberately invalid +/// configuration options) +/// - I'm inclined to stick with this macro for now, until the better handling +/// approach becomes a little more obvious... +#define GRTest_MAKE_CTX_PACK(lhs, conf) \ + /* GRTest_CONCAT_TOK(rslt, __LINE__) is the temporary variable's name */ \ + std::pair<::grtest::GrackleCtxPack, ::grtest::Status> GRTest_CONCAT_TOKEN_( \ + rslt, __LINE__) = ::grtest::GrackleCtxPack::create(conf); \ + \ + /* Check whether construction succeeded */ \ + if (GRTest_CONCAT_TOKEN_(rslt, __LINE__).second.is_missing_std_file()) { \ + GTEST_SKIP() << GRTest_CONCAT_TOKEN_(rslt, __LINE__).second.to_string(); \ + } else if (GRTest_CONCAT_TOKEN_(rslt, __LINE__).second.is_err()) { \ + FAIL() << GRTest_CONCAT_TOKEN_(rslt, __LINE__).second.to_string() \ + << " occurred for:\n" \ + << conf.stringify(false, " "); \ + } \ + \ + /* store the result in lhs */ \ + lhs = std::move(GRTest_CONCAT_TOKEN_(rslt, __LINE__).first) + namespace grtest { /// test-fixture that can used to run one or more tests with a chemistry data @@ -46,14 +120,7 @@ class ConfigPresetFixture : public testing::Test { void SetUp() override { // called immediately after the constructor (but before the test-case) ParamConf conf = ParamConf::SimplePreset(chem_preset, unit_preset); - std::pair tmp = GrackleCtxPack::create(conf); - if (tmp.second.is_ok()) { - pack = std::move(tmp.first); - } else if (tmp.second.is_missing_std_file()) { - GTEST_SKIP() << tmp.second.to_string(); - } else { - FAIL() << tmp.second.to_string(); - } + GRTest_MAKE_CTX_PACK(pack, conf); } GrackleCtxPack pack; @@ -77,14 +144,8 @@ class ParametrizedConfigPresetFixture protected: void SetUp() override { // called immediately after the constructor (but before the test-case) - std::pair tmp = GrackleCtxPack::create(GetParam()); - if (tmp.second.is_ok()) { - pack = std::move(tmp.first); - } else if (tmp.second.is_missing_std_file()) { - GTEST_SKIP() << tmp.second.to_string(); - } else { - FAIL() << tmp.second.to_string(); - } + ParamConf conf = GetParam(); + GRTest_MAKE_CTX_PACK(pack, conf); } GrackleCtxPack pack; From f56e7df8bbff69a563412d06843743fcfd470db3 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Wed, 4 Feb 2026 23:18:40 -0500 Subject: [PATCH 11/29] minor bugfix --- tests/grtestutils/param.hpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/grtestutils/param.hpp b/tests/grtestutils/param.hpp index a45881743..55ddc224f 100644 --- a/tests/grtestutils/param.hpp +++ b/tests/grtestutils/param.hpp @@ -14,6 +14,7 @@ #include "grackle.h" #include "./status.hpp" +#include // std::nullptr_t #include #include #include @@ -37,7 +38,7 @@ class ParamVal { const ParamVal& par_val, param_detail::StrAllocTracker* str_allocs); - using val_type = std::variant; + using val_type = std::variant; // private attribute val_type val_; From fa8852c5ec96c8f2b263f293dbbe1bd3a5727829 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sun, 1 Feb 2026 23:32:22 -0500 Subject: [PATCH 12/29] adjust preset --- tests/grtestutils/preset.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/preset.cpp index 07bca4a06..81b072134 100644 --- a/tests/grtestutils/preset.cpp +++ b/tests/grtestutils/preset.cpp @@ -51,8 +51,7 @@ std::pair, Status> get_chem_preset_vals_( } std::vector> v = make_ParamPair_vec({ - {"use_grackle", 1}, // chemistry on - {"use_isrf_field", 1}, + {"use_grackle", 1}, // chemistry on {"with_radiative_cooling", 1}, // cooling on {"metal_cooling", 1}, // metal cooling on {"UVbackground", 1}, // UV background on From f2e7ec0973f04aa6278a8cb5175aa3dfba887018 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Wed, 4 Feb 2026 23:44:19 -0500 Subject: [PATCH 13/29] another minor bugfix --- tests/grtestutils/param.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/grtestutils/param.cpp b/tests/grtestutils/param.cpp index 7feb0de1a..e6834054c 100644 --- a/tests/grtestutils/param.cpp +++ b/tests/grtestutils/param.cpp @@ -74,7 +74,7 @@ bool ParamVal::is_equal(std::string_view val) const { bool ParamVal::is_equal(const char* val) const { if (val == nullptr) { - return std::holds_alternative(val_); + return std::holds_alternative(val_); } return is_equal(std::string_view(val)); } From db2693d1e8c761fcfe1334f85a3ed63dc8d8718c Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 5 Feb 2026 00:14:41 -0500 Subject: [PATCH 14/29] hopefully this is the last fix --- tests/grtestutils/param.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/grtestutils/param.hpp b/tests/grtestutils/param.hpp index 55ddc224f..90473e8d6 100644 --- a/tests/grtestutils/param.hpp +++ b/tests/grtestutils/param.hpp @@ -85,7 +85,7 @@ class ParamVal { /// Returns whether `this` holds an empty string bool is_empty_string() const { - return std::holds_alternative(val_); + return std::holds_alternative(val_); } /*!{*/ From 207e3412e3e1274f4678f614aa3f01da8c774c73 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 12 Feb 2026 20:28:19 -0500 Subject: [PATCH 15/29] rearranging files --- tests/grtestutils/CMakeLists.txt | 9 +++++---- tests/grtestutils/googletest/fixtures.hpp | 6 +++--- .../grackle_ctx_pack.cpp} | 2 +- .../grackle_ctx_pack.hpp} | 0 tests/grtestutils/{ => harness}/param.cpp | 11 ++++++----- tests/grtestutils/{ => harness}/param.hpp | 0 tests/grtestutils/{ => harness}/preset.cpp | 2 +- tests/grtestutils/{ => harness}/preset.hpp | 0 tests/grtestutils/{ => harness}/status.cpp | 0 tests/grtestutils/{ => harness}/status.hpp | 0 tests/grtestutils/iterator_adaptor.hpp | 3 ++- 11 files changed, 18 insertions(+), 15 deletions(-) rename tests/grtestutils/{GrackleCtxPack.cpp => harness/grackle_ctx_pack.cpp} (98%) rename tests/grtestutils/{GrackleCtxPack.hpp => harness/grackle_ctx_pack.hpp} (100%) rename tests/grtestutils/{ => harness}/param.cpp (95%) rename tests/grtestutils/{ => harness}/param.hpp (100%) rename tests/grtestutils/{ => harness}/preset.cpp (99%) rename tests/grtestutils/{ => harness}/preset.hpp (100%) rename tests/grtestutils/{ => harness}/status.cpp (100%) rename tests/grtestutils/{ => harness}/status.hpp (100%) diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index 3cd85dc2b..ca218faa1 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -80,14 +80,15 @@ add_library(grtestutils # files outside of the googletest subdirectory # -> these shouldn't include headers from the googletest subdirectory cmd.hpp cmd.cpp - GrackleCtxPack.cpp GrackleCtxPack.hpp iterator_adaptor.hpp os.hpp os.cpp - param.cpp param.hpp - preset.cpp preset.hpp - status.cpp status.hpp utils.hpp utils.cpp + harness/grackle_ctx_pack.cpp harness/grackle_ctx_pack.hpp + harness/param.cpp harness/param.hpp + harness/preset.cpp harness/preset.hpp + harness/status.cpp harness/status.hpp + # files in the googletest subdirectory (reminder: should just be headers!) googletest/check_allclose.hpp googletest/fixtures.hpp diff --git a/tests/grtestutils/googletest/fixtures.hpp b/tests/grtestutils/googletest/fixtures.hpp index db7685d53..4886e0a29 100644 --- a/tests/grtestutils/googletest/fixtures.hpp +++ b/tests/grtestutils/googletest/fixtures.hpp @@ -18,9 +18,9 @@ // 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 "../GrackleCtxPack.hpp" -#include "../preset.hpp" -#include "../status.hpp" +#include "../harness/grackle_ctx_pack.hpp" +#include "../harness/preset.hpp" +#include "../harness/status.hpp" #include // std::move diff --git a/tests/grtestutils/GrackleCtxPack.cpp b/tests/grtestutils/harness/grackle_ctx_pack.cpp similarity index 98% rename from tests/grtestutils/GrackleCtxPack.cpp rename to tests/grtestutils/harness/grackle_ctx_pack.cpp index 487bc96b7..e135d298c 100644 --- a/tests/grtestutils/GrackleCtxPack.cpp +++ b/tests/grtestutils/harness/grackle_ctx_pack.cpp @@ -10,7 +10,7 @@ /// //===----------------------------------------------------------------------===// -#include "./GrackleCtxPack.hpp" +#include "./grackle_ctx_pack.hpp" #include "./preset.hpp" namespace grtest { diff --git a/tests/grtestutils/GrackleCtxPack.hpp b/tests/grtestutils/harness/grackle_ctx_pack.hpp similarity index 100% rename from tests/grtestutils/GrackleCtxPack.hpp rename to tests/grtestutils/harness/grackle_ctx_pack.hpp diff --git a/tests/grtestutils/param.cpp b/tests/grtestutils/harness/param.cpp similarity index 95% rename from tests/grtestutils/param.cpp rename to tests/grtestutils/harness/param.cpp index e6834054c..3eb293ec8 100644 --- a/tests/grtestutils/param.cpp +++ b/tests/grtestutils/harness/param.cpp @@ -11,11 +11,12 @@ //===----------------------------------------------------------------------===// #include "status_reporting.h" -#include "param.hpp" -#include "GrackleCtxPack.hpp" // param_detail::StrAllocTracker -#include // std::strlen -#include // std::ostream -#include // std::false_type, std::is_same_v, std::decay_t +#include "./param.hpp" +#include "./grackle_ctx_pack.hpp" // param_detail::StrAllocTracker + +#include // std::strlen +#include // std::ostream +#include // std::false_type, std::is_same_v, std::decay_t namespace { // stuff inside an anonymous namespace is local to this file // this is used to report a compile-time error in the lambda function that diff --git a/tests/grtestutils/param.hpp b/tests/grtestutils/harness/param.hpp similarity index 100% rename from tests/grtestutils/param.hpp rename to tests/grtestutils/harness/param.hpp diff --git a/tests/grtestutils/preset.cpp b/tests/grtestutils/harness/preset.cpp similarity index 99% rename from tests/grtestutils/preset.cpp rename to tests/grtestutils/harness/preset.cpp index 81b072134..f98a79838 100644 --- a/tests/grtestutils/preset.cpp +++ b/tests/grtestutils/harness/preset.cpp @@ -12,7 +12,7 @@ #include "./param.hpp" #include "./preset.hpp" -#include "./utils.hpp" +#include "../utils.hpp" #include "grackle.h" #include "status_reporting.h" // GR_INTERNAL_UNREACHABLE_ERROR diff --git a/tests/grtestutils/preset.hpp b/tests/grtestutils/harness/preset.hpp similarity index 100% rename from tests/grtestutils/preset.hpp rename to tests/grtestutils/harness/preset.hpp diff --git a/tests/grtestutils/status.cpp b/tests/grtestutils/harness/status.cpp similarity index 100% rename from tests/grtestutils/status.cpp rename to tests/grtestutils/harness/status.cpp diff --git a/tests/grtestutils/status.hpp b/tests/grtestutils/harness/status.hpp similarity index 100% rename from tests/grtestutils/status.hpp rename to tests/grtestutils/harness/status.hpp diff --git a/tests/grtestutils/iterator_adaptor.hpp b/tests/grtestutils/iterator_adaptor.hpp index c992b9490..ddb510558 100644 --- a/tests/grtestutils/iterator_adaptor.hpp +++ b/tests/grtestutils/iterator_adaptor.hpp @@ -16,7 +16,8 @@ #include #include "grackle.h" -#include "GrackleCtxPack.hpp" + +#include "./harness/grackle_ctx_pack.hpp" namespace grtest { From da61fbed71778dc4fc7588225ff895cf4e63aa31 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sun, 1 Feb 2026 19:45:07 -0500 Subject: [PATCH 16/29] move/adjust string formatting routines --- .../grtestutils/googletest/check_allclose.hpp | 69 ++++--------------- tests/grtestutils/utils.cpp | 6 ++ tests/grtestutils/utils.hpp | 55 +++++++++++++++ 3 files changed, 76 insertions(+), 54 deletions(-) diff --git a/tests/grtestutils/googletest/check_allclose.hpp b/tests/grtestutils/googletest/check_allclose.hpp index 3147f05cb..31d64645c 100644 --- a/tests/grtestutils/googletest/check_allclose.hpp +++ b/tests/grtestutils/googletest/check_allclose.hpp @@ -6,50 +6,7 @@ #include #include -/// equivalent of %g (this is extremely crude!) -std::string pretty_format_(double val) { - char buf[30]; - snprintf(buf, 30, "%g", val); - return std::string(buf); -} - -/// formats a std::vector as a string -/// -/// @note -/// This is highly inefficient, partially because it consists of code written -/// from before we adopted googletest -inline std::string vec_to_string(const std::vector& vec) { - std::string out = "{"; - - std::size_t len = vec.size(); - - std::size_t pause_start; - std::size_t pause_stop; - - if (len > 30){ - pause_start = 3; - pause_stop = len - 3; - } else { - pause_start = len *2; - pause_stop = pause_start; - } - - for (std::size_t i = 0; i < len; i++) { - if ((i > pause_start) && (i < pause_stop)) { continue; } - - if (i == pause_stop) { - out += ", ... "; - } else if (i != 0) { - out += ", "; - } - - const int BUF_SIZE = 30; - char buf[BUF_SIZE]; - snprintf(buf, BUF_SIZE, "%g", vec[i]); - out += buf; - } - return out + "}"; -} +#include "grtestutils/utils.hpp" /// this compares 2 std::vectors /// @@ -107,24 +64,28 @@ inline testing::AssertionResult check_allclose( if (num_mismatches == 0) { return testing::AssertionSuccess(); } - std::string actual_vec_str = vec_to_string(actual); - std::string ref_vec_str = vec_to_string(desired); + std::string actual_vec_str = grtest::ptr_to_string(actual.data(), + actual.size()); + std::string ref_vec_str = grtest::ptr_to_string(desired.data(), + desired.size()); + + using grtest::to_pretty_string; return testing::AssertionFailure() << "\narrays are unequal for the tolerance: " - << "rtol = " << pretty_format_(rtol) << ", " - << "atol = " << pretty_format_(atol) << '\n' + << "rtol = " << to_pretty_string(rtol) << ", " + << "atol = " << to_pretty_string(atol) << '\n' << err_msg << '\n' // custom error message << "Mismatched elements: " << num_mismatches << " / " << actual.size() << '\n' - << "Max absolute difference: " << pretty_format_(max_absDiff) << ", " + << "Max absolute difference: " << to_pretty_string(max_absDiff) << ", " << "ind = " << max_absDiff_ind << ", " - << "actual = " << pretty_format_(actual[max_absDiff_ind]) << ", " - << "reference = " << pretty_format_(desired[max_absDiff_ind]) << '\n' - << "Max relative difference: " << pretty_format_(max_relDiff) << ", " + << "actual = " << to_pretty_string(actual[max_absDiff_ind]) << ", " + << "reference = " << to_pretty_string(desired[max_absDiff_ind]) << '\n' + << "Max relative difference: " << to_pretty_string(max_relDiff) << ", " << "ind = " << max_absDiff_ind << ", " - << "actual = " << pretty_format_(actual[max_relDiff_ind]) << ", " - << "desired = " << pretty_format_(desired[max_relDiff_ind]) << '\n' + << "actual = " << to_pretty_string(actual[max_relDiff_ind]) << ", " + << "desired = " << to_pretty_string(desired[max_relDiff_ind]) << '\n' << "actual: " << actual_vec_str << '\n' << "desired: " << ref_vec_str << '\n'; } diff --git a/tests/grtestutils/utils.cpp b/tests/grtestutils/utils.cpp index 3665911f4..2efd03a56 100644 --- a/tests/grtestutils/utils.cpp +++ b/tests/grtestutils/utils.cpp @@ -49,4 +49,10 @@ std::optional grtest::get_standard_datafile(const char* datafile) { return std::nullopt; } return std::optional(tmp); +} + +std::string grtest::to_pretty_string(double val) { + char buf[30]; + snprintf(buf, 30, "%g", val); + return std::string(buf); } \ No newline at end of file diff --git a/tests/grtestutils/utils.hpp b/tests/grtestutils/utils.hpp index b0fc12bcc..dae9d1623 100644 --- a/tests/grtestutils/utils.hpp +++ b/tests/grtestutils/utils.hpp @@ -8,6 +8,7 @@ #include #include #include +#include // std::is_floating_point_v namespace grtest { @@ -54,4 +55,58 @@ inline double uniform_dist_transform(std::minstd_rand &generator) { } // namespace grtest::random + +namespace grtest { + +/// equivalent of %g (the value should be fully specified) +/// +/// The current implementation is extremely crude! +std::string to_pretty_string(double val); + +inline std::string to_pretty_string(float val) { + // this is crude! + return to_pretty_string(static_cast(val)); +} + +/// formats a std::vector as a string +/// +/// @note +/// This is highly inefficient, partially because it consists of code written +/// from before we adopted googletest +template +std::string ptr_to_string(const T* ptr, std::size_t len) { + static_assert(std::is_floating_point_v); + + std::string out = "{"; + + std::size_t pause_start; + std::size_t pause_stop; + + if (len > 30){ + pause_start = 3; + pause_stop = len - 3; + } else { + pause_start = len *2; + pause_stop = pause_start; + } + + for (std::size_t i = 0; i < len; i++) { + if ((i > pause_start) && (i < pause_stop)) { continue; } + + if (i == pause_stop) { + out += ", ... "; + } else if (i != 0) { + out += ", "; + } + + const int BUF_SIZE = 30; + char buf[BUF_SIZE]; + snprintf(buf, BUF_SIZE, "%g", ptr[i]); + out += buf; + } + return out + "}"; +} + +} // namespace grtest + #endif /* GRTEST_UTILS_HPP */ From d5e05b1dedccab88c2b90f17bdce895ec9669886 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sun, 25 Jan 2026 14:43:22 -0500 Subject: [PATCH 17/29] Initial introduction of FieldContainer --- tests/grtestutils/CMakeLists.txt | 2 + tests/grtestutils/harness/field_container.cpp | 239 ++++++++++ tests/grtestutils/harness/field_container.hpp | 172 +++++++ .../grtestutils/harness/field_info_detail.cpp | 89 ++++ .../grtestutils/harness/field_info_detail.hpp | 422 ++++++++++++++++++ .../grtestutils/harness/grackle_ctx_pack.hpp | 3 + 6 files changed, 927 insertions(+) create mode 100644 tests/grtestutils/harness/field_container.cpp create mode 100644 tests/grtestutils/harness/field_container.hpp create mode 100644 tests/grtestutils/harness/field_info_detail.cpp create mode 100644 tests/grtestutils/harness/field_info_detail.hpp diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index ca218faa1..b8de75ded 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -85,6 +85,8 @@ add_library(grtestutils utils.hpp utils.cpp harness/grackle_ctx_pack.cpp harness/grackle_ctx_pack.hpp + harness/field_container.cpp harness/field_container.hpp + harness/field_info_detail.cpp harness/field_info_detail.hpp harness/param.cpp harness/param.hpp harness/preset.cpp harness/preset.hpp harness/status.cpp harness/status.hpp diff --git a/tests/grtestutils/harness/field_container.cpp b/tests/grtestutils/harness/field_container.cpp new file mode 100644 index 000000000..d97f22073 --- /dev/null +++ b/tests/grtestutils/harness/field_container.cpp @@ -0,0 +1,239 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Implement logic pertaining to @ref FieldContainer +/// +//===----------------------------------------------------------------------===// + +#include "./grackle_ctx_pack.hpp" +#include "./field_container.hpp" +#include "./field_info_detail.hpp" +#include "./status.hpp" + +#include "grackle.h" +#include "status_reporting.h" + +#include // std::memcmp, std::memcpy +#include // std::pair, std::move + +namespace grtest { +namespace field_detail { + +/// Construct a name-pointer mapping based on @p ctx_pack +/// +/// The returned where the keys are the names of every active Grackle field +/// and the associated values are all nullptr +static MapType make_nullptr_map_(const GrackleCtxPack& ctx_pack, + bool exclude_metal) { + MapType m; + // fill up m with (field, nullptr) pairs for each field enabled by ctx_pack + auto fn = [&m](const char* name, const FieldInfo& info) { + if (!m.emplace(name, nullptr).second) { + GR_INTERNAL_ERROR("%s was inserted multiple times", name); + } + }; + for_each_named_field(ctx_pack, fn); + + if (exclude_metal) { + auto search = m.find("metal_density"); + if (search != m.end()) { + m.erase(search); + } + } + return m; +} + +/// Construct a `CorePack` instance by consuming @p premade_map +/// +/// @param premade_map A string to pointer mapping. The keys of this argument +/// should specify the names for each desired Grackle field. We assume that +/// the pointers associated with each key hold meaningless garbage values +/// @param buf_size The number of elements to allocate per field +static std::pair setup_1d_CorePack(MapType&& premade_map, + int buf_size) { + if (buf_size <= 0) { + return {CorePack{}, error::Adhoc("buf_size must be positive")}; + } + std::size_t data_sz = premade_map.size() * buf_size; + + CorePack pack{ + // the trailing parentheses when allocating an array of integers or + // floating point values sets the initial values to 0 + /* prop_buf */ std::unique_ptr(new int[9]()), + /* data_buf */ std::unique_ptr(new gr_float[data_sz]()), + /* my_fields */ + std::unique_ptr(new grackle_field_data), + /* map */ std::move(premade_map)}; + if (gr_initialize_field_data(pack.my_fields.get()) != GR_SUCCESS) { + // this really should never fail + return {CorePack{}, error::Adhoc("gr_initialize_field_data failed")}; + } + + // setup grid properties + pack.my_fields->grid_dimension = pack.prop_buf.get(); + pack.my_fields->grid_start = pack.prop_buf.get() + 3; + pack.my_fields->grid_end = pack.prop_buf.get() + 6; + + pack.my_fields->grid_rank = 1; + pack.my_fields->grid_dimension[0] = buf_size; + pack.my_fields->grid_end[0] = buf_size - 1; + + // distribute allocated field-memory among all the appropriate slots of + // pack.my_fields and pack.map + gr_float* ptr = pack.data_buf.get(); + int counter = 0; + auto fn = [ptr, buf_size, &counter, &pack](const char* name, + const FieldInfo& finfo) { + // return immediately if finfo.name isn't a key of `pack.map` + MapType::iterator search = pack.map.find(name); + if (search == pack.map.end()) { + return; // finfo.name isn't a key of `map` + } + // get the pointer to the memory reserved for the current field + gr_float* cur_ptr = ptr + (counter * buf_size); + counter++; + // update `pack.my_fields` to associate the current field with `cur_ptr` + pack.my_fields.get()->*finfo.relative_addr = cur_ptr; + // update `map` to associate the current field with `cur_ptr` + search->second = cur_ptr; + }; + // this acts like a for-loop that passes a FieldInfo struct for every known + // grackle-field into `fn` + for_each_named_field(fn); + return {std::move(pack), OkStatus()}; +} + +} // namespace field_detail + +std::pair FieldContainer::create_1d( + const GrackleCtxPack& ctx_pack, int buf_size, bool disable_metal) { + if (!ctx_pack.is_initialized()) { + return {FieldContainer{}, error::Adhoc("ctx_pack isn't initialized")}; + } + + // construct a map for each relevant field (the values are all nullptr) + field_detail::MapType m = + field_detail::make_nullptr_map_(ctx_pack, disable_metal); + std::pair tmp = + field_detail::setup_1d_CorePack(std::move(m), buf_size); + if (tmp.second.is_err()) { + return {FieldContainer(), std::move(tmp.second)}; + } else { + FieldContainer fc; + fc.data_ = std::move(tmp.first); + return {std::move(fc), OkStatus()}; + } +} + +std::pair FieldContainer::create( + const GrackleCtxPack& ctx_pack, const GridLayout& layout, + bool disable_metal) { + if (layout.rank < 1 || layout.rank > 3) { + return {FieldContainer(), error::Adhoc("layout.rank must be 1, 2, or 3")}; + } + int total_count = 1; + for (int i = 0; i < layout.rank; i++) { + if (layout.dimension[i] <= 0) { + return {FieldContainer(), + error::Adhoc("layout.dimension[i] must be positive")}; + } else if (layout.ghost_depth[i] < 0) { + return {FieldContainer(), + error::Adhoc("layout.ghost_depth[i] is negative")}; + } else if (layout.ghost_depth[i] * 2 >= layout.dimension[i]) { + return {FieldContainer(), + error::Adhoc( + "layout.dimension[i] must exceed 2*layout.ghost_depth[i]")}; + } + total_count *= layout.dimension[i]; + } + + std::pair tmp = + FieldContainer::create_1d(ctx_pack, total_count, disable_metal); + + if (tmp.second.is_ok()) { + grackle_field_data* my_fields = tmp.first.data_.my_fields.get(); + tmp.first.data_.my_fields->grid_rank = layout.rank; + for (int i = 0; i < layout.rank; i++) { + int dim = layout.dimension[i]; + int ghost_depth = layout.ghost_depth[i]; + my_fields->grid_dimension[i] = dim; + my_fields->grid_start[i] = ghost_depth; + my_fields->grid_end[i] = dim - ghost_depth - 1; + } + } + return tmp; +} + +FieldContainer FieldContainer::clone() const { + // make a copy of m, and use it construct a new CorePack + // -> the keys of `m` tell `setup_1d_CorePack` which Grackle fields get used + // -> right after we create + field_detail::MapType m = this->data_.map; + std::pair tmp = + field_detail::setup_1d_CorePack(std::move(m), this->elements_per_field()); + // it shouldn't be possible for tmp.second.is_err() to return true + FieldContainer out; + out.data_ = std::move(tmp.first); + + // copy over layout properties + int rank = this->rank(); + for (int i = 0; i < rank; i++) { + out.data_.my_fields->grid_start[i] = this->data_.my_fields->grid_start[i]; + out.data_.my_fields->grid_end[i] = this->data_.my_fields->grid_end[i]; + out.data_.my_fields->grid_dimension[i] = + this->data_.my_fields->grid_dimension[i]; + } + + copy_into_helper_(out); + return out; +} + +void FieldContainer::copy_into_helper_(FieldContainer& other) const { + // I'm still not entirely sure how to best handle grid_dx, but this is here + // to make sure we don't forget to propagate the logic when we ultimately + // make a decision + if (this->get_ptr()->grid_dx != other.get_ptr()->grid_dx) { + GR_INTERNAL_ERROR("did you forget to update grid_dx handling?"); + } + + const gr_float* src = this->data_.data_buf.get(); + gr_float* dst = other.data_.data_buf.get(); + int length = this->elements_per_field() * this->n_fields(); + std::memcpy(dst, src, length * sizeof(gr_float)); +} + +bool FieldContainer::same_grid_props(const FieldContainer& other) const { + const grackle_field_data* a = this->data_.my_fields.get(); + const grackle_field_data* b = other.data_.my_fields.get(); + + if (a->grid_rank != b->grid_rank) { + return false; + } + int rank = a->grid_rank; + std::size_t sz = rank * sizeof(int); + bool s = std::memcmp(a->grid_start, b->grid_start, sz) == 0; + bool e = std::memcmp(a->grid_end, b->grid_end, sz) == 0; + bool d = std::memcmp(a->grid_dimension, b->grid_dimension, sz) == 0; + return s && d && e; +} + +bool FieldContainer::same_fields(const FieldContainer& other) const { + // this takes advantage of the fact that MapType is sorted + MapType::const_iterator it_a = this->data_.map.begin(); + MapType::const_iterator stop_a = this->data_.map.end(); + MapType::const_iterator it_b = other.data_.map.begin(); + + for (; it_a != stop_a; ++it_a, ++it_b) { + if (it_a->first != it_b->first) { + return false; + } + } + return true; +} + +} // namespace grtest \ No newline at end of file diff --git a/tests/grtestutils/harness/field_container.hpp b/tests/grtestutils/harness/field_container.hpp new file mode 100644 index 000000000..4a6555833 --- /dev/null +++ b/tests/grtestutils/harness/field_container.hpp @@ -0,0 +1,172 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Declare the FieldContainer class +/// +//===----------------------------------------------------------------------===// +#ifndef GRTESTUTILS_FIELD_CONTAINER_HPP +#define GRTESTUTILS_FIELD_CONTAINER_HPP + +#include "grackle.h" + +#include "./grackle_ctx_pack.hpp" +#include "./status.hpp" + +#include // std::less +#include +#include // std::map +#include +#include +#include +#include // std::pair + +namespace grtest { + +namespace field_detail { + +inline int elements_per_field_(const grackle_field_data& my_fields) { + int total = 1; + for (int i = 0; i < my_fields.grid_rank; i++) { + total *= my_fields.grid_dimension[i]; + } + return total; +} + +// by including std::less<> in the configuration, we ensure that we can perform +// a lookup with a string +using MapType = std::map>; + +/// Holds the core data of a FieldContainer +/// +/// @par Implementation Notes +/// This holds a @ref grackle_field_data instance, associated buffers, and a +/// string-buffer mapping. +/// +/// Consider a case where @ref my_fields is fully initialized: +/// - each pointer data member for specifying grid properties refers to a +/// non-overlapping segment of the buffer tracked by @ref prop_buf +/// - each pointer data member for specifying an unused grackle-field is holds +/// a nullptr +/// - each pointer data member for specifying an actively used grackle-field: +/// - holds an associated pointer to a non-overlapping segment of the buffer +/// tracked by @ref data_buf. The numer of elements per segment is given by +/// `elements_per_field_(*my_fields)`. +/// - has a corresponding key-value pair tracked by @ref map, where the key +/// is the name of the grackle-field, and the value is associated pointer +struct CorePack { + /// the buffer used for holding field shape and ghost zones + std::unique_ptr prop_buf; + /// the buffer used to hold all field data + std::unique_ptr data_buf; + /// the struct understood by Grackle + std::unique_ptr my_fields; + /// maps field names to pointers + MapType map; +}; + +} // namespace field_detail + +/// Encapsulates an arbitrary GridLayout +struct GridLayout { + int rank; + int dimension[3]; + int ghost_depth[3]; +}; + +/// A container wrapping grackle_field_data that owns the underlying data +/// +/// @par Implementation Notes +/// The current implementation attempts to be a general-purpose test-harness +/// that can be used for testing and for benchmarking. At the moment, +/// construction is a little slow. That's ok for benchmarking since @ref +/// copy_into is fast (it's effectively just a single memcpy) +/// +/// @note +/// At the moment, I have explicitly avoided addressing grid_dx +class FieldContainer { + field_detail::CorePack data_; + + /// Can only be invoked by a factory method + FieldContainer() = default; + + void copy_into_helper_(FieldContainer& other) const; + +public: + using MapType = field_detail::MapType; + + FieldContainer(const FieldContainer& other) = delete; + FieldContainer& operator=(const FieldContainer& other) = delete; + FieldContainer(FieldContainer&& other) = default; + FieldContainer& operator=(FieldContainer&& other) = default; + ~FieldContainer() = default; + + /// A factory method to make a simple 1d container + static std::pair create_1d( + const GrackleCtxPack& ctx_pack, int buf_size, bool disable_metal = false); + + /// A factory method to make a container + static std::pair create( + const GrackleCtxPack& ctx_pack, const GridLayout& layout, + bool disable_metal = false); + + /// Create a clone of FieldContainer + FieldContainer clone() const; + + /// Overwrite the field data in @p dest with the field data from `this` + /// + /// This doesn't perform any allocations. + /// + /// @warning + /// Setting bypass_check to `true` is risky. It primarily exists for the + /// case where you call this method in a loop + Status copy_into(FieldContainer& dest, bool bypass_check = false) const { + if (!(bypass_check || this->same_grid_props(dest))) { + return error::Adhoc("grid properties are incompatible"); + } else if (!(bypass_check || this->same_fields(dest))) { + return error::Adhoc("field sets are incompatible"); + } + this->copy_into_helper_(dest); + return OkStatus(); + } + + /// returns whether `this` and @p other contains the same grid properties + bool same_grid_props(const FieldContainer& other) const; + + /// returns whether `this` and @p other contains the same set of fields + bool same_fields(const FieldContainer& other) const; + + const grackle_field_data* get_ptr() const { return data_.my_fields.get(); } + grackle_field_data* get_ptr() { return data_.my_fields.get(); } + + std::optional find(std::string_view key) { + auto s = data_.map.find(key); + return (s != data_.map.end()) ? std::optional{s->second} : std::nullopt; + } + + std::optional find(std::string_view key) const { + auto s = data_.map.find(key); + return (s != data_.map.end()) ? std::optional{s->second} : std::nullopt; + } + + int n_fields() const { return static_cast(data_.map.size()); } + + int rank() const { return data_.my_fields->grid_rank; } + + /// the number of elements per field (unaffected by ghost zones) + int elements_per_field() const { + return field_detail::elements_per_field_(*data_.my_fields); + } + + // we define both of the following methods to support the writing of + // range-based for-loops to iterate over key-buffer pairs + MapType::const_iterator begin() const { return data_.map.begin(); } + MapType::const_iterator end() const { return data_.map.end(); } +}; +} // namespace grtest + +#endif // GRTESTUTILS_FIELD_CONTAINER_HPP diff --git a/tests/grtestutils/harness/field_info_detail.cpp b/tests/grtestutils/harness/field_info_detail.cpp new file mode 100644 index 000000000..f847c2adb --- /dev/null +++ b/tests/grtestutils/harness/field_info_detail.cpp @@ -0,0 +1,89 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// help implement logic pertaining to query_field_info +/// +//===----------------------------------------------------------------------===// + +#include "./field_info_detail.hpp" + +#include +#include +#include +#include // std::pair + +namespace grtest::field_detail { + +namespace { // stuff inside an anonymous namespace is local to this file + +constexpr int get_known_field_count_() { + int i = 0; + for_each_named_field([&i](const char* name, const FieldInfo& finfo) { i++; }); + return i; +} + +/// Holds the number of known grackle fields (known at compile-time) +constexpr int N_KNOWN_FIELDS = get_known_field_count_(); + +using FieldNameInfoArray = + std::array, N_KNOWN_FIELDS>; + +constexpr FieldNameInfoArray make_name_info_array() { + FieldNameInfoArray out; + std::size_t i = 0; + for_each_named_field([&out, &i](const char* name, const FieldInfo& finfo) { + std::pair& cur_pair = out[i++]; + cur_pair.first = std::string_view(name); + cur_pair.second = finfo; + }); + return out; +} + +/// Holds the number of known grackle fields +/// +/// @note Computed at compile-time +constexpr FieldNameInfoArray name_info_array = make_name_info_array(); + +/// A mapping between known all known field names and FieldInfo structs +/// +/// @par Startup Overhead +/// Each time an executable linked against this file starts up, this mapping is +/// constructed before the main function gets executed. It's unfortunate that +/// we have this overhead: +/// - To reduce the overhead, we compute the key-value pairs at compile-time +/// - The best alternative would involve constructing this mapping the very +/// first time we invoke query_field_info. We probably would want to use +/// something like std::call_once (but we need to be careful about colliding +/// threading runtimes) +/// +/// @par Implementation Note +/// Ordinarily, C++ containers should avoid holding std::string_view instances +/// since a std::string_view isn't tied to an underlying allocation, which can +/// lead to unexpected lifetime issues. In this case +/// - the use of std::string_view is ok because +/// 1. each value is a string-literal (i.e. there is no way for the underlying +/// memory to be de-allocated) +/// 2. this variable is constant (i.e. there is no way for keys to be added +/// that don't wrap string-literals) +/// - moreover, the only alternative is to use std::string and that would +/// involve heap allocations (adding to startup overhead) +/// +/// @note +const std::unordered_map name_info_map_( + name_info_array.begin(), name_info_array.end()); + +} // anonymous namespace + +std::optional query_field_info(std::string_view name) noexcept { + auto search = name_info_map_.find(name); + return (search == name_info_map_.end()) + ? std::optional{} + : std::optional{search->second}; +} + +} // namespace grtest::field_detail \ No newline at end of file diff --git a/tests/grtestutils/harness/field_info_detail.hpp b/tests/grtestutils/harness/field_info_detail.hpp new file mode 100644 index 000000000..56875e899 --- /dev/null +++ b/tests/grtestutils/harness/field_info_detail.hpp @@ -0,0 +1,422 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Tracks machinery to query field information +/// +//===----------------------------------------------------------------------===// +#ifndef GRTESTUTILS_FIELD_INFO_DETAIL_HPP +#define GRTESTUTILS_FIELD_INFO_DETAIL_HPP + +#include "grackle.h" +#include "status_reporting.h" + +#include "./grackle_ctx_pack.hpp" + +#include +#include + +/// @defgroup FieldInfo Field Info Logic +/// +/// This group all logic of the testing harness pertaining to both: +/// 1. Enumerate all fields within @ref grackle_field_data and associate +/// them with string names (to facillitate easy programatic access). +/// 2. Determine the fields Grackle expects from the current configuration. +/// +/// Changes to @ref grackle_field_data should *only* directly impact the +/// test-harness logic in this group. +/// +/// Longer Term Goal +/// ================ +/// The longer term goal is to facillitate benchmarking with the test harness +/// logic and to make it possible to compile the test-harness logic with older +/// versions of Grackle. Certain design-decisions were made in service of this +/// goal. +/// +/// Primer on Pointer-to-Data-Member +/// ================================ +/// Some of this logic makes use of C++'s Pointer-to-Member functionallity (to +/// be precise, we describe a pointer to a data member). This section provides +/// a brief overview on this feature. +/// +/// Just as a function-pointer is a special kind of C pointer, a +/// pointer-to-data-member is a special kind of C++ pointer. +/// +/// A pointer-to-data-member is used in C++ to describe the address of a +/// struct/class's data member relative to the start of a struct. It is used +/// for to write code that in pure C might be written using `offsetof`. +/// +/// For concreteness, consider the following pure C example, where `offsetof` +/// is used to access a value of the struct `S`. +/// @code{C} +/// #include +/// #include +/// #include +/// +/// struct S {char c, double d; double e; }; +/// +/// void print_e(struct S s) { +/// double tmp = 0.0; +/// memcpy(&tmp, (char*)s + offsetof(struct S, e), sizeof(double)); +/// printf("s.e = %f\n", tmp); +/// } +/// @endcode +/// +/// In contrast, the analogous C++ program using a pointer-to-data-member might +/// look something like the following snippet: +/// @code{C++} +/// #include +/// +/// struct S {char c, double d; double e; }; +/// +/// void print_e(S s) { +/// double S::* ptr_to_member = &S::e; +/// std::printf("s.e = %f\n", s.*ptr_to_member); +/// } +/// @endcode +/// In this snippet the `ptr_to_member` variable holds a pointer-to-data-member. +/// Specifically it can hold a pointer-to-data-member that refers to any data +/// member of type `double` in the struct `S`. In other words, it could hold +/// either `&S::d` OR `&S::e` +/** @{ */ + +namespace grtest::field_detail { + +enum class CmpOp { EQ, GEQ }; + +/// a requirement for enabling a field (or group of fields) +/// +/// @note +/// For performance reasons, @ref param is a pointer-to-data-member. Longer +/// term, we may need to convert it to a string and use the dynamic API +struct Req { + int chemistry_data::* param; + CmpOp op; + int rhs; +}; + +/// we implicitly assume that my_chem isn't a nullptr +inline bool check_req_(const chemistry_data& my_chem, const Req& req) { + // ugh, this const_cast isn't great but, it's ok for our purposes + int val = my_chem.*req.param; + switch (req.op) { + case CmpOp::EQ: + return val == req.rhs; + case CmpOp::GEQ: + return val >= req.rhs; + default: + GR_INTERNAL_UNREACHABLE_ERROR(); + } +} + +/// This classifies fields for the purpose of initialization +enum struct Kind { UNSET, PRIMORDIAL_SPECIES, ELEMENTWISE_SOLVER_ARG }; + +/// Holds information about a field member of @ref grackle_field_data +/// +/// @par Further Context about Pointer-to-Data-Member +/// For the uninitiated, @ref FieldInfo::relative_addr is a special +/// kind of pointer called a pointer-to-data-member. We provide a brief primer +/// on this logic up above. +struct FieldInfo { + gr_float* grackle_field_data::* relative_addr; ///< address of the member + Kind kind; ///< field kind +}; + +/// helps implement @ref for_each_named_field +template +constexpr void for_each_named_field_(CheckReqFn check_req, Fn fn) { + using FInfo = FieldInfo; + +#define MK_REQ(param_name, op, RHS) Req{&chemistry_data::param_name, op, RHS} + + fn("density", FInfo{&grackle_field_data::density, Kind::UNSET}); + fn("internal_energy", + FInfo{&grackle_field_data::internal_energy, Kind::UNSET}); + + if (check_req(MK_REQ(metal_cooling, CmpOp::EQ, 1))) { + fn("metal_density", FInfo{&grackle_field_data::metal_density, Kind::UNSET}); + } + + if (check_req(MK_REQ(use_dust_density_field, CmpOp::EQ, 1))) { + fn("dust_density", FInfo{&grackle_field_data::dust_density, Kind::UNSET}); + } + + if (check_req(MK_REQ(primordial_chemistry, CmpOp::GEQ, 1))) { + fn("e_density", + FInfo{&grackle_field_data::e_density, Kind::PRIMORDIAL_SPECIES}); + fn("HI_density", + FInfo{&grackle_field_data::HI_density, Kind::PRIMORDIAL_SPECIES}); + fn("HII_density", + FInfo{&grackle_field_data::HII_density, Kind::PRIMORDIAL_SPECIES}); + fn("HeI_density", + FInfo{&grackle_field_data::HeI_density, Kind::PRIMORDIAL_SPECIES}); + fn("HeII_density", + FInfo{&grackle_field_data::HeII_density, Kind::PRIMORDIAL_SPECIES}); + fn("HeIII_density", + FInfo{&grackle_field_data::HeIII_density, Kind::PRIMORDIAL_SPECIES}); + } + + if (check_req(MK_REQ(primordial_chemistry, CmpOp::GEQ, 2))) { + fn("HM_density", + FInfo{&grackle_field_data::HM_density, Kind::PRIMORDIAL_SPECIES}); + fn("H2I_density", + FInfo{&grackle_field_data::H2I_density, Kind::PRIMORDIAL_SPECIES}); + fn("H2II_density", + FInfo{&grackle_field_data::H2II_density, Kind::PRIMORDIAL_SPECIES}); + } + + if (check_req(MK_REQ(primordial_chemistry, CmpOp::GEQ, 3))) { + fn("DI_density", + FInfo{&grackle_field_data::DI_density, Kind::PRIMORDIAL_SPECIES}); + fn("DII_density", + FInfo{&grackle_field_data::DII_density, Kind::PRIMORDIAL_SPECIES}); + fn("HDI_density", + FInfo{&grackle_field_data::HDI_density, Kind::PRIMORDIAL_SPECIES}); + } + + if (check_req(MK_REQ(primordial_chemistry, CmpOp::GEQ, 4))) { + fn("DM_density", + FInfo{&grackle_field_data::DM_density, Kind::PRIMORDIAL_SPECIES}); + fn("HDII_density", + FInfo{&grackle_field_data::HDII_density, Kind::PRIMORDIAL_SPECIES}); + fn("HeHII_density", + FInfo{&grackle_field_data::HeHII_density, Kind::PRIMORDIAL_SPECIES}); + } + + if (check_req(MK_REQ(metal_chemistry, CmpOp::EQ, 1))) { + fn("CI_density", FInfo{&grackle_field_data::CI_density, Kind::UNSET}); + fn("CII_density", FInfo{&grackle_field_data::CII_density, Kind::UNSET}); + fn("CO_density", FInfo{&grackle_field_data::CO_density, Kind::UNSET}); + fn("CO2_density", FInfo{&grackle_field_data::CO2_density, Kind::UNSET}); + fn("OI_density", FInfo{&grackle_field_data::OI_density, Kind::UNSET}); + fn("OH_density", FInfo{&grackle_field_data::OH_density, Kind::UNSET}); + fn("H2O_density", FInfo{&grackle_field_data::H2O_density, Kind::UNSET}); + fn("O2_density", FInfo{&grackle_field_data::O2_density, Kind::UNSET}); + fn("SiI_density", FInfo{&grackle_field_data::SiI_density, Kind::UNSET}); + fn("SiOI_density", FInfo{&grackle_field_data::SiOI_density, Kind::UNSET}); + fn("SiO2I_density", FInfo{&grackle_field_data::SiO2I_density, Kind::UNSET}); + fn("CH_density", FInfo{&grackle_field_data::CH_density, Kind::UNSET}); + fn("CH2_density", FInfo{&grackle_field_data::CH2_density, Kind::UNSET}); + fn("COII_density", FInfo{&grackle_field_data::COII_density, Kind::UNSET}); + fn("OII_density", FInfo{&grackle_field_data::OII_density, Kind::UNSET}); + fn("OHII_density", FInfo{&grackle_field_data::OHII_density, Kind::UNSET}); + fn("H2OII_density", FInfo{&grackle_field_data::H2OII_density, Kind::UNSET}); + fn("H3OII_density", FInfo{&grackle_field_data::H3OII_density, Kind::UNSET}); + fn("O2II_density", FInfo{&grackle_field_data::O2II_density, Kind::UNSET}); + } + + if (check_req(MK_REQ(dust_species, CmpOp::GEQ, 1))) { + fn("Mg_density", FInfo{&grackle_field_data::Mg_density, Kind::UNSET}); + } + + if (check_req(MK_REQ(dust_species, CmpOp::GEQ, 2))) { + fn("Al_density", FInfo{&grackle_field_data::Al_density, Kind::UNSET}); + fn("S_density", FInfo{&grackle_field_data::S_density, Kind::UNSET}); + fn("Fe_density", FInfo{&grackle_field_data::Fe_density, Kind::UNSET}); + } + + if (check_req(MK_REQ(dust_species, CmpOp::GEQ, 1))) { + fn("MgSiO3_dust_density", + FInfo{&grackle_field_data::MgSiO3_dust_density, Kind::UNSET}); + fn("AC_dust_density", + FInfo{&grackle_field_data::AC_dust_density, Kind::UNSET}); + } + + if (check_req(MK_REQ(dust_species, CmpOp::GEQ, 2))) { + fn("SiM_dust_density", + FInfo{&grackle_field_data::SiM_dust_density, Kind::UNSET}); + fn("FeM_dust_density", + FInfo{&grackle_field_data::FeM_dust_density, Kind::UNSET}); + fn("Mg2SiO4_dust_density", + FInfo{&grackle_field_data::Mg2SiO4_dust_density, Kind::UNSET}); + fn("Fe3O4_dust_density", + FInfo{&grackle_field_data::Fe3O4_dust_density, Kind::UNSET}); + fn("SiO2_dust_density", + FInfo{&grackle_field_data::SiO2_dust_density, Kind::UNSET}); + fn("MgO_dust_density", + FInfo{&grackle_field_data::MgO_dust_density, Kind::UNSET}); + fn("FeS_dust_density", + FInfo{&grackle_field_data::FeS_dust_density, Kind::UNSET}); + fn("Al2O3_dust_density", + FInfo{&grackle_field_data::Al2O3_dust_density, Kind::UNSET}); + } + + if (check_req(MK_REQ(dust_species, CmpOp::GEQ, 3))) { + fn("ref_org_dust_density", + FInfo{&grackle_field_data::ref_org_dust_density, Kind::UNSET}); + fn("vol_org_dust_density", + FInfo{&grackle_field_data::vol_org_dust_density, Kind::UNSET}); + fn("H2O_ice_dust_density", + FInfo{&grackle_field_data::H2O_ice_dust_density, Kind::UNSET}); + } + + // we are going to ignore the case with multi_metals == 0 since I think that + // the note in grackle_field_data is wrong in that case. + // - That note suggests that we use one of the following fields, depending on + // the value of metal_abundances. + // - In practice, I think we actually interpret the metal_density field as + // though it was one of the following fields. + // - It's not worth correcting this note and handling this case since PR #480 + // has already been proposed to eliminate the hardcoded path names + if (check_req(MK_REQ(multi_metals, CmpOp::EQ, 1))) { + fn("local_ISM_metal_density", + FInfo{&grackle_field_data::local_ISM_metal_density, Kind::UNSET}); + fn("ccsn13_metal_density", + FInfo{&grackle_field_data::ccsn13_metal_density, Kind::UNSET}); + fn("ccsn20_metal_density", + FInfo{&grackle_field_data::ccsn20_metal_density, Kind::UNSET}); + fn("ccsn25_metal_density", + FInfo{&grackle_field_data::ccsn25_metal_density, Kind::UNSET}); + fn("ccsn30_metal_density", + FInfo{&grackle_field_data::ccsn30_metal_density, Kind::UNSET}); + fn("fsn13_metal_density", + FInfo{&grackle_field_data::fsn13_metal_density, Kind::UNSET}); + fn("fsn15_metal_density", + FInfo{&grackle_field_data::fsn15_metal_density, Kind::UNSET}); + fn("fsn50_metal_density", + FInfo{&grackle_field_data::fsn50_metal_density, Kind::UNSET}); + fn("fsn80_metal_density", + FInfo{&grackle_field_data::fsn80_metal_density, Kind::UNSET}); + fn("pisn170_metal_density", + FInfo{&grackle_field_data::pisn170_metal_density, Kind::UNSET}); + fn("pisn200_metal_density", + FInfo{&grackle_field_data::pisn200_metal_density, Kind::UNSET}); + fn("y19_metal_density", + FInfo{&grackle_field_data::y19_metal_density, Kind::UNSET}); + } + + // volumetric heating rate (provide in units [erg s^-1 cm^-3]) + if (check_req(MK_REQ(use_volumetric_heating_rate, CmpOp::EQ, 1))) { + fn("volumetric_heating_rate", + FInfo{&grackle_field_data::volumetric_heating_rate, + Kind::ELEMENTWISE_SOLVER_ARG}); + } + + // specific heating rate (provide in units [erg s^-1 g^-1] + if (check_req(MK_REQ(use_specific_heating_rate, CmpOp::EQ, 1))) { + fn("specific_heating_rate", + FInfo{&grackle_field_data::specific_heating_rate, + Kind::ELEMENTWISE_SOLVER_ARG}); + } + + if (check_req(MK_REQ(use_temperature_floor, CmpOp::EQ, 1))) { + fn("temperature_floor", FInfo{&grackle_field_data::temperature_floor, + Kind::ELEMENTWISE_SOLVER_ARG}); + } + + if (check_req(MK_REQ(use_radiative_transfer, CmpOp::EQ, 1))) { + if (check_req(MK_REQ(primordial_chemistry, CmpOp::GEQ, 1))) { + // radiative transfer heating rate (provide in units [erg s^-1 cm^-3]) + fn("RT_heating_rate", FInfo{&grackle_field_data::RT_heating_rate, + Kind::ELEMENTWISE_SOLVER_ARG}); + // radiative transfer ionization / dissociation rate fields (provided in + // units of [1/s]) + fn("RT_HI_ionization_rate", + FInfo{&grackle_field_data::RT_HI_ionization_rate, + Kind::ELEMENTWISE_SOLVER_ARG}); + fn("RT_HeI_ionization_rate", + FInfo{&grackle_field_data::RT_HeI_ionization_rate, + Kind::ELEMENTWISE_SOLVER_ARG}); + fn("RT_HeII_ionization_rate", + FInfo{&grackle_field_data::RT_HeII_ionization_rate, + Kind::ELEMENTWISE_SOLVER_ARG}); + } + + if (check_req(MK_REQ(primordial_chemistry, CmpOp::GEQ, 2))) { + fn("RT_H2_dissociation_rate", + FInfo{&grackle_field_data::RT_H2_dissociation_rate, + Kind::ELEMENTWISE_SOLVER_ARG}); + } + + if (check_req(MK_REQ(radiative_transfer_HDI_dissociation, CmpOp::EQ, 1))) { + fn("RT_HDI_dissociation_rate", + FInfo{&grackle_field_data::RT_HDI_dissociation_rate, + Kind::ELEMENTWISE_SOLVER_ARG}); + } + + if (check_req(MK_REQ(radiative_transfer_metal_ionization, CmpOp::EQ, 1))) { + fn("RT_CI_ionization_rate", + FInfo{&grackle_field_data::RT_CI_ionization_rate, + Kind::ELEMENTWISE_SOLVER_ARG}); + fn("RT_OI_ionization_rate", + FInfo{&grackle_field_data::RT_OI_ionization_rate, + Kind::ELEMENTWISE_SOLVER_ARG}); + } + + if (check_req( + MK_REQ(radiative_transfer_metal_dissociation, CmpOp::EQ, 1))) { + fn("RT_CO_dissociation_rate", + FInfo{&grackle_field_data::RT_CO_dissociation_rate, + Kind::ELEMENTWISE_SOLVER_ARG}); + fn("RT_OH_dissociation_rate", + FInfo{&grackle_field_data::RT_OH_dissociation_rate, + Kind::ELEMENTWISE_SOLVER_ARG}); + fn("RT_H2O_dissociation_rate", + FInfo{&grackle_field_data::RT_H2O_dissociation_rate, + Kind::ELEMENTWISE_SOLVER_ARG}); + } + } + + // H2_self_shielding = 2 + if (check_req(MK_REQ(H2_self_shielding, CmpOp::EQ, 2))) { + fn("H2_self_shielding_length", + FInfo{&grackle_field_data::H2_self_shielding_length, + Kind::ELEMENTWISE_SOLVER_ARG}); + } + + if (check_req(MK_REQ(H2_custom_shielding, CmpOp::EQ, 1))) { + fn("H2_custom_shielding_factor", + FInfo{&grackle_field_data::H2_custom_shielding_factor, + Kind::ELEMENTWISE_SOLVER_ARG}); + } + + if (check_req(MK_REQ(use_isrf_field, CmpOp::EQ, 1))) { + fn("isrf_habing", + FInfo{&grackle_field_data::isrf_habing, Kind::ELEMENTWISE_SOLVER_ARG}); + } + +#undef MK_REQ +} + +/// applies the given callback function on all selected named fields +/// +/// There are 2 versions of this function. These function either +/// 1. iterate over each known named field +/// 2. iterate over each named field selected by a @ref GrackleCtxPack instance +/// +/// The callback function should have has a signature that resembles +/// @code{C++} +/// void my_fn(const char* field_name, const FieldInfo& f_info); +/// @endcode +/// As per usual, to maximize performance, you should pass a lambda function or +/// a callable struct rather than an ordinary function +template +constexpr void for_each_named_field(Fn f) { + return for_each_named_field_([](const Req&) -> bool { return true; }, f); +} + +template +void for_each_named_field(const GrackleCtxPack& ctx_pack, Fn f) { + GR_INTERNAL_REQUIRE(ctx_pack.is_initialized(), + "received an uninitialized GrackleCtxPack"); + const chemistry_data* my_chemistry = ctx_pack.my_chemistry(); + + auto check_req = [my_chemistry](const Req& req) -> bool { + return check_req_(*my_chemistry, req); + }; + return for_each_named_field_(check_req, f); +} + +/// query the FieldInfo from a field name +std::optional query_field_info(std::string_view name) noexcept; + +} // namespace grtest::field_detail + +/** @}*/ // end of doxygen group + +#endif // GRTESTUTILS_FIELD_INFO_DETAIL_HPP diff --git a/tests/grtestutils/harness/grackle_ctx_pack.hpp b/tests/grtestutils/harness/grackle_ctx_pack.hpp index 02cbef659..cfd78ae4d 100644 --- a/tests/grtestutils/harness/grackle_ctx_pack.hpp +++ b/tests/grtestutils/harness/grackle_ctx_pack.hpp @@ -113,6 +113,9 @@ 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(); } /// create an initialized instance From 8b3007df94f9bee4bbde6355559c2c7288f4d409 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Thu, 29 Jan 2026 15:50:34 -0500 Subject: [PATCH 18/29] introduce fill_field_vals --- tests/grtestutils/CMakeLists.txt | 1 + tests/grtestutils/harness/fill_field_vals.hpp | 150 ++++++++++++++++++ 2 files changed, 151 insertions(+) create mode 100644 tests/grtestutils/harness/fill_field_vals.hpp diff --git a/tests/grtestutils/CMakeLists.txt b/tests/grtestutils/CMakeLists.txt index b8de75ded..1c4bb8d8c 100644 --- a/tests/grtestutils/CMakeLists.txt +++ b/tests/grtestutils/CMakeLists.txt @@ -87,6 +87,7 @@ add_library(grtestutils harness/grackle_ctx_pack.cpp harness/grackle_ctx_pack.hpp harness/field_container.cpp harness/field_container.hpp harness/field_info_detail.cpp harness/field_info_detail.hpp + harness/fill_field_vals.hpp harness/param.cpp harness/param.hpp harness/preset.cpp harness/preset.hpp harness/status.cpp harness/status.hpp diff --git a/tests/grtestutils/harness/fill_field_vals.hpp b/tests/grtestutils/harness/fill_field_vals.hpp new file mode 100644 index 000000000..b19f42341 --- /dev/null +++ b/tests/grtestutils/harness/fill_field_vals.hpp @@ -0,0 +1,150 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// define fill_field_vals +/// +//===----------------------------------------------------------------------===// +#ifndef GRTESTUTILS_FILL_FIELD_VALS_HPP +#define GRTESTUTILS_FILL_FIELD_VALS_HPP + +#include "GrackleCtxPack.hpp" +#include "grackle.h" +#include "status_reporting.h" +#include "./field_container.hpp" +#include "./field_info_detail.hpp" +#include "./status.hpp" + +#include +#include +#include + +namespace grtest { + +/// Fill in the values of @p fc by repeating the values from @p field_tile +/// +/// @param[in, out] fc The field container that will be filled +/// @param[in] field_tile Specifies the values to use +template +Status fill_field_vals(FieldContainer& fc, const Tile& field_tile) { + // access field grid index properties + int rank = fc.rank(); + int ix_start = fc.get_ptr()->grid_start[0]; + int ix_stop = fc.get_ptr()->grid_end[0] + 1; + int iy_start = (rank >= 2) ? fc.get_ptr()->grid_start[1] : 0; + int iy_stop = (rank >= 2) ? fc.get_ptr()->grid_end[1] + 1 : 1; + int iz_start = (rank == 3) ? fc.get_ptr()->grid_start[2] : 0; + int iz_stop = (rank == 3) ? fc.get_ptr()->grid_end[2] + 1 : 1; + + int mx = fc.get_ptr()->grid_dimension[0]; + int my = (rank >= 2) ? fc.get_ptr()->grid_dimension[1] : 1; + + int n_x_vals = ix_stop - ix_start; + + // access properties about the tile + int tile_len = field_tile.get_n_vals(); + + // a few error consistency checks + if (tile_len <= 0) { + return error::Adhoc("tile_len must be positive"); + } else if (tile_len > n_x_vals) { + // we may want to revisit this in the future + return error::Adhoc("tile_len can't exceed length of contiguous axis"); + } + + // allocate the buffer + std::vector tile_buf(tile_len); + + for (auto [field_name, field_ptr] : fc) { + if (!field_tile.fill_buf(tile_buf.data(), field_name)) { + return error::Adhoc("field_tile doesn't recognize field name: " + + field_name); + } + + for (int iz = iz_start; iz < iz_stop; iz++) { + for (int iy = iy_start; iy < iy_stop; iy++) { + for (int ix = ix_start; ix < ix_stop; ix++) { + int i = ix + mx * (iy + my * iz); + field_ptr[i] = tile_buf[ix % tile_len]; + } + } + } + } + + return OkStatus(); +} + +/// Represents a very simplistic set of conditions +struct SimpleFieldTile { + double mfrac_metal; + double mfrac_H; + double mfrac_He; + double mfrac_D; + + // both of these are in code-units + double common_density; + double common_eint; + + int get_n_vals() const noexcept { return 1; } + + bool fill_buf(gr_float* buf, std::string_view field_name) const noexcept { + if (field_name == "density") { + buf[0] = common_density; + } else if (field_name == "internal_energy") { + buf[0] = common_eint; + } else if (field_name == "metal_density") { + buf[0] = mfrac_metal * common_density; + } else if (field_name == "HI_density") { + buf[0] = mfrac_H * common_density; + } else if (field_name == "HeI_density") { + buf[0] = mfrac_He * common_density; + } else if (field_name == "DI_density") { + buf[0] = mfrac_D * common_density; + } else { + field_detail::Kind kind = field_detail::Kind::UNSET; + std::optional maybe_field_info = + field_detail::query_field_info(field_name); + if (maybe_field_info.has_value()) { + kind = maybe_field_info->kind; + } + + if (kind == field_detail::Kind::ELEMENTWISE_SOLVER_ARG) { + buf[0] = 0.0; + } else if (kind == field_detail::Kind::PRIMORDIAL_SPECIES) { + gr_float tiny_number = 1.e-20; + buf[0] = tiny_number * common_density; + } else { + return false; + } + } + return true; + } +}; + +inline SimpleFieldTile make_simple_tile(const GrackleCtxPack& ctx_pack, + code_units units, double metallicity) { + GR_INTERNAL_REQUIRE(ctx_pack.is_initialized(), "ctx_pack was initialized"); + + const chemistry_data* my_chem = ctx_pack.my_chemistry(); + + // the internal energy roughly corresponds to 1000 K + double common_eint = 1000.0 / get_temperature_units(&units); + + return SimpleFieldTile{ + /* mfrac_metal = */ metallicity * my_chem->SolarMetalFractionByMass, + /* mfrac_H = */ my_chem->HydrogenFractionByMass, + /* mfrac_He = */ (1.0 - my_chem->HydrogenFractionByMass), + /* mfrac_D = */ 2.0 * 3.4e-5, + + // both of these are in code-units + /* common_density = */ 1.0, + /* common_eint = */ common_eint}; +} + +} // namespace grtest + +#endif // GRTESTUTILS_SET_FIELD_CONDITIONS_HPP From 2979e8a460d3973b0f72b4a0270b44ce3c19be76 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 30 Jan 2026 16:26:24 -0500 Subject: [PATCH 19/29] refactor GridLayout --- tests/grtestutils/harness/field_container.cpp | 112 +++++----- tests/grtestutils/harness/field_container.hpp | 205 ++++++++++++++++-- 2 files changed, 241 insertions(+), 76 deletions(-) diff --git a/tests/grtestutils/harness/field_container.cpp b/tests/grtestutils/harness/field_container.cpp index d97f22073..27435a2e5 100644 --- a/tests/grtestutils/harness/field_container.cpp +++ b/tests/grtestutils/harness/field_container.cpp @@ -19,9 +19,43 @@ #include "status_reporting.h" #include // std::memcmp, std::memcpy +#include +#include #include // std::pair, std::move namespace grtest { + +bool operator==(const GridLayout& lhs, const GridLayout& rhs) noexcept { + bool rank = lhs.rank_ == rhs.rank_; + bool start = std::memcmp(lhs.start_, rhs.start_, 3 * sizeof(int)) == 0; + bool stop = std::memcmp(lhs.stop_, rhs.stop_, 3 * sizeof(int)) == 0; + bool dim = std::memcmp(lhs.dim_, rhs.dim_, 3 * sizeof(int)) == 0; + return rank && start && stop && dim; +} + +void PrintTo(const GridLayout& layout, std::ostream* os) { + auto write_arr_ = [&](const int* vals) -> void { + for (int i = 0; i < layout.rank_; i++) { + *os << ((i == 0) ? "{" : ", "); + *os << vals[i]; + } + *os << '}'; + }; + *os << "GridLayout{ dim: "; + write_arr_(layout.dim_); + *os << ", start: "; + write_arr_(layout.start_); + *os << ", stop: "; + write_arr_(layout.stop_); + *os << '}'; +} + +std::string GridLayout::to_string() const { + std::stringstream s; + PrintTo(*this, &s); + return s.str(); +} + namespace field_detail { /// Construct a name-pointer mapping based on @p ctx_pack @@ -48,23 +82,19 @@ static MapType make_nullptr_map_(const GrackleCtxPack& ctx_pack, return m; } -/// Construct a `CorePack` instance by consuming @p premade_map -/// -/// @param premade_map A string to pointer mapping. The keys of this argument -/// should specify the names for each desired Grackle field. We assume that -/// the pointers associated with each key hold meaningless garbage values -/// @param buf_size The number of elements to allocate per field -static std::pair setup_1d_CorePack(MapType&& premade_map, - int buf_size) { +std::pair CorePack::setup_1d(MapType&& premade_map, + int buf_size) { if (buf_size <= 0) { return {CorePack{}, error::Adhoc("buf_size must be positive")}; } std::size_t data_sz = premade_map.size() * buf_size; + GridLayout layout = GridLayout::from_dim(1, &buf_size); + CorePack pack{ // the trailing parentheses when allocating an array of integers or // floating point values sets the initial values to 0 - /* prop_buf */ std::unique_ptr(new int[9]()), + /* layout */ std::unique_ptr(new GridLayout(layout)), /* data_buf */ std::unique_ptr(new gr_float[data_sz]()), /* my_fields */ std::unique_ptr(new grackle_field_data), @@ -75,9 +105,9 @@ static std::pair setup_1d_CorePack(MapType&& premade_map, } // setup grid properties - pack.my_fields->grid_dimension = pack.prop_buf.get(); - pack.my_fields->grid_start = pack.prop_buf.get() + 3; - pack.my_fields->grid_end = pack.prop_buf.get() + 6; + pack.my_fields->grid_dimension = pack.layout->dim_; + pack.my_fields->grid_start = pack.layout->start_; + pack.my_fields->grid_end = pack.layout->end_; pack.my_fields->grid_rank = 1; pack.my_fields->grid_dimension[0] = buf_size; @@ -120,7 +150,7 @@ std::pair FieldContainer::create_1d( field_detail::MapType m = field_detail::make_nullptr_map_(ctx_pack, disable_metal); std::pair tmp = - field_detail::setup_1d_CorePack(std::move(m), buf_size); + field_detail::CorePack::setup_1d(std::move(m), buf_size); if (tmp.second.is_err()) { return {FieldContainer(), std::move(tmp.second)}; } else { @@ -133,38 +163,13 @@ std::pair FieldContainer::create_1d( std::pair FieldContainer::create( const GrackleCtxPack& ctx_pack, const GridLayout& layout, bool disable_metal) { - if (layout.rank < 1 || layout.rank > 3) { - return {FieldContainer(), error::Adhoc("layout.rank must be 1, 2, or 3")}; - } - int total_count = 1; - for (int i = 0; i < layout.rank; i++) { - if (layout.dimension[i] <= 0) { - return {FieldContainer(), - error::Adhoc("layout.dimension[i] must be positive")}; - } else if (layout.ghost_depth[i] < 0) { - return {FieldContainer(), - error::Adhoc("layout.ghost_depth[i] is negative")}; - } else if (layout.ghost_depth[i] * 2 >= layout.dimension[i]) { - return {FieldContainer(), - error::Adhoc( - "layout.dimension[i] must exceed 2*layout.ghost_depth[i]")}; - } - total_count *= layout.dimension[i]; - } + int total_count = layout.n_elements(); std::pair tmp = FieldContainer::create_1d(ctx_pack, total_count, disable_metal); if (tmp.second.is_ok()) { - grackle_field_data* my_fields = tmp.first.data_.my_fields.get(); - tmp.first.data_.my_fields->grid_rank = layout.rank; - for (int i = 0; i < layout.rank; i++) { - int dim = layout.dimension[i]; - int ghost_depth = layout.ghost_depth[i]; - my_fields->grid_dimension[i] = dim; - my_fields->grid_start[i] = ghost_depth; - my_fields->grid_end[i] = dim - ghost_depth - 1; - } + (*tmp.first.data_.layout) = layout; } return tmp; } @@ -175,20 +180,16 @@ FieldContainer FieldContainer::clone() const { // -> right after we create field_detail::MapType m = this->data_.map; std::pair tmp = - field_detail::setup_1d_CorePack(std::move(m), this->elements_per_field()); + field_detail::CorePack::setup_1d(std::move(m), + this->elements_per_field()); // it shouldn't be possible for tmp.second.is_err() to return true FieldContainer out; out.data_ = std::move(tmp.first); // copy over layout properties - int rank = this->rank(); - for (int i = 0; i < rank; i++) { - out.data_.my_fields->grid_start[i] = this->data_.my_fields->grid_start[i]; - out.data_.my_fields->grid_end[i] = this->data_.my_fields->grid_end[i]; - out.data_.my_fields->grid_dimension[i] = - this->data_.my_fields->grid_dimension[i]; - } + (*out.data_.layout) = *this->data_.layout; + // now copy over field values copy_into_helper_(out); return out; } @@ -207,21 +208,6 @@ void FieldContainer::copy_into_helper_(FieldContainer& other) const { std::memcpy(dst, src, length * sizeof(gr_float)); } -bool FieldContainer::same_grid_props(const FieldContainer& other) const { - const grackle_field_data* a = this->data_.my_fields.get(); - const grackle_field_data* b = other.data_.my_fields.get(); - - if (a->grid_rank != b->grid_rank) { - return false; - } - int rank = a->grid_rank; - std::size_t sz = rank * sizeof(int); - bool s = std::memcmp(a->grid_start, b->grid_start, sz) == 0; - bool e = std::memcmp(a->grid_end, b->grid_end, sz) == 0; - bool d = std::memcmp(a->grid_dimension, b->grid_dimension, sz) == 0; - return s && d && e; -} - bool FieldContainer::same_fields(const FieldContainer& other) const { // this takes advantage of the fact that MapType is sorted MapType::const_iterator it_a = this->data_.map.begin(); diff --git a/tests/grtestutils/harness/field_container.hpp b/tests/grtestutils/harness/field_container.hpp index 4a6555833..4bc3a33ff 100644 --- a/tests/grtestutils/harness/field_container.hpp +++ b/tests/grtestutils/harness/field_container.hpp @@ -16,8 +16,10 @@ #include "./grackle_ctx_pack.hpp" #include "./status.hpp" +#include "status_reporting.h" #include // std::less +#include #include #include // std::map #include @@ -27,6 +29,181 @@ namespace grtest { +namespace field_detail { +struct CorePack; +} // namespace field_detail + +/// Represents Grid Properties +/// +/// This type is probably a little over-engineered +/// +/// Broader Context +/// =============== +/// To best describe this type, it's insightful to draw comparisons with C++ +/// conventions. +/// +/// Background +/// ---------- +/// For some background, C++23 introduced `std::mdspan` to describe +/// multi-dimensional views. A `std::mdspan` is parameterized by +/// - the data's extents (aka the shape) +/// - the data's layout, which dictates how a multidimensional index is mapped +/// to a 1D pointer offset +/// +/// For views of contiguous data there are 2 obvious layouts: +/// 1. layout-right: where the stride is `1` along the rightmost extent. +/// - for extents `{a,b,c}`, an optimal nested for-loop will iterates from +// `0` up to `a` in the outermost loop and from `0` up to `c` +/// in the innermost loop +/// - this is the "natural layout" for a multidimensional c-style array +/// `arr[a][b][c]` +/// 2. layout-left: where the stride is `1` along the leftmost extent +/// - for extents `{a,b,c}`, an optimal nested for-loop will iterates from +// `0` up to `c` in the outermost loop and from `0` up to `a` +/// in the innermost loop +/// - this is the "natural layout" for a multidimensional fortran array () +/// `arr[a][b][c]` +/// +/// > Aside: More sophisticated layouts are possible when strides along each +/// > axis aren't directly tied to extents (this comes up when making subviews). +/// +/// About this type +/// --------------- +/// This type specifies array extents for a layout-left mapping. It also +/// specifies the region of valid values. For context, arrays of +/// fluid-quantities in mesh-based hydro codes have an outer layer "ghost zones" +/// (Computer Scientists sometimes call this a "halo") that may not contain +/// valid values when calling Grackle. +/// +/// > Aside: We only describe the concept of "ghost zones" because we want to +/// > to support tests for validating that ghost zones aren't modified. +/// > Otherwise, we could reframe this description in terms of strided layouts. +class GridLayout { + friend field_detail::CorePack; + + /// the number of dimensions + int rank_; + /// number of elements along an axis + int dim_[3]; + + /// the first active-zone index along an axis + int start_[3]; + + /// the last active-zone index along an axis + /// + /// @note + /// We track this because Grackle natively understands this + int end_[3]; + + /// elements are one larger than the corresponding value in @ref end + /// + /// @note + /// This is the more natural than @ref end for 0-based indexing + int stop_[3]; + + // only invoked by factory methods + GridLayout() = default; + + static std::pair create_(int rank, const int* dim, + const int* start, + const int* stop) { + GridLayout out; + out.rank_ = rank; + if ((rank < 1) || (rank > 3)) { + return {out, error::Adhoc("rank is invalid")}; + } else if (dim == nullptr) { + return {out, error::Adhoc("dim is a nullptr")}; + } + + for (int i = 0; i < 3; i++) { + out.dim_[i] = (i < rank) ? dim[i] : 1; + out.start_[i] = (start != nullptr && i < rank) ? start[i] : 0; + out.stop_[i] = (stop != nullptr && i < rank) ? stop[i] : out.dim_[i]; + out.end_[i] = out.stop_[i] - 1; + + if (out.dim_[i] < 1) { + return {out, error::Adhoc("dim must hold positive vals")}; + } else if (out.start_[i] < 0) { + return {out, error::Adhoc("start must hold non-negative vals")}; + } else if (out.stop_[i] <= out.start_[i]) { + return {out, error::Adhoc("stop must exceed start")}; + } else if (out.stop_[i] > out.dim_[i]) { + return {out, error::Adhoc("stop must not exceed dim")}; + } + } + + return {out, OkStatus()}; + } + +public: + /// factory method + static std::pair try_from_dim(int rank, const int* dim) { + return GridLayout::create_(rank, dim, nullptr, nullptr); + } + + /// factory method (aborts for invalid arguments) + static GridLayout from_dim(int rank, const int* dim) { + std::pair tmp = GridLayout::try_from_dim(rank, dim); + if (tmp.second.is_err()) { + std::string msg = tmp.second.to_string(); + GR_INTERNAL_ERROR("%s", msg.c_str()); + } + return tmp.first; + } + + /// factory method (aborts for invalid arguments) + static GridLayout from_ghostdepth_and_dims(int rank, const int* ghostdepth, + const int* dim) { + GR_INTERNAL_REQUIRE(rank == 1 || rank == 2 || rank == 3, "rank is invalid"); + GR_INTERNAL_REQUIRE(ghostdepth != nullptr, "ghostdepth is a nullptr"); + GR_INTERNAL_REQUIRE(dim != nullptr, "dim is a nullptr"); + + int start[3] = {0, 0, 0}; + int stop[3] = {1, 1, 1}; + for (int i = 0; i < rank; i++) { + GR_INTERNAL_REQUIRE(ghostdepth[i] >= 0, "ghostdepth must be nonnegative"); + GR_INTERNAL_REQUIRE(2 * ghostdepth[i] < dim[i], + "dim[i] must exceed 2*ghostdepth[i]") + start[i] = ghostdepth[i]; + stop[i] = dim[i] - ghostdepth[i]; + } + + std::pair tmp = + GridLayout::create_(rank, dim, start, stop); + if (tmp.second.is_err()) { + std::string msg = tmp.second.to_string(); + GR_INTERNAL_ERROR("%s", msg.c_str()); + } + return tmp.first; + } + + int rank() const noexcept { return rank_; } + const int* dim() const noexcept { return dim_; } + const int* start() const noexcept { return start_; } + const int* stop() const noexcept { return stop_; } + const int* end() const noexcept { return end_; } + + friend bool operator==(const GridLayout& lhs, const GridLayout& rhs) noexcept; + friend bool operator!=(const GridLayout& lhs, + const GridLayout& rhs) noexcept { + return !(lhs == rhs); + } + + int n_elements(bool exclude_inactive = false) const noexcept { + int total = 1; + for (int i = 0; i < rank_; i++) { + total *= (exclude_inactive) ? stop_[i] - start_[i] : dim_[i]; + } + return total; + } + + // teach googletest how to print this type + friend void PrintTo(const GridLayout& layout, std::ostream* os); + + /// create a string representation + std::string to_string() const; +}; + namespace field_detail { inline int elements_per_field_(const grackle_field_data& my_fields) { @@ -49,7 +226,7 @@ using MapType = std::map>; /// /// Consider a case where @ref my_fields is fully initialized: /// - each pointer data member for specifying grid properties refers to a -/// non-overlapping segment of the buffer tracked by @ref prop_buf +/// memory tracked by @ref grid_layout /// - each pointer data member for specifying an unused grackle-field is holds /// a nullptr /// - each pointer data member for specifying an actively used grackle-field: @@ -60,24 +237,26 @@ using MapType = std::map>; /// is the name of the grackle-field, and the value is associated pointer struct CorePack { /// the buffer used for holding field shape and ghost zones - std::unique_ptr prop_buf; + std::unique_ptr layout; /// the buffer used to hold all field data std::unique_ptr data_buf; /// the struct understood by Grackle std::unique_ptr my_fields; /// maps field names to pointers MapType map; + + /// factory method that consumes a @p premade_map + /// + /// @param premade_map A string to pointer mapping. The keys of this argument + /// should specify the names for each desired Grackle field. We assume + /// that pointers associated with each key hold meaningless garbage values + /// @param buf_size The number of elements to allocate per field + static std::pair setup_1d(MapType&& premade_map, + int buf_size); }; } // namespace field_detail -/// Encapsulates an arbitrary GridLayout -struct GridLayout { - int rank; - int dimension[3]; - int ghost_depth[3]; -}; - /// A container wrapping grackle_field_data that owns the underlying data /// /// @par Implementation Notes @@ -135,7 +314,9 @@ class FieldContainer { } /// returns whether `this` and @p other contains the same grid properties - bool same_grid_props(const FieldContainer& other) const; + bool same_grid_props(const FieldContainer& other) const { + return (*this->data_.layout) == (*other.data_.layout); + } /// returns whether `this` and @p other contains the same set of fields bool same_fields(const FieldContainer& other) const; @@ -158,9 +339,7 @@ class FieldContainer { int rank() const { return data_.my_fields->grid_rank; } /// the number of elements per field (unaffected by ghost zones) - int elements_per_field() const { - return field_detail::elements_per_field_(*data_.my_fields); - } + int elements_per_field() const { return data_.layout->n_elements(); } // we define both of the following methods to support the writing of // range-based for-loops to iterate over key-buffer pairs From 03bfe8ce32299bacb97e390996e6abaa7f902326 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sun, 1 Feb 2026 23:28:52 -0500 Subject: [PATCH 20/29] improve FieldContainer and lightly refactor fill_field_vals --- tests/grtestutils/harness/field_container.cpp | 40 ++++++--- tests/grtestutils/harness/field_container.hpp | 84 ++++++++++++++++-- tests/grtestutils/harness/fill_field_vals.hpp | 87 ++++++++++++++++--- 3 files changed, 183 insertions(+), 28 deletions(-) diff --git a/tests/grtestutils/harness/field_container.cpp b/tests/grtestutils/harness/field_container.cpp index 27435a2e5..60c851741 100644 --- a/tests/grtestutils/harness/field_container.cpp +++ b/tests/grtestutils/harness/field_container.cpp @@ -14,6 +14,7 @@ #include "./field_container.hpp" #include "./field_info_detail.hpp" #include "./status.hpp" +#include "../utils.hpp" #include "grackle.h" #include "status_reporting.h" @@ -63,7 +64,7 @@ namespace field_detail { /// The returned where the keys are the names of every active Grackle field /// and the associated values are all nullptr static MapType make_nullptr_map_(const GrackleCtxPack& ctx_pack, - bool exclude_metal) { + const std::set& exclude_fields) { MapType m; // fill up m with (field, nullptr) pairs for each field enabled by ctx_pack auto fn = [&m](const char* name, const FieldInfo& info) { @@ -73,8 +74,8 @@ static MapType make_nullptr_map_(const GrackleCtxPack& ctx_pack, }; for_each_named_field(ctx_pack, fn); - if (exclude_metal) { - auto search = m.find("metal_density"); + for (const std::string& name : exclude_fields) { + auto search = m.find(name); if (search != m.end()) { m.erase(search); } @@ -141,14 +142,15 @@ std::pair CorePack::setup_1d(MapType&& premade_map, } // namespace field_detail std::pair FieldContainer::create_1d( - const GrackleCtxPack& ctx_pack, int buf_size, bool disable_metal) { + const GrackleCtxPack& ctx_pack, int buf_size, + const std::set& exclude_fields) { if (!ctx_pack.is_initialized()) { return {FieldContainer{}, error::Adhoc("ctx_pack isn't initialized")}; } // construct a map for each relevant field (the values are all nullptr) field_detail::MapType m = - field_detail::make_nullptr_map_(ctx_pack, disable_metal); + field_detail::make_nullptr_map_(ctx_pack, exclude_fields); std::pair tmp = field_detail::CorePack::setup_1d(std::move(m), buf_size); if (tmp.second.is_err()) { @@ -162,14 +164,14 @@ std::pair FieldContainer::create_1d( std::pair FieldContainer::create( const GrackleCtxPack& ctx_pack, const GridLayout& layout, - bool disable_metal) { + const std::set& exclude_fields) { int total_count = layout.n_elements(); std::pair tmp = - FieldContainer::create_1d(ctx_pack, total_count, disable_metal); + FieldContainer::create_1d(ctx_pack, total_count, exclude_fields); if (tmp.second.is_ok()) { - (*tmp.first.data_.layout) = layout; + tmp.first.data_.override_layout(layout); } return tmp; } @@ -181,13 +183,13 @@ FieldContainer FieldContainer::clone() const { field_detail::MapType m = this->data_.map; std::pair tmp = field_detail::CorePack::setup_1d(std::move(m), - this->elements_per_field()); + this->grid_layout().n_elements()); // it shouldn't be possible for tmp.second.is_err() to return true FieldContainer out; out.data_ = std::move(tmp.first); // copy over layout properties - (*out.data_.layout) = *this->data_.layout; + out.data_.override_layout(*this->data_.layout); // now copy over field values copy_into_helper_(out); @@ -204,7 +206,7 @@ void FieldContainer::copy_into_helper_(FieldContainer& other) const { const gr_float* src = this->data_.data_buf.get(); gr_float* dst = other.data_.data_buf.get(); - int length = this->elements_per_field() * this->n_fields(); + int length = this->grid_layout().n_elements() * this->n_fields(); std::memcpy(dst, src, length * sizeof(gr_float)); } @@ -222,4 +224,20 @@ bool FieldContainer::same_fields(const FieldContainer& other) const { return true; } +void PrintTo(const FieldContainer& fc, std::ostream* os) { + const GridLayout& layout = fc.grid_layout(); + *os << "FieldContainer{\n" + << " " << layout.to_string() << ",\n" + << " grid_dx = " << fc.data_.my_fields->grid_dx << ",\n" + << " fields = {\n"; + std::size_t n_elements = layout.n_elements(); + for (const auto& it : fc) { + const std::string& field_name = it.first; + const gr_float* ptr = it.second; + *os << " " << field_name << " =\n"; + *os << " " << ptr_to_string(ptr, n_elements) << '\n'; + } + *os << " }\n}\n"; +} + } // namespace grtest \ No newline at end of file diff --git a/tests/grtestutils/harness/field_container.hpp b/tests/grtestutils/harness/field_container.hpp index 4bc3a33ff..fb20bf108 100644 --- a/tests/grtestutils/harness/field_container.hpp +++ b/tests/grtestutils/harness/field_container.hpp @@ -23,6 +23,7 @@ #include #include // std::map #include +#include #include #include #include // std::pair @@ -136,6 +137,17 @@ class GridLayout { } public: + /// factory method + /// + /// This is for the common case where we want a 1D layout with a hardcoded + /// number of entries (and we know at compile-time that it can't fail) + template + static GridLayout create_1d() noexcept { + static_assert(N >= 1, "N must be positive"); + int dim[3] = {N, 0, 0}; + return GridLayout::create_(1, dim, nullptr, nullptr).first; + } + /// factory method static std::pair try_from_dim(int rank, const int* dim) { return GridLayout::create_(rank, dim, nullptr, nullptr); @@ -245,6 +257,19 @@ struct CorePack { /// maps field names to pointers MapType map; + /// prefer this method over directly modifying layout + /// + /// @note This obviously requires that this->layout and this->my_fields are + /// not nullptr. + void override_layout(const GridLayout& new_layout) noexcept { + // overriding this->layout implicitly updates the members of `my_fields`, + // `my_fields->grid_(dimension|start|end)`, because these members all point + // to statically sized C array members of GridLayout + *this->layout = new_layout; + // make sure grid_rank remains up to date + this->my_fields->grid_rank = this->layout->rank(); + } + /// factory method that consumes a @p premade_map /// /// @param premade_map A string to pointer mapping. The keys of this argument @@ -285,13 +310,24 @@ class FieldContainer { ~FieldContainer() = default; /// A factory method to make a simple 1d container + /// + /// @param ctx_pack The Grackle Configuration used for initialization + /// @param buf_size The positive number of elements in the container + /// @param exclude_fields Names of fields that should be excluded + /// + /// @note This is provided as a convenience. Do we really need it? static std::pair create_1d( - const GrackleCtxPack& ctx_pack, int buf_size, bool disable_metal = false); + const GrackleCtxPack& ctx_pack, int buf_size, + const std::set& exclude_fields = {}); /// A factory method to make a container + /// + /// @param ctx_pack The Grackle Configuration used for initialization + /// @param layout The Grid layout to use + /// @param exclude_fields Names of fields that should be excluded static std::pair create( const GrackleCtxPack& ctx_pack, const GridLayout& layout, - bool disable_metal = false); + const std::set& exclude_fields = {}); /// Create a clone of FieldContainer FieldContainer clone() const; @@ -302,7 +338,8 @@ class FieldContainer { /// /// @warning /// Setting bypass_check to `true` is risky. It primarily exists for the - /// case where you call this method in a loop + /// case where you call this method in a loop and we already know that 2 + /// containers are compatible Status copy_into(FieldContainer& dest, bool bypass_check = false) const { if (!(bypass_check || this->same_grid_props(dest))) { return error::Adhoc("grid properties are incompatible"); @@ -321,9 +358,20 @@ class FieldContainer { /// returns whether `this` and @p other contains the same set of fields bool same_fields(const FieldContainer& other) const; + /**@{*/ + /// get the pointer to the wrapped @ref grackle_field_data instance + /// + /// @note + /// This primarily exists to support Grackle API function. Avoid mutating + /// the returned pointer const grackle_field_data* get_ptr() const { return data_.my_fields.get(); } + + // NOLINTNEXTLINE(readability-make-member-function-const) grackle_field_data* get_ptr() { return data_.my_fields.get(); } + /**@}*/ + /**@{*/ + /// finds the field data pointer for the field with the specified name std::optional find(std::string_view key) { auto s = data_.map.find(key); return (s != data_.map.end()) ? std::optional{s->second} : std::nullopt; @@ -333,18 +381,40 @@ class FieldContainer { auto s = data_.map.find(key); return (s != data_.map.end()) ? std::optional{s->second} : std::nullopt; } + /**@}*/ + + /**@{*/ + /// finds the field data pointer or aborts the program + gr_float* get_or_abort(std::string_view key) { + std::optional tmp = find(key); + if (!tmp.has_value()) { + std::string key_copy{key}; // required b/c key isn't '\0' terminated + GR_INTERNAL_ERROR("\"%s\" field wasn't found", key_copy.c_str()); + } + return *tmp; + } - int n_fields() const { return static_cast(data_.map.size()); } + const gr_float* get_or_abort(std::string_view key) const { + std::optional tmp = find(key); + if (!tmp.has_value()) { + std::string key_copy{key}; // required b/c key isn't '\0' terminated + GR_INTERNAL_ERROR("\"%s\" field wasn't found", key_copy.c_str()); + } + return *tmp; + } + /**@}*/ - int rank() const { return data_.my_fields->grid_rank; } + const GridLayout& grid_layout() const { return *data_.layout; } - /// the number of elements per field (unaffected by ghost zones) - int elements_per_field() const { return data_.layout->n_elements(); } + int n_fields() const { return static_cast(data_.map.size()); } // we define both of the following methods to support the writing of // range-based for-loops to iterate over key-buffer pairs MapType::const_iterator begin() const { return data_.map.begin(); } MapType::const_iterator end() const { return data_.map.end(); } + + // teach googletest how to print this type + friend void PrintTo(const FieldContainer& fc, std::ostream* os); }; } // namespace grtest diff --git a/tests/grtestutils/harness/fill_field_vals.hpp b/tests/grtestutils/harness/fill_field_vals.hpp index b19f42341..67ea48143 100644 --- a/tests/grtestutils/harness/fill_field_vals.hpp +++ b/tests/grtestutils/harness/fill_field_vals.hpp @@ -12,7 +12,7 @@ #ifndef GRTESTUTILS_FILL_FIELD_VALS_HPP #define GRTESTUTILS_FILL_FIELD_VALS_HPP -#include "GrackleCtxPack.hpp" +#include "./grackle_ctx_pack.hpp" #include "grackle.h" #include "status_reporting.h" #include "./field_container.hpp" @@ -25,20 +25,55 @@ namespace grtest { +/// @defgroup fillfieldsgrp Logic for filling field values +/// +/// This group of entities defines the logic for initializing values in a +/// @ref FluidContainer. +/// +/// Since our testing tools may be used to test (or benchmark) scenarios with +/// an arbitrarily large number of elements, we adopt a general-purpose scheme +/// where we essentially repeat a "tile" of field values 1 or more times. +/// +/// In general, a "tile" refers to an instance of a generic type that implements +/// the methods illustrated in the following snippet. +/// +/// @code{C++} +/// struct MySampleTileType { +/// /// number of elements in the tile +/// int get_n_vals() const noexcept; +/// +/// // iF field_name is known, fill buf with associated values associated +/// // and return true. Otherwise, return false. +/// // +/// // buf should have a length given by get_n_vals() +/// bool fill_buf(gr_float* buf, std::string_view field_name) const noexcept; +/// }; +/// @endcode +/// +/// This approach was picked to maximize flexibility. +/// - it can work for initializing a single value or an arbitrary number of +/// values. This reduces the maintenance burden (at the cost of some +/// performance) +/// - it should be easy enough to implement a new tile type +/// - it is conceivable that we could reuse this machinery for initializing +/// a field from serialized values (maybe from a json or toml or hdf5 file) +/**@{*/ // open the doxygen group + /// Fill in the values of @p fc by repeating the values from @p field_tile /// -/// @param[in, out] fc The field container that will be filled /// @param[in] field_tile Specifies the values to use +/// @param[in, out] fc The field container that will be filled template -Status fill_field_vals(FieldContainer& fc, const Tile& field_tile) { +Status fill_field_vals(const Tile& field_tile, FieldContainer& fc) { // access field grid index properties - int rank = fc.rank(); - int ix_start = fc.get_ptr()->grid_start[0]; - int ix_stop = fc.get_ptr()->grid_end[0] + 1; - int iy_start = (rank >= 2) ? fc.get_ptr()->grid_start[1] : 0; - int iy_stop = (rank >= 2) ? fc.get_ptr()->grid_end[1] + 1 : 1; - int iz_start = (rank == 3) ? fc.get_ptr()->grid_start[2] : 0; - int iz_stop = (rank == 3) ? fc.get_ptr()->grid_end[2] + 1 : 1; + const GridLayout& layout = fc.grid_layout(); + int rank = layout.rank(); + int ix_start = layout.start()[0]; + int ix_stop = layout.stop()[0]; + int iy_start = (rank >= 2) ? layout.start()[1] : 0; + int iy_stop = (rank >= 2) ? layout.stop()[1] : 1; + int iz_start = (rank == 3) ? layout.start()[2] : 0; + int iz_stop = (rank == 3) ? layout.stop()[2] : 1; int mx = fc.get_ptr()->grid_dimension[0]; int my = (rank >= 2) ? fc.get_ptr()->grid_dimension[1] : 1; @@ -78,6 +113,30 @@ Status fill_field_vals(FieldContainer& fc, const Tile& field_tile) { return OkStatus(); } +/// Create a @ref FieldContainer and fill in values by repeating the values from +/// @p field_tile +/// +/// @param field_tile Specifies the values to use +/// @param ctx_pack The Grackle Configuration used for initialization +/// @param layout The Grid layout to use +/// @param exclude_fields Field names that should be excluded during creation +/// +/// This function is simple: it just calls @ref FieldContainer::create and +/// @ref fill_field_vals. But because this sequence of events is relatively +/// common, the aggregation of @ref Status instance is quite convenient. +template +std::pair create_and_fill_FieldContainer( + const Tile& field_tile, const GrackleCtxPack& ctx_pack, + const GridLayout& layout, + const std::set& exclude_fields = {}) { + std::pair out = + grtest::FieldContainer::create(ctx_pack, layout, exclude_fields); + if (out.second.is_ok()) { + out.second = fill_field_vals(field_tile, out.first); + } + return out; +} + /// Represents a very simplistic set of conditions struct SimpleFieldTile { double mfrac_metal; @@ -89,8 +148,14 @@ struct SimpleFieldTile { double common_density; double common_eint; + /// number of values specified by the tile int get_n_vals() const noexcept { return 1; } + /// fill @p buf with values associated with the specified @p field_name + /// + /// @param[out] buf This is filled. The length is given by @ref get_n_vals() + /// @param[in] field_name Name of the field + /// @returns `true` if the field is known. Otherwise, returns `false` bool fill_buf(gr_float* buf, std::string_view field_name) const noexcept { if (field_name == "density") { buf[0] = common_density; @@ -145,6 +210,8 @@ inline SimpleFieldTile make_simple_tile(const GrackleCtxPack& ctx_pack, /* common_eint = */ common_eint}; } +/**@}*/ // close the doxygen group + } // namespace grtest #endif // GRTESTUTILS_SET_FIELD_CONDITIONS_HPP From 5d5ec6d5ffc728771da5d3346884f7c1cb18b5f8 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Fri, 20 Feb 2026 10:26:18 -0500 Subject: [PATCH 21/29] introduce grtest::IdxMapping --- tests/grtestutils/view.hpp | 278 +++++++++++++++++++++++++++++++++++++ 1 file changed, 278 insertions(+) diff --git a/tests/grtestutils/view.hpp b/tests/grtestutils/view.hpp index 88d03ec12..102cee1fc 100644 --- a/tests/grtestutils/view.hpp +++ b/tests/grtestutils/view.hpp @@ -16,11 +16,275 @@ #ifndef GRTESTUTILS_VIEW_HPP #define GRTESTUTILS_VIEW_HPP +#include #include // std::size_t +#include +#include #include +#include // std::pair + +#include "status_reporting.h" namespace grtest { +/// To be used with @ref IdxMapping +enum struct DataLayout { + LEFT, ///< the leftmost dimension has a stride 1 + RIGHT ///< the rightmost dimension has a stride 1 +}; + +/// Maps multi-dimensional indices to a 1D pointer offset +/// +/// Broader Context +/// =============== +/// To best describe this type, it's insightful to draw comparisons with C++ +/// conventions. +/// +/// Background +/// ---------- +/// For some background, C++23 introduced `std::mdspan` to describe +/// multi-dimensional views. A `std::mdspan` is parameterized by +/// - the data's extents (aka the shape) +/// - the data's layout, which dictates how a multidimensional index is mapped +/// to a 1D pointer offset +/// +/// For views of contiguous data there are 2 obvious layouts: +/// 1. layout-right: where the stride is `1` along the rightmost extent. +/// - for extents `{a,b,c}`, an optimal nested for-loop will iterates from +/// `0` up to `a` in the outermost loop and from `0` up to `c` +/// in the innermost loop +/// - this is the "natural layout" for a multidimensional c-style array +/// `arr[a][b][c]` +/// 2. layout-left: where the stride is `1` along the leftmost extent +/// - for extents `{a,b,c}`, an optimal nested for-loop will iterates from +/// `0` up to `c` in the outermost loop and from `0` up to `a` +/// in the innermost loop +/// - this is the "natural layout" for a multidimensional fortran array +/// `arr[a][b][c]` +/// +/// > Aside: More sophisticated layouts are possible when strides along each +/// > axis aren't directly tied to extents (this comes up when making subviews). +/// +/// About this type +/// --------------- +/// This type specifies the data layout, extents, and provides the mapping. You +/// draw analogies with C++23 types: +/// - `IdxMapping` <---> `std::layout_left::mapping` +/// - `IdxMapping` <---> `std::layout_right::mapping` +/// +/// At the time of writing, the type **only** represents contiguous data layouts +/// +/// @note +/// The template specialization using @ref DataLayout::RIGHT mostly exists for +/// exposition purposes +/// - it's useful to talk about this scenario since it is the "natural" layout +/// for C and C++ +/// - in practice, Grackle was written assuming @ref DataLayout::LEFT. Thus, +/// most loops will initially get written assuming that layout (but we always +/// have the option to conditionally provide the better kind of loop) +template +struct IdxMapping { + static_assert(Layout == DataLayout::LEFT || Layout == DataLayout::RIGHT, + "A new layout type was introduced that we don't yet support"); + static constexpr DataLayout layout = Layout; + + /// max rank value + static constexpr int MAX_RANK = 3; + +private: + // attributes: + int rank_; ///< the number of dimensions + int extents_[MAX_RANK]; ///< dimensions of the multi-dimensional index-space + + // private functions: + + // default constructor is only invoked by factory methods + // -> this explicitly set each extent_ to a value of 0 + IdxMapping() : rank_(0), extents_{} {} + + // the object returned by the create_ factory method: + // - `out.second == nullptr` indicates the method succeeded + // - otherwise, `out.second` is a string-literal specifying an error message + using InnerMappingMsgPair_ = std::pair, const char*>; + + static InnerMappingMsgPair_ create_(int rank, const int* extents) noexcept { + // arg checking + if ((rank < 1) || (rank > MAX_RANK)) { + return {IdxMapping(), "rank is invalid"}; + } else if (extents == nullptr) { + return {IdxMapping(), "extents is a nullptr"}; + } + for (int i = 0; i < rank; i++) { + if (extents[i] < 1) { + return {IdxMapping(), "extents must hold positive vals"}; + } + } + + // build and return the mapping + IdxMapping mapping; + mapping.rank_ = rank; + for (int i = 0; i < rank; i++) { + mapping.extents_[i] = extents[i]; + } + return {mapping, nullptr}; + } + + /// factory method that aborts with an error message upon failure + static IdxMapping create_or_abort_(int rank, const int* extents) { + InnerMappingMsgPair_ pair = IdxMapping::create_(rank, extents); + if (pair.second != nullptr) { + GR_INTERNAL_ERROR("%s", pair.second); + } + return pair.first; + } + +public: + /// factory method that returns an empty optional upon failure + static std::optional> try_create(int rank, + const int* extents) { + InnerMappingMsgPair_ pair = IdxMapping::create_(rank, extents); + return (pair.second == nullptr) + ? std::optional>{pair.first} + : std::optional>{}; + } + + explicit IdxMapping(int extent0) { + // for less experienced C++ devs: the `explicit` kwarg prevents the use of + // this constructor for implicit casts + int extents[1] = {extent0}; + *this = create_or_abort_(1, extents); + } + + IdxMapping(int extent0, int extent1) { + int extents[2] = {extent0, extent1}; + *this = create_or_abort_(2, extents); + } + + IdxMapping(int extent0, int extent1, int extent2) { + int extents[3] = {extent0, extent1, extent2}; + *this = create_or_abort_(3, extents); + } + + IdxMapping(const IdxMapping&) = default; + IdxMapping(IdxMapping&&) = default; + IdxMapping& operator=(const IdxMapping&) = default; + IdxMapping& operator=(IdxMapping&&) = default; + ~IdxMapping() = default; + + /// access the rank + int rank() const noexcept { return rank_; } + + /// access the extents pointer (the length is given by @ref rank) + const int* extents() const noexcept { return extents_; } + + /// construct an equivalent 3d IdxMapping + IdxMapping to_3d_mapping() const noexcept { + int out_rank = 3; + int rank_diff = out_rank - this->rank_; + GR_INTERNAL_REQUIRE(rank_diff >= 0, "current rank exceeds new rank"); + + IdxMapping out; + out.rank_ = out_rank; + for (int i = 0; i < out_rank; i++) { + out.extents_[i] = 1; + } + int offset = (Layout == DataLayout::RIGHT) ? rank_diff : 0; + for (int i = out; i < this->rank_; i++) { + out.extents_[i + offset] = this->extents_[i]; + } + return out; + } + + static constexpr bool is_contiguous() { return true; } + + int n_elements() const noexcept { + int product = 1; + for (int i = 0; i < this->rank_; i++) { + product *= this->extents_[i]; + } + return product; + } + + /** @{ */ // <- open the group of member functions with a shared docstring + /// compute the 1D pointer offset associated with the multidimensional index + /// + /// @note + /// For less experienced C++ devs, these methods overloads the "function call + /// operator". In python they would be named `__call__(self, ...)` + /// + /// Behavior is undefined if the number of arguments doesn't match the value + /// returned by `this->rank()` + [[gnu::always_inline]] int operator()(int i) const noexcept { + assert(this->rank_ == 1); + return i; + } + + [[gnu::always_inline]] int operator()(int i, int j) const noexcept { + assert(this->rank_ == 2); + if constexpr (Layout == DataLayout::LEFT) { + return i + this->extents_[0] * j; + } else { // Layout == DataLayout::RIGHT + return j + this->extents_[1] * i; + } + } + + [[gnu::always_inline]] int operator()(int i, int j, int k) const noexcept { + assert(this->rank_ == 3); + if constexpr (Layout == DataLayout::LEFT) { + return i + this->extents_[0] * (j + k * this->extents_[1]); + } else { // Layout == DataLayout::RIGHT + return k + this->extents_[2] * (j + i * this->extents_[1]); + } + } + /** @} */ // <- close the group of member functions with a shared docstring + + /// Convert a 1D pointer offset to a multidimensional index + /// + /// The number of components for the output index is given by calling the + /// @ref rank method. Behavior is undefined if @p out doesn't have enough + /// space. + /// + /// @param[in] offset The 1D pointer offset (must be non-negative) + /// @param[out] out Buffer where result is stored + void offset_to_md_idx(int offset, int* out) const noexcept { + assert(out != nullptr); + + const int contig_ax = (Layout == DataLayout::LEFT) ? 0 : (this->rank_ - 1); + const int slowest_ax = (Layout == DataLayout::LEFT) ? (this->rank_ - 1) : 0; + + switch (this->rank_) { + case 1: + out[0] = offset; + return; + case 2: + out[slowest_ax] = offset / this->extents_[contig_ax]; + out[contig_ax] = offset % this->extents_[contig_ax]; + return; + case 3: { + int largest_stride = this->extents_[contig_ax] * this->extents_[1]; + out[slowest_ax] = offset / largest_stride; + int remainder = offset % largest_stride; + out[1] = remainder / this->extents_[contig_ax]; + out[contig_ax] = remainder % this->extents_[contig_ax]; + return; + } + default: + GR_INTERNAL_ERROR("should be unreachable"); + } + } + + // teach googletest how to print this type + friend void PrintTo(const IdxMapping& mapping, std::ostream* os) { + const char* layout = + (Layout == DataLayout::LEFT) ? "DataLayout::LEFT" : "DataLayout::RIGHT"; + *os << "IdxMapping<" << layout << ">("; + for (int i = 0; i < mapping.rank_; i++) { + *os << mapping.extents_[i]; + *os << ((i + 1) < mapping.rank_ ? ',' : ')'); + } + } +}; + /// equivalent of converting output of printf("%g", val) to std::string std::string to_pretty_string(float val); std::string to_pretty_string(double val); @@ -29,6 +293,20 @@ std::string to_pretty_string(double val); std::string ptr_to_string(const float* ptr, std::size_t len); std::string ptr_to_string(const double* ptr, std::size_t len); +/// formats a pointer as a string (current implementation prints a 1D array) +template +std::string ptr_to_string(const float* ptr, IdxMapping idx_mapping) { + static_assert(IdxMapping::is_contiguous()); + return ptr_to_string(ptr, idx_mapping.n_elements()); +} + +/// formats a pointer as a string (current implementation prints a 1D array) +template +std::string ptr_to_string(const double* ptr, IdxMapping idx_mapping) { + static_assert(IdxMapping::is_contiguous()); + return ptr_to_string(ptr, idx_mapping.n_elements()); +} + } // namespace grtest #endif // GRTESTUTILS_VIEW_HPP \ No newline at end of file From 0b8b3f3bbc3aeeee93917e723dbc792581264c48 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Mon, 23 Feb 2026 11:21:54 -0500 Subject: [PATCH 22/29] Refactor check_allclose --- .../grtestutils/googletest/check_allclose.cpp | 325 +++++++++++++++--- .../grtestutils/googletest/check_allclose.hpp | 77 ++++- .../googletest/check_allclose_detail.hpp | 143 ++++++++ 3 files changed, 485 insertions(+), 60 deletions(-) create mode 100644 tests/grtestutils/googletest/check_allclose_detail.hpp diff --git a/tests/grtestutils/googletest/check_allclose.cpp b/tests/grtestutils/googletest/check_allclose.cpp index ae6ca702b..d13239ffb 100644 --- a/tests/grtestutils/googletest/check_allclose.cpp +++ b/tests/grtestutils/googletest/check_allclose.cpp @@ -12,81 +12,294 @@ #include "./check_allclose.hpp" #include "../view.hpp" +#include "gtest/gtest.h" +#include "status_reporting.h" #include +#include #include +#include #include #include -testing::AssertionResult check_allclose(const std::vector& actual, - const std::vector& desired, - double rtol, double atol) { - if (actual.size() != desired.size()) { - return testing::AssertionFailure() - << "the compared arrays have different lengths"; +namespace grtest::arraycmp_detail { + +namespace { // stuff inside an anonymous namespace is local to this file + +/// calls the unary function once for each element +/// +/// if selected is provided, we skip locations where selected[i] is false +template +void flat_for_each(Fn fn, int n_elements, const bool* selected) { + if (selected == nullptr) { + for (int flat_idx = 0; flat_idx < n_elements; flat_idx++) { + fn(flat_idx); + } + } else { + for (int flat_idx = 0; flat_idx < n_elements; flat_idx++) { + if (selected[flat_idx]) { + fn(flat_idx); + } + } } +} + +/// Specifies a noteworthy detail about a mismatching pairs of pointers +/// +/// There are 2 kinds of details: +/// - a detail describing a particular pair of mismatching elements (e.g. the +/// very first mismatch, the place where the size of the mismatch is most +/// significant). In these cases, the flattened index is tracked so that +/// the location of the mismatch and the values of the elements can be +/// printed +/// - a generic string that doesn't have any single associated location +struct MismatchDetail { + std::string description; + std::optional flat_idx; + + // the following constructor is defined in order to make this work with + // std::vector::emplace_back. Delete it, once we require C++20 or newer +#if __cpp_aggregate_paren_init < 201902L + MismatchDetail(const std::string& description, + const std::optional& flat_idx) + : description(description), flat_idx(flat_idx) {} +#endif +}; + +/// collects interesting details about mismatched elements in a pair of pointers +/// +/// This is called by @ref compare_ptrs_, if we determine that the compared +/// pointers contain at least one pair of mismatching elements. This loops back +/// over all pairs of elements and collects a vector of noteworthy mismatches. +/// +/// @note +/// It's ok if this is a little slow, as long as it provides useful messages. +/// (After all, this logic only gets invoked when comparisons fail). +template +std::vector collect_details_( + const T* actual, const T* desired, const IdxMapping& idx_mapping, + const bool* selection_mask, Cmp cmp_fn) { + // define some variables that we will fill as we loop over the array + int first_mismatch_idx = -1; + + int first_nan_mismatch_idx = -1; + int nan_mismatch_count = 0; + bool any_nan = false; - std::size_t num_mismatches = 0; double max_absDiff = 0.0; - std::size_t max_absDiff_ind = 0; + int max_absDiff_idx = -1; + double max_relDiff = 0.0; - std::size_t max_relDiff_ind = 0; - bool has_nan_mismatch = false; - - for (std::size_t i = 0; i < actual.size(); i++) { - double cur_absDiff = std::fabs(actual[i] - desired[i]); - - bool isnan_actual = std::isnan(actual[i]); - bool isnan_desired = std::isnan(desired[i]); - - if ((cur_absDiff > (atol + rtol * std::fabs(desired[i]))) || - (isnan_actual != isnan_desired)) { - num_mismatches++; - if (isnan_actual != isnan_desired) { - has_nan_mismatch = true; - max_absDiff = NAN; - max_absDiff_ind = i; - max_relDiff = NAN; - max_relDiff_ind = i; - } else if (!has_nan_mismatch) { + int max_relDiff_idx = -1; + + auto fn = [&](int flat_idx) { + bool either_nan = + std::isnan(actual[flat_idx]) || std::isnan(desired[flat_idx]); + any_nan = any_nan || either_nan; // <- record whether we have seen a NaN + + // record properties if there is a mismatch + if (!cmp_fn(actual[flat_idx], desired[flat_idx])) { + if (first_mismatch_idx == -1) { + first_mismatch_idx = flat_idx; + } + nan_mismatch_count += either_nan; + if (either_nan && first_nan_mismatch_idx == -1) { + first_nan_mismatch_idx = flat_idx; + } else if (!either_nan) { + double cur_absDiff = std::fabs(actual[flat_idx] - desired[flat_idx]); if (cur_absDiff > max_absDiff) { max_absDiff = cur_absDiff; - max_absDiff_ind = i; + max_absDiff_idx = flat_idx; } - if (cur_absDiff > (max_relDiff * std::fabs(desired[i]))) { - max_relDiff = cur_absDiff / std::fabs(desired[i]); - max_relDiff_ind = i; + if (cur_absDiff > (max_relDiff * std::fabs(desired[flat_idx]))) { + max_relDiff = cur_absDiff / std::fabs(desired[flat_idx]); + max_relDiff_idx = flat_idx; } } } + }; + flat_for_each(fn, idx_mapping.n_elements(), selection_mask); + + // now, let's construct the vector of details + std::vector details; + + if (first_mismatch_idx == -1) { + return details; // <- this is probably indicative of an error + } else { + details.emplace_back("first mismatch", + std::optional{first_mismatch_idx}); + } + + if (max_absDiff_idx == -1) { + details.emplace_back("Max abs diff: NaN (i.e. each mismatch involves NaN)", + std::nullopt); + } else { + details.emplace_back("Max abs diff: " + to_pretty_string(max_absDiff), + std::optional{max_absDiff_idx}); + } + + if (max_relDiff_idx == -1) { + details.emplace_back( + "Max rel diff: NaN (i.e. each mismatch involves NaN or has actual=0.0)", + std::nullopt); + } else { + details.emplace_back("Max rel diff: " + to_pretty_string(max_relDiff), + std::optional{max_relDiff_idx}); + } + + if (first_nan_mismatch_idx == -1) { + details.emplace_back(any_nan ? "all NaNs match" : "there are no NaNs", + std::nullopt); + } else { + details.emplace_back( + "First (of " + std::to_string(nan_mismatch_count) + ") NaN mismatch", + first_nan_mismatch_idx); } - if (num_mismatches == 0) { + return details; +} + +/// Returns a `testing::AssertionResult` instance specifying whether all pairs +/// of values from @p actual and @p desired pointers satisfy the comparison +/// operation specified by @p cmp_fn +/// +/// @tparam T is either `float` or `double` +/// @tparam Layout specifies the data-layout +/// @tparam Cmp Function-like type that does the underlying comparison. See the +/// description of the @p cmp_fn function for more details +/// +/// @param actual,desired The pointers being compared +/// @param idx_mapping Specifies information for treating the pointers as +/// contiguous multi-dimensional arrays. It maps between multi-dimensional +/// indices & pointer 1d offsets, and specifies all relevant information +/// for this mapping (i.e. extents and data layout) +/// @param selection_mask When specified, only the locations holding `true` +/// values are compared +/// @param cmp_fn "Callable" object that implements a function signature +/// equivalent to `bool fun(T actual, T desired)`. This signature is called +/// by passing pairs of values from the @p actual and @p desired pointers. +/// This should implement a member function called `describe_false` that +/// returns a `std::string` +template +testing::AssertionResult compare_ptrs_(const T* actual, const T* desired, + const IdxMapping& idx_mapping, + const bool* selection_mask, Cmp cmp_fn) { + GR_INTERNAL_REQUIRE(actual != nullptr && desired != nullptr, + "it's illegal to compare nullptr"); + // Part 1: perform the comparison (this is as fast as possible) + const int n_elements = idx_mapping.n_elements(); + int mismatch_num = 0; + int n_comparisons = 0; + auto loop_callback = [=, &mismatch_num, &n_comparisons](int flat_idx) { + n_comparisons++; + mismatch_num += !cmp_fn(actual[flat_idx], desired[flat_idx]); + }; + flat_for_each(loop_callback, n_elements, selection_mask); + + if (mismatch_num == 0) { return testing::AssertionSuccess(); } - std::string actual_vec_str = - grtest::ptr_to_string(actual.data(), actual.size()); - std::string ref_vec_str = - grtest::ptr_to_string(desired.data(), desired.size()); - - using grtest::to_pretty_string; - - return testing::AssertionFailure() - << "\narrays are unequal for the tolerance: " - << "rtol = " << to_pretty_string(rtol) << ", " - << "atol = " << to_pretty_string(atol) << '\n' - << "Mismatched elements: " << num_mismatches << " / " << actual.size() - << '\n' - << "Max absolute difference: " << to_pretty_string(max_absDiff) << ", " - << "ind = " << max_absDiff_ind << ", " - << "actual = " << to_pretty_string(actual[max_absDiff_ind]) << ", " - << "reference = " << to_pretty_string(desired[max_absDiff_ind]) << '\n' - << "Max relative difference: " << to_pretty_string(max_relDiff) << ", " - << "ind = " << max_absDiff_ind << ", " - << "actual = " << to_pretty_string(actual[max_relDiff_ind]) << ", " - << "desired = " << to_pretty_string(desired[max_relDiff_ind]) << '\n' - << "actual: " << actual_vec_str << '\n' - << "desired: " << ref_vec_str << '\n'; -} \ No newline at end of file + // Part 2: build the failure result and construct the detailed error message + // -> it's ok if this isn't extremely optimized. This logic shouldn't come up + // very frequently + testing::AssertionResult out = testing::AssertionFailure(); + + out << '\n' + << "arrays are " << cmp_fn.describe_false() << '\n' + << "index mapping: " << testing::PrintToString(idx_mapping) << '\n'; + out << "Mismatched elements: " << mismatch_num << " (" << n_comparisons + << " were compared"; + if (n_comparisons != n_elements) { + out << ", " << n_elements - n_comparisons << "ignored from masking"; + } + out << ")\n"; + + std::vector detail_vec = + collect_details_(actual, desired, idx_mapping, selection_mask, cmp_fn); + if (detail_vec.empty()) { + GR_INTERNAL_ERROR("something went wrong with finding mismatch details"); + } + + // now let's append the interesting mismatch details + for (const MismatchDetail& detail : detail_vec) { + if (!detail.flat_idx.has_value()) { + out << detail.description << '\n'; + continue; + } + int flat_idx = detail.flat_idx.value(); + + out << detail.description << ", "; + // write the index + int idx_components[IdxMapping::MAX_RANK]; + idx_mapping.offset_to_md_idx(flat_idx, idx_components); + out << "idx: {"; + for (int i = 0; i < idx_mapping.rank(); i++) { + out << idx_components[i]; + out << ((i + 1) < idx_mapping.rank() ? ',' : '}'); + } + out << ", "; + + // write the actual and description value + out << "actual = " << to_pretty_string(actual[flat_idx]) << ", " + << "desired = " << to_pretty_string(desired[flat_idx]) << '\n'; + } + + // print out final summary details + bool has_mask = selection_mask != nullptr; + out << "Flattened Ptr Details" + << (has_mask ? " (selection mask is ignored):\n" : ":\n") + << " actual: " << ptr_to_string(actual, idx_mapping) << '\n' + << " desired: " << ptr_to_string(desired, idx_mapping); + return out; +} + +} // anonymous namespace + +testing::AssertionResult compare_(CmpPack pack) { + // this function launches the appropriate specialization of compare_ptrs_ + // -> there are 3 template parameters to consider + // -> (see the docstring in the header file for a little more context) + + // load either (f32_actual, f32_desired) OR (f64_actual, f64_desired) + const float *f32_actual, *f32_desired; + const double *f64_actual, *f64_desired; + bool use_f32 = + std::holds_alternative>(pack.actual_desired_pair); + if (use_f32) { + f32_actual = std::get>(pack.actual_desired_pair).first; + f32_desired = std::get>(pack.actual_desired_pair).second; + } else { + f64_actual = std::get>(pack.actual_desired_pair).first; + f64_desired = std::get>(pack.actual_desired_pair).second; + } + + // Either idx_map_L OR idx_map_R will not be a nullptr + const IdxMapping* idx_map_L = + std::get_if>(&pack.idx_mapping); + const IdxMapping* idx_map_R = + std::get_if>(&pack.idx_mapping); + + // dispatcher_ is a "generic lambda" + // -> it acts as if the type of cmp_fn is a template parameter. + // -> when we pass it to std::visit, the cmp_fn argument is a copy of the + // alternative currently held by the `CmpPack::cmp_fn` variant + auto dispatcher_ = [&](auto cmp_fn) -> testing::AssertionResult { + const bool* smask = pack.selection_mask; + if (use_f32 && idx_map_L != nullptr) { + return compare_ptrs_(f32_actual, f32_desired, *idx_map_L, smask, cmp_fn); + } else if (use_f32 && idx_map_R != nullptr) { + return compare_ptrs_(f32_actual, f32_desired, *idx_map_R, smask, cmp_fn); + } else if (idx_map_L != nullptr) { + return compare_ptrs_(f64_actual, f64_desired, *idx_map_L, smask, cmp_fn); + } else if (idx_map_R != nullptr) { + return compare_ptrs_(f64_actual, f64_desired, *idx_map_R, smask, cmp_fn); + } else { + GR_INTERNAL_ERROR("should be unreachable"); + } + }; + return std::visit(dispatcher_, pack.cmp_fn); +} + +} // namespace grtest::arraycmp_detail \ No newline at end of file diff --git a/tests/grtestutils/googletest/check_allclose.hpp b/tests/grtestutils/googletest/check_allclose.hpp index dedb5049a..615e5e607 100644 --- a/tests/grtestutils/googletest/check_allclose.hpp +++ b/tests/grtestutils/googletest/check_allclose.hpp @@ -12,18 +12,87 @@ #ifndef GRTESTUTILS_GOOGLETEST_CHECK_ALLCLOSE_HPP #define GRTESTUTILS_GOOGLETEST_CHECK_ALLCLOSE_HPP -#include #include + #include +#include "../view.hpp" +#include "./check_allclose_detail.hpp" + +#define COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping) \ + ::grtest::arraycmp_detail::compare_(::grtest::arraycmp_detail::CmpPack{ \ + {cmp_fn}, \ + {::grtest::arraycmp_detail::PtrPair{actual, desired}}, \ + selection_mask, \ + {idx_mapping}}) + +/// Returns whether 2 pointers are exactly equal +/// +/// This draws a lot of inspiration from numpy.testing.assert_array_equal +template +testing::AssertionResult check_array_equal( + const float* actual, const float* desired, + grtest::IdxMapping idx_mapping, + const bool* selection_mask = nullptr) { + grtest::arraycmp_detail::FltIsEqual cmp_fn; + return COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping); +} + +/// Returns whether 2 pointers are exactly equal +/// +/// This draws a lot of inspiration from numpy.testing.assert_array_equal +template +testing::AssertionResult check_array_equal( + const double* actual, const double* desired, + grtest::IdxMapping idx_mapping, + const bool* selection_mask = nullptr) { + grtest::arraycmp_detail::FltIsEqual cmp_fn; + return COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping); +} + +/// compares 2 pointers +/// +/// This draws a lot of inspiration from numpy.testing.assert_allclose +template +testing::AssertionResult check_allclose(const float* actual, + const float* desired, + grtest::IdxMapping idx_mapping, + double rtol, double atol = 0.0, + const bool* selection_mask = nullptr) { + grtest::arraycmp_detail::FltIsClose cmp_fn(rtol, atol); + return COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping); +} + +/// compares 2 pointers +/// +/// This draws a lot of inspiration from numpy.testing.assert_allclose +template +testing::AssertionResult check_allclose(const double* actual, + const double* desired, + grtest::IdxMapping idx_mapping, + double rtol, double atol = 0.0, + const bool* selection_mask = nullptr) { + grtest::arraycmp_detail::FltIsClose cmp_fn(rtol, atol); + return COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping); +} + +#undef COMPARE_ + /// this compares 2 std::vectors /// /// This draws a lot of inspiration from numpy.testing.assert_allclose /// /// Parts of this are fairly inefficient, partially because it is adapted from /// code written from before we adopted googletest -testing::AssertionResult check_allclose(const std::vector& actual, - const std::vector& desired, - double rtol = 0.0, double atol = 0.0); +inline testing::AssertionResult check_allclose( + const std::vector& actual, const std::vector& desired, + double rtol = 0.0, double atol = 0.0) { + if (actual.size() != desired.size()) { + return testing::AssertionFailure() + << "the compared arrays have different lengths"; + } + grtest::IdxMapping idx_mapping(actual.size()); + return check_allclose(actual.data(), desired.data(), idx_mapping, rtol, atol); +} #endif // GRTESTUTILS_GOOGLETEST_CHECK_ALLCLOSE_HPP diff --git a/tests/grtestutils/googletest/check_allclose_detail.hpp b/tests/grtestutils/googletest/check_allclose_detail.hpp new file mode 100644 index 000000000..9285b5d5c --- /dev/null +++ b/tests/grtestutils/googletest/check_allclose_detail.hpp @@ -0,0 +1,143 @@ +//===----------------------------------------------------------------------===// +// +// See the LICENSE file for license and copyright information +// SPDX-License-Identifier: NCSA AND BSD-3-Clause +// +//===----------------------------------------------------------------------===// +/// +/// @file +/// Declares/implements a bunch of helper code to assist with implementing +/// logic in check_allclose.hpp +/// +//===----------------------------------------------------------------------===// +#ifndef GRTESTUTILS_GOOGLETEST_CHECK_ALLCLOSE_DETAIL_HPP +#define GRTESTUTILS_GOOGLETEST_CHECK_ALLCLOSE_DETAIL_HPP + +#include +#include +#include +#include +#include + +#include "../view.hpp" +#include "gtest/gtest.h" + +namespace grtest::arraycmp_detail { + +template +[[gnu::always_inline]] bool isclose_(T actual, T desired, double rtol, + double atol) { + static_assert(std::is_floating_point_v); + + T abs_diff = std::fabs(actual - desired); + // the following variable is false if actual or desired (or both) is NaN + bool isclose = abs_diff <= atol + rtol * std::fabs(desired); + + if constexpr (EqualNan) { + bool actual_isnan = std::isnan(actual); + bool desired_isnan = std::isnan(desired); + return (actual_isnan && desired_isnan) || isclose; + } else { + return isclose; + } +} + +/// "functor" to check if floating point values are equal within a tolerance +/// +/// This effectively implements numpy's isclose function (see +/// https://numpy.org/doc/stable/reference/generated/numpy.isclose.html). As in +/// the original function, the max allowed variations from the relative +/// difference tolerance and the absolute difference tolerance are summed and +/// compared against the absolute difference. +/// +/// @note +/// For less experienced C++ developers, `operator()` overloads the "function +/// call operation" (it is analogous to python's `__call__` method) +class FltIsClose { + double rtol; + double atol; + +public: + FltIsClose() = delete; + FltIsClose(double rtol, double atol) : rtol{rtol}, atol{atol} {} + + /// determines whether arguments are equal within the tolerance + bool operator()(float actual, float desired) const noexcept { + return isclose_(actual, desired, this->rtol, this->atol); + } + + /// determines whether arguments are equal within the tolerance + bool operator()(double actual, double desired) const noexcept { + return isclose_(actual, desired, this->rtol, this->atol); + } + + /// describe relationship between values for which this functor returns false + std::string describe_false() const { + std::string rtol_str = to_pretty_string(rtol); + std::string atol_str = to_pretty_string(atol); + return ("unequal for the tolerance (rtol = " + rtol_str + + ", atol = " + atol_str + ")"); + } +}; + +/// "functor" to check if floating point values are exactly equal +/// +/// @note +/// For less experienced C++ developers, `operator()` overloads the "function +/// call operation" (it is analogous to python's `__call__` method) +struct FltIsEqual { + /// determines whether arguments are exactly equal + bool operator()(float actual, float desired) const noexcept { + return (actual == desired) || std::isnan(actual) == std::isnan(desired); + } + + /// determines whether arguments are exactly equal + bool operator()(double actual, double desired) const noexcept { + return (actual == desired) || std::isnan(actual) == std::isnan(desired); + } + + /// describe relationship between values for which this functor returns false + std::string describe_false() const { return "not exactly equal"; } +}; + +template +using PtrPair = std::pair; + +/// Packages up the information for a comparison of 2 pointers +/// +/// See the docstring of @ref compare_ for an extended discussion for why +/// this type actually exists. +struct CmpPack { + std::variant cmp_fn; + std::variant, PtrPair> actual_desired_pair; + const bool* selection_mask; + std::variant, IdxMapping> + idx_mapping; +}; + +/// this dispatches the appropriate logic to drive the comparison +/// +/// The most pragmatic approach for implementing the underlying comparisons in +/// an extendable manner (without extensive code duplication or sacrificing +/// performance) is to implement them using templates and to make the datatype +/// a template parameter. +/// +/// This function was designed in a misguided attempt to shift most of the +/// implementation into source files in order to reduce compile times. In order +/// to hide all calls to a set of templates into a source file, this must be +/// a totally ordinary function that dispatches to the proper templates: +/// - thus, @ref CmpPack as a well-defined type to package up all of the +/// possible type combinations. This is achieved through the use of +/// std::variant (i.e. type-safe unions). +/// - the idea is that callers package up `CmpPack`, call this function, and +/// then function unpacks the values from `CmpPack` and dispatches to the +/// appropriate template function. +/// +/// With the benefit of hindsight, this was probably all a mistake... Reducing +/// the compilation cost was probably **NOT** worth the added complexity (if +/// nothing else, we probably should have measured it first...) +testing::AssertionResult compare_(CmpPack pack); + +} // namespace grtest::arraycmp_detail + +#endif // GRTESTUTILS_GOOGLETEST_CHECK_ALLCLOSE_DETAIL_HPP \ No newline at end of file From 8eaefaca8a8f5552a381c0d4cd636c5b71277186 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Tue, 24 Feb 2026 09:17:27 -0500 Subject: [PATCH 23/29] lightly refactor test_linalg --- tests/grtestutils/googletest/check_allclose.hpp | 17 ----------------- tests/unit/test_linalg.cpp | 7 +++++-- 2 files changed, 5 insertions(+), 19 deletions(-) diff --git a/tests/grtestutils/googletest/check_allclose.hpp b/tests/grtestutils/googletest/check_allclose.hpp index 615e5e607..2e34d7977 100644 --- a/tests/grtestutils/googletest/check_allclose.hpp +++ b/tests/grtestutils/googletest/check_allclose.hpp @@ -78,21 +78,4 @@ testing::AssertionResult check_allclose(const double* actual, #undef COMPARE_ -/// this compares 2 std::vectors -/// -/// This draws a lot of inspiration from numpy.testing.assert_allclose -/// -/// Parts of this are fairly inefficient, partially because it is adapted from -/// code written from before we adopted googletest -inline testing::AssertionResult check_allclose( - const std::vector& actual, const std::vector& desired, - double rtol = 0.0, double atol = 0.0) { - if (actual.size() != desired.size()) { - return testing::AssertionFailure() - << "the compared arrays have different lengths"; - } - grtest::IdxMapping idx_mapping(actual.size()); - return check_allclose(actual.data(), desired.data(), idx_mapping, rtol, atol); -} - #endif // GRTESTUTILS_GOOGLETEST_CHECK_ALLCLOSE_HPP diff --git a/tests/unit/test_linalg.cpp b/tests/unit/test_linalg.cpp index d3fb7d080..829349059 100644 --- a/tests/unit/test_linalg.cpp +++ b/tests/unit/test_linalg.cpp @@ -3,7 +3,7 @@ #include "fortran_func_wrappers.hpp" #include "grtestutils/googletest/check_allclose.hpp" - +#include "grtestutils/view.hpp" /// Records the paramters for a linear algebra test-case struct LinAlgCase { @@ -63,7 +63,10 @@ TEST_P(LinAlgTestSolve, CheckSuccessfulSolve) { ASSERT_EQ(rslt, 0) << "expected a return-code of 0, which indicates " << "that the linear equations were successfully solved"; - EXPECT_TRUE(check_allclose(/* actual: */ vec, my_case.solution_vector, + grtest::IdxMapping idx_mapping(vec.size()); + EXPECT_TRUE(check_allclose(/* actual: */ vec.data(), + /* desired: */ my_case.solution_vector.data(), + /* idx_mapping: */ idx_mapping, /* rtol: */1e-15, /* atol: */ 0.0)); } From 9f084926c54cc54578c6f2af450bed106ff67983 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Tue, 24 Feb 2026 14:19:41 -0500 Subject: [PATCH 24/29] Address gcc-specific issues --- .../grtestutils/googletest/check_allclose.cpp | 4 ++-- .../grtestutils/googletest/check_allclose.hpp | 19 ++++++++++--------- .../googletest/check_allclose_detail.hpp | 4 ++-- 3 files changed, 14 insertions(+), 13 deletions(-) diff --git a/tests/grtestutils/googletest/check_allclose.cpp b/tests/grtestutils/googletest/check_allclose.cpp index d13239ffb..b0f9d5602 100644 --- a/tests/grtestutils/googletest/check_allclose.cpp +++ b/tests/grtestutils/googletest/check_allclose.cpp @@ -263,8 +263,8 @@ testing::AssertionResult compare_(CmpPack pack) { // -> (see the docstring in the header file for a little more context) // load either (f32_actual, f32_desired) OR (f64_actual, f64_desired) - const float *f32_actual, *f32_desired; - const double *f64_actual, *f64_desired; + const float *f32_actual = nullptr, *f32_desired = nullptr; + const double *f64_actual = nullptr, *f64_desired = nullptr; bool use_f32 = std::holds_alternative>(pack.actual_desired_pair); if (use_f32) { diff --git a/tests/grtestutils/googletest/check_allclose.hpp b/tests/grtestutils/googletest/check_allclose.hpp index 2e34d7977..6b2686f59 100644 --- a/tests/grtestutils/googletest/check_allclose.hpp +++ b/tests/grtestutils/googletest/check_allclose.hpp @@ -19,12 +19,9 @@ #include "../view.hpp" #include "./check_allclose_detail.hpp" -#define COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping) \ +#define COMPARE_(cmp_fn, ptr_pair, selection_mask, idx_mapping) \ ::grtest::arraycmp_detail::compare_(::grtest::arraycmp_detail::CmpPack{ \ - {cmp_fn}, \ - {::grtest::arraycmp_detail::PtrPair{actual, desired}}, \ - selection_mask, \ - {idx_mapping}}) + {cmp_fn}, {ptr_pair}, selection_mask, {idx_mapping}}) /// Returns whether 2 pointers are exactly equal /// @@ -35,7 +32,8 @@ testing::AssertionResult check_array_equal( grtest::IdxMapping idx_mapping, const bool* selection_mask = nullptr) { grtest::arraycmp_detail::FltIsEqual cmp_fn; - return COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping); + grtest::arraycmp_detail::PtrPair ptr_pair{actual, desired}; + return COMPARE_(cmp_fn, ptr_pair, selection_mask, idx_mapping); } /// Returns whether 2 pointers are exactly equal @@ -47,7 +45,8 @@ testing::AssertionResult check_array_equal( grtest::IdxMapping idx_mapping, const bool* selection_mask = nullptr) { grtest::arraycmp_detail::FltIsEqual cmp_fn; - return COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping); + grtest::arraycmp_detail::PtrPair ptr_pair{actual, desired}; + return COMPARE_(cmp_fn, ptr_pair, selection_mask, idx_mapping); } /// compares 2 pointers @@ -60,7 +59,8 @@ testing::AssertionResult check_allclose(const float* actual, double rtol, double atol = 0.0, const bool* selection_mask = nullptr) { grtest::arraycmp_detail::FltIsClose cmp_fn(rtol, atol); - return COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping); + grtest::arraycmp_detail::PtrPair ptr_pair{actual, desired}; + return COMPARE_(cmp_fn, ptr_pair, selection_mask, idx_mapping); } /// compares 2 pointers @@ -73,7 +73,8 @@ testing::AssertionResult check_allclose(const double* actual, double rtol, double atol = 0.0, const bool* selection_mask = nullptr) { grtest::arraycmp_detail::FltIsClose cmp_fn(rtol, atol); - return COMPARE_(cmp_fn, actual, desired, selection_mask, idx_mapping); + grtest::arraycmp_detail::PtrPair ptr_pair{actual, desired}; + return COMPARE_(cmp_fn, ptr_pair, selection_mask, idx_mapping); } #undef COMPARE_ diff --git a/tests/grtestutils/googletest/check_allclose_detail.hpp b/tests/grtestutils/googletest/check_allclose_detail.hpp index 9285b5d5c..a0dd264d6 100644 --- a/tests/grtestutils/googletest/check_allclose_detail.hpp +++ b/tests/grtestutils/googletest/check_allclose_detail.hpp @@ -25,8 +25,8 @@ namespace grtest::arraycmp_detail { template -[[gnu::always_inline]] bool isclose_(T actual, T desired, double rtol, - double atol) { +[[gnu::always_inline]] inline bool isclose_(T actual, T desired, double rtol, + double atol) { static_assert(std::is_floating_point_v); T abs_diff = std::fabs(actual - desired); From c182e48740e39e20fd8f295eb2f9ebeb6a31e9f6 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 28 Feb 2026 12:44:09 -0500 Subject: [PATCH 25/29] refactor IdxMapping to make use of Status --- .../grtestutils/googletest/check_allclose.hpp | 2 - tests/grtestutils/view.hpp | 49 ++++++++----------- 2 files changed, 21 insertions(+), 30 deletions(-) diff --git a/tests/grtestutils/googletest/check_allclose.hpp b/tests/grtestutils/googletest/check_allclose.hpp index 6b2686f59..b8bd95364 100644 --- a/tests/grtestutils/googletest/check_allclose.hpp +++ b/tests/grtestutils/googletest/check_allclose.hpp @@ -12,8 +12,6 @@ #ifndef GRTESTUTILS_GOOGLETEST_CHECK_ALLCLOSE_HPP #define GRTESTUTILS_GOOGLETEST_CHECK_ALLCLOSE_HPP -#include - #include #include "../view.hpp" diff --git a/tests/grtestutils/view.hpp b/tests/grtestutils/view.hpp index 102cee1fc..b15ce8819 100644 --- a/tests/grtestutils/view.hpp +++ b/tests/grtestutils/view.hpp @@ -23,6 +23,8 @@ #include #include // std::pair +#include "./harness/status.hpp" + #include "status_reporting.h" namespace grtest { @@ -102,21 +104,31 @@ struct IdxMapping { // -> this explicitly set each extent_ to a value of 0 IdxMapping() : rank_(0), extents_{} {} - // the object returned by the create_ factory method: - // - `out.second == nullptr` indicates the method succeeded - // - otherwise, `out.second` is a string-literal specifying an error message - using InnerMappingMsgPair_ = std::pair, const char*>; + /// factory method that aborts with an error message upon failure + static IdxMapping create_or_abort_(int rank, const int* extents) { + std::pair, Status> pair = + IdxMapping::try_create(rank, extents); + if (pair.second.is_err()) { + std::string tmp = pair.second.to_string(); + GR_INTERNAL_ERROR("%s", tmp.c_str()); + } + return pair.first; + } - static InnerMappingMsgPair_ create_(int rank, const int* extents) noexcept { +public: + /// factory method that tries to create an instance + static std::pair, Status> try_create( + int rank, const int* extents) noexcept { // arg checking if ((rank < 1) || (rank > MAX_RANK)) { - return {IdxMapping(), "rank is invalid"}; + return {IdxMapping(), error::Adhoc("rank is invalid")}; } else if (extents == nullptr) { - return {IdxMapping(), "extents is a nullptr"}; + return {IdxMapping(), error::Adhoc("extents is a nullptr")}; } for (int i = 0; i < rank; i++) { if (extents[i] < 1) { - return {IdxMapping(), "extents must hold positive vals"}; + return {IdxMapping(), + error::Adhoc("extents must hold positive vals")}; } } @@ -126,26 +138,7 @@ struct IdxMapping { for (int i = 0; i < rank; i++) { mapping.extents_[i] = extents[i]; } - return {mapping, nullptr}; - } - - /// factory method that aborts with an error message upon failure - static IdxMapping create_or_abort_(int rank, const int* extents) { - InnerMappingMsgPair_ pair = IdxMapping::create_(rank, extents); - if (pair.second != nullptr) { - GR_INTERNAL_ERROR("%s", pair.second); - } - return pair.first; - } - -public: - /// factory method that returns an empty optional upon failure - static std::optional> try_create(int rank, - const int* extents) { - InnerMappingMsgPair_ pair = IdxMapping::create_(rank, extents); - return (pair.second == nullptr) - ? std::optional>{pair.first} - : std::optional>{}; + return {mapping, OkStatus()}; } explicit IdxMapping(int extent0) { From 64d338d40b89d27b8eab17b84ba970df6411d236 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 28 Feb 2026 13:20:29 -0500 Subject: [PATCH 26/29] refactor GridLayout so that it wraps IdxMapping --- tests/grtestutils/harness/field_container.cpp | 15 +-- tests/grtestutils/harness/field_container.hpp | 92 +++++++------------ tests/grtestutils/view.hpp | 30 +++++- 3 files changed, 70 insertions(+), 67 deletions(-) diff --git a/tests/grtestutils/harness/field_container.cpp b/tests/grtestutils/harness/field_container.cpp index 60c851741..d2a0e798f 100644 --- a/tests/grtestutils/harness/field_container.cpp +++ b/tests/grtestutils/harness/field_container.cpp @@ -17,6 +17,7 @@ #include "../utils.hpp" #include "grackle.h" +#include "preset.hpp" #include "status_reporting.h" #include // std::memcmp, std::memcpy @@ -27,23 +28,23 @@ namespace grtest { bool operator==(const GridLayout& lhs, const GridLayout& rhs) noexcept { - bool rank = lhs.rank_ == rhs.rank_; + bool idx_mapping = lhs.idx_mapping_ == rhs.idx_mapping_; bool start = std::memcmp(lhs.start_, rhs.start_, 3 * sizeof(int)) == 0; bool stop = std::memcmp(lhs.stop_, rhs.stop_, 3 * sizeof(int)) == 0; - bool dim = std::memcmp(lhs.dim_, rhs.dim_, 3 * sizeof(int)) == 0; - return rank && start && stop && dim; + return idx_mapping && start && stop; } void PrintTo(const GridLayout& layout, std::ostream* os) { + const int rank = layout.rank(); auto write_arr_ = [&](const int* vals) -> void { - for (int i = 0; i < layout.rank_; i++) { + for (int i = 0; i < rank; i++) { *os << ((i == 0) ? "{" : ", "); *os << vals[i]; } *os << '}'; }; - *os << "GridLayout{ dim: "; - write_arr_(layout.dim_); + *os << "GridLayout{"; + PrintTo(layout.idx_mapping_, os); *os << ", start: "; write_arr_(layout.start_); *os << ", stop: "; @@ -106,7 +107,7 @@ std::pair CorePack::setup_1d(MapType&& premade_map, } // setup grid properties - pack.my_fields->grid_dimension = pack.layout->dim_; + pack.my_fields->grid_dimension = pack.layout->idx_mapping_.unsafe_extents(); pack.my_fields->grid_start = pack.layout->start_; pack.my_fields->grid_end = pack.layout->end_; diff --git a/tests/grtestutils/harness/field_container.hpp b/tests/grtestutils/harness/field_container.hpp index fb20bf108..d3f403864 100644 --- a/tests/grtestutils/harness/field_container.hpp +++ b/tests/grtestutils/harness/field_container.hpp @@ -16,6 +16,7 @@ #include "./grackle_ctx_pack.hpp" #include "./status.hpp" +#include "../view.hpp" #include "status_reporting.h" #include // std::less @@ -36,45 +37,14 @@ struct CorePack; /// Represents Grid Properties /// -/// This type is probably a little over-engineered +/// This type maps multi-dimensional indices to a 1D pointer offset, specifies +/// the rank of the grid, and the extents of the grid (see the docstring of +/// @ref IdxMapping for more detail). It also specifies the region of valid +/// values. /// -/// Broader Context -/// =============== -/// To best describe this type, it's insightful to draw comparisons with C++ -/// conventions. -/// -/// Background -/// ---------- -/// For some background, C++23 introduced `std::mdspan` to describe -/// multi-dimensional views. A `std::mdspan` is parameterized by -/// - the data's extents (aka the shape) -/// - the data's layout, which dictates how a multidimensional index is mapped -/// to a 1D pointer offset -/// -/// For views of contiguous data there are 2 obvious layouts: -/// 1. layout-right: where the stride is `1` along the rightmost extent. -/// - for extents `{a,b,c}`, an optimal nested for-loop will iterates from -// `0` up to `a` in the outermost loop and from `0` up to `c` -/// in the innermost loop -/// - this is the "natural layout" for a multidimensional c-style array -/// `arr[a][b][c]` -/// 2. layout-left: where the stride is `1` along the leftmost extent -/// - for extents `{a,b,c}`, an optimal nested for-loop will iterates from -// `0` up to `c` in the outermost loop and from `0` up to `a` -/// in the innermost loop -/// - this is the "natural layout" for a multidimensional fortran array () -/// `arr[a][b][c]` -/// -/// > Aside: More sophisticated layouts are possible when strides along each -/// > axis aren't directly tied to extents (this comes up when making subviews). -/// -/// About this type -/// --------------- -/// This type specifies array extents for a layout-left mapping. It also -/// specifies the region of valid values. For context, arrays of -/// fluid-quantities in mesh-based hydro codes have an outer layer "ghost zones" -/// (Computer Scientists sometimes call this a "halo") that may not contain -/// valid values when calling Grackle. +/// For some additional context, arrays of fluid-quantities in mesh-based hydro +/// codes have an outer layer of "ghost zones" (Computer Scientists sometimes +/// call this a "halo") that may not contain valid values when calling Grackle. /// /// > Aside: We only describe the concept of "ghost zones" because we want to /// > to support tests for validating that ghost zones aren't modified. @@ -82,10 +52,8 @@ struct CorePack; class GridLayout { friend field_detail::CorePack; - /// the number of dimensions - int rank_; - /// number of elements along an axis - int dim_[3]; + /// describes extents, rank, and the index mapping + IdxMapping idx_mapping_; /// the first active-zone index along an axis int start_[3]; @@ -109,26 +77,24 @@ class GridLayout { const int* start, const int* stop) { GridLayout out; - out.rank_ = rank; - if ((rank < 1) || (rank > 3)) { - return {out, error::Adhoc("rank is invalid")}; - } else if (dim == nullptr) { - return {out, error::Adhoc("dim is a nullptr")}; + + std::pair, Status> idx_mapping_rslt = + IdxMapping::try_create(rank, dim); + if (idx_mapping_rslt.second.is_err()) { + return {out, idx_mapping_rslt.second}; } + out.idx_mapping_ = idx_mapping_rslt.first; for (int i = 0; i < 3; i++) { - out.dim_[i] = (i < rank) ? dim[i] : 1; out.start_[i] = (start != nullptr && i < rank) ? start[i] : 0; - out.stop_[i] = (stop != nullptr && i < rank) ? stop[i] : out.dim_[i]; + out.stop_[i] = (stop != nullptr && i < rank) ? stop[i] : 1; out.end_[i] = out.stop_[i] - 1; - if (out.dim_[i] < 1) { - return {out, error::Adhoc("dim must hold positive vals")}; - } else if (out.start_[i] < 0) { + if (out.start_[i] < 0) { return {out, error::Adhoc("start must hold non-negative vals")}; } else if (out.stop_[i] <= out.start_[i]) { return {out, error::Adhoc("stop must exceed start")}; - } else if (out.stop_[i] > out.dim_[i]) { + } else if (i < rank && out.stop_[i] > dim[i]) { return {out, error::Adhoc("stop must not exceed dim")}; } } @@ -189,12 +155,16 @@ class GridLayout { return tmp.first; } - int rank() const noexcept { return rank_; } - const int* dim() const noexcept { return dim_; } + int rank() const noexcept { return idx_mapping_.rank(); } + const int* dim() const noexcept { return idx_mapping_.extents(); } const int* start() const noexcept { return start_; } const int* stop() const noexcept { return stop_; } const int* end() const noexcept { return end_; } + const IdxMapping& idx_mapping() const noexcept { + return idx_mapping_; + } + friend bool operator==(const GridLayout& lhs, const GridLayout& rhs) noexcept; friend bool operator!=(const GridLayout& lhs, const GridLayout& rhs) noexcept { @@ -202,11 +172,15 @@ class GridLayout { } int n_elements(bool exclude_inactive = false) const noexcept { - int total = 1; - for (int i = 0; i < rank_; i++) { - total *= (exclude_inactive) ? stop_[i] - start_[i] : dim_[i]; + if (exclude_inactive) { + int total = 1; + int rank = this->rank(); + for (int i = 0; i < rank; i++) { + total *= stop_[i] - start_[i]; + } + return total; } - return total; + return idx_mapping_.n_elements(); } // teach googletest how to print this type diff --git a/tests/grtestutils/view.hpp b/tests/grtestutils/view.hpp index b15ce8819..6d5072e32 100644 --- a/tests/grtestutils/view.hpp +++ b/tests/grtestutils/view.hpp @@ -18,7 +18,6 @@ #include #include // std::size_t -#include #include #include #include // std::pair @@ -93,6 +92,11 @@ struct IdxMapping { /// max rank value static constexpr int MAX_RANK = 3; + // make it possible for GridLayout to use the default constructor + // (this is useful when GridLayout's factor method reports a failure of + // some kind) + friend class GridLayout; + private: // attributes: int rank_; ///< the number of dimensions @@ -170,6 +174,15 @@ struct IdxMapping { /// access the extents pointer (the length is given by @ref rank) const int* extents() const noexcept { return extents_; } + /// provides access to the underlying extents pointer + /// + /// @warning + /// This is unsafe because the user can mutate entries in the returned + /// pointer. If that happens, then undefined behavior may emerge + /// + /// This **ONLY** exists so that @ref grackle_field_data can wrap this type + int* unsafe_extents() noexcept { return extents_; } + /// construct an equivalent 3d IdxMapping IdxMapping to_3d_mapping() const noexcept { int out_rank = 3; @@ -266,6 +279,21 @@ struct IdxMapping { } } + /// overload the equality operator + friend bool operator==(const IdxMapping& lhs, + const IdxMapping& rhs) noexcept { + // (this could be a little more efficient) + if (lhs.rank_ != rhs.rank_) { + return false; + } + for (int i = 0; i < lhs.rank_; i++) { + if (lhs.extents_[i] != rhs.extents_[i]) { + return false; + } + } + return true; + } + // teach googletest how to print this type friend void PrintTo(const IdxMapping& mapping, std::ostream* os) { const char* layout = From b7aaa6aae6ed4b55abf27473a8f6d3f790fb61f6 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 28 Feb 2026 13:47:33 -0500 Subject: [PATCH 27/29] remove a duplicated set of functions --- tests/grtestutils/utils.cpp | 6 ----- tests/grtestutils/utils.hpp | 54 ------------------------------------- 2 files changed, 60 deletions(-) diff --git a/tests/grtestutils/utils.cpp b/tests/grtestutils/utils.cpp index 2efd03a56..3665911f4 100644 --- a/tests/grtestutils/utils.cpp +++ b/tests/grtestutils/utils.cpp @@ -49,10 +49,4 @@ std::optional grtest::get_standard_datafile(const char* datafile) { return std::nullopt; } return std::optional(tmp); -} - -std::string grtest::to_pretty_string(double val) { - char buf[30]; - snprintf(buf, 30, "%g", val); - return std::string(buf); } \ No newline at end of file diff --git a/tests/grtestutils/utils.hpp b/tests/grtestutils/utils.hpp index dae9d1623..690d54e6a 100644 --- a/tests/grtestutils/utils.hpp +++ b/tests/grtestutils/utils.hpp @@ -55,58 +55,4 @@ inline double uniform_dist_transform(std::minstd_rand &generator) { } // namespace grtest::random - -namespace grtest { - -/// equivalent of %g (the value should be fully specified) -/// -/// The current implementation is extremely crude! -std::string to_pretty_string(double val); - -inline std::string to_pretty_string(float val) { - // this is crude! - return to_pretty_string(static_cast(val)); -} - -/// formats a std::vector as a string -/// -/// @note -/// This is highly inefficient, partially because it consists of code written -/// from before we adopted googletest -template -std::string ptr_to_string(const T* ptr, std::size_t len) { - static_assert(std::is_floating_point_v); - - std::string out = "{"; - - std::size_t pause_start; - std::size_t pause_stop; - - if (len > 30){ - pause_start = 3; - pause_stop = len - 3; - } else { - pause_start = len *2; - pause_stop = pause_start; - } - - for (std::size_t i = 0; i < len; i++) { - if ((i > pause_start) && (i < pause_stop)) { continue; } - - if (i == pause_stop) { - out += ", ... "; - } else if (i != 0) { - out += ", "; - } - - const int BUF_SIZE = 30; - char buf[BUF_SIZE]; - snprintf(buf, BUF_SIZE, "%g", ptr[i]); - out += buf; - } - return out + "}"; -} - -} // namespace grtest - #endif /* GRTEST_UTILS_HPP */ From e85b00b1aff104766d4b88451e4f195b8bd4552c Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Sat, 28 Feb 2026 14:51:40 -0500 Subject: [PATCH 28/29] fixing a minor bug --- .../googletest/check_allclose_detail.hpp | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/tests/grtestutils/googletest/check_allclose_detail.hpp b/tests/grtestutils/googletest/check_allclose_detail.hpp index a0dd264d6..cdeff25b4 100644 --- a/tests/grtestutils/googletest/check_allclose_detail.hpp +++ b/tests/grtestutils/googletest/check_allclose_detail.hpp @@ -24,6 +24,14 @@ namespace grtest::arraycmp_detail { +/// the goal is to avoid short-circuiting +template +[[gnu::always_inline]] inline bool both_nan_(T actual, T desired) { + bool actual_isnan = std::isnan(actual); + bool desired_isnan = std::isnan(desired); + return actual_isnan & desired_isnan; // use & to avoid &&'s short-circuiting +} + template [[gnu::always_inline]] inline bool isclose_(T actual, T desired, double rtol, double atol) { @@ -34,9 +42,7 @@ template bool isclose = abs_diff <= atol + rtol * std::fabs(desired); if constexpr (EqualNan) { - bool actual_isnan = std::isnan(actual); - bool desired_isnan = std::isnan(desired); - return (actual_isnan && desired_isnan) || isclose; + return both_nan_(actual, desired) || isclose; } else { return isclose; } @@ -88,12 +94,12 @@ class FltIsClose { struct FltIsEqual { /// determines whether arguments are exactly equal bool operator()(float actual, float desired) const noexcept { - return (actual == desired) || std::isnan(actual) == std::isnan(desired); + return (actual == desired) || both_nan_(actual, desired); } /// determines whether arguments are exactly equal bool operator()(double actual, double desired) const noexcept { - return (actual == desired) || std::isnan(actual) == std::isnan(desired); + return (actual == desired) || both_nan_(actual, desired); } /// describe relationship between values for which this functor returns false From c4f87f0afaee0ac22ed27ba9d1b31f361f67dbb9 Mon Sep 17 00:00:00 2001 From: Matthew Abruzzo Date: Wed, 4 Mar 2026 18:24:10 -0500 Subject: [PATCH 29/29] minor bugfix --- tests/grtestutils/harness/field_container.hpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/grtestutils/harness/field_container.hpp b/tests/grtestutils/harness/field_container.hpp index d3f403864..15e6ea1b7 100644 --- a/tests/grtestutils/harness/field_container.hpp +++ b/tests/grtestutils/harness/field_container.hpp @@ -84,10 +84,15 @@ class GridLayout { return {out, idx_mapping_rslt.second}; } out.idx_mapping_ = idx_mapping_rslt.first; + const int* extents = out.idx_mapping_.extents(); for (int i = 0; i < 3; i++) { out.start_[i] = (start != nullptr && i < rank) ? start[i] : 0; - out.stop_[i] = (stop != nullptr && i < rank) ? stop[i] : 1; + if (i < rank) { + out.stop_[i] = (stop != nullptr) ? stop[i] : extents[i]; + } else { + out.stop_[i] = 1; + } out.end_[i] = out.stop_[i] - 1; if (out.start_[i] < 0) {