From 25229bb3b21230d7e9af66486fc74302f6c3c07c Mon Sep 17 00:00:00 2001 From: Matthew Carroll <28577806+MJC598@users.noreply.github.com> Date: Tue, 22 Sep 2026 10:01:46 -0400 Subject: [PATCH 01/11] adding mermaid docs for dependency inversion and ioc --- docs/src/dependency_inversion.mmd | 44 +++++++++++++++++++++++++++++++ docs/src/ioc.mmd | 39 +++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) create mode 100644 docs/src/dependency_inversion.mmd create mode 100644 docs/src/ioc.mmd diff --git a/docs/src/dependency_inversion.mmd b/docs/src/dependency_inversion.mmd new file mode 100644 index 00000000..0431e933 --- /dev/null +++ b/docs/src/dependency_inversion.mmd @@ -0,0 +1,44 @@ +--- +config: + theme: 'base' + themeVariables: + primaryColor: '#3d9be9' + secondaryColor: '#003771' + tertiaryColor: '#ffffff' + lineColor: '#f0325f' + primaryTextColor: '#ffffff' + primaryBorderColor: '#f0325f' + titleColor: '#003771' + fontFamily: 'Helvetica Bold, Helvetica' + fontSize: '22px' +--- + +flowchart TB + subgraph Layered["Layered architecture / dependency inversion"] + direction TB + subgraph Application["Application layer"] + direction LR + S[Simulation] + AM[Model] + end + + subgraph Abstraction["Abstraction layer"] + direction LR + T["Transition interface"] + TS[Timestep] + end + + subgraph Implementation["Implementation layer"] + direction TB + MIG[Migration] + BEH[Behavior] + INT[Intervention] + ODO[Overdose] + BGD[Background Mortality] + end + + S --> AM + AM --> TS + TS -->|depends on| T + T <-.-|implements| Implementation + end diff --git a/docs/src/ioc.mmd b/docs/src/ioc.mmd new file mode 100644 index 00000000..40b471b6 --- /dev/null +++ b/docs/src/ioc.mmd @@ -0,0 +1,39 @@ +--- +config: + theme: 'base' + themeVariables: + primaryColor: '#3d9be9' + secondaryColor: '#003771' + tertiaryColor: '#ffffff' + lineColor: '#f0325f' + primaryTextColor: '#ffffff' + primaryBorderColor: '#f0325f' + titleColor: '#003771' + fontFamily: 'Helvetica Bold, Helvetica' + fontSize: '22px' +--- + +flowchart LR + subgraph Traditional["RESPONDv1 control flow"] + direction LR + R1[RESPOND] + R1 <-->|gets| D1[Data] + R1 <-->|runs| C1[Migration] + R1 <-->|runs| C2[Behaviors] + R1 <-->|runs| C3[Intervention] + R1 <-->|runs| C4[Overdoses] + R1 <-->|runs| C5[Fatal Overdoses] + R1 <-->|runs| C6[Background Mortality] + end + + subgraph Inverted["RESPONDv2 control flow"] + direction TB + D[Data] + D -->|injected into| M[Model] + I["«interface»
Transition::Execute()"] + M -->|calls| I + I -.->|chosen at runtime| T1[Migration
Behaviors
Intervention
Overdoses
Fatal Overdoses
Background Mortality] + end + + Traditional --- Inverted + From 3cfcddabc50d9cc560f08798f2a79d7c94a04c5b Mon Sep 17 00:00:00 2001 From: Matthew Carroll <28577806+MJC598@users.noreply.github.com> Date: Tue, 22 Sep 2026 12:04:26 -0400 Subject: [PATCH 02/11] Adding updates for execution config and logging config to improve constructor overloading --- include/respond/execution_config.hpp | 28 ++++++++ include/respond/logging.hpp | 7 ++ include/respond/logging_config.hpp | 28 ++++++++ include/respond/model.hpp | 19 ++++++ include/respond/respond.hpp | 3 + include/respond/runtime_config.hpp | 23 +++++++ include/respond/simulation.hpp | 96 +++++++++++++++++++++------- src/internals/logging_internals.hpp | 10 +-- src/internals/markov.hpp | 45 ++++++++++--- src/logging.cpp | 31 ++++++--- src/model_factory.cpp | 12 ++++ tests/unit/logging_test.cpp | 14 ++++ tests/unit/markov_test.cpp | 8 +++ tests/unit/simulation_test.cpp | 49 ++++++++++++++ 14 files changed, 326 insertions(+), 47 deletions(-) create mode 100644 include/respond/execution_config.hpp create mode 100644 include/respond/logging_config.hpp create mode 100644 include/respond/runtime_config.hpp diff --git a/include/respond/execution_config.hpp b/include/respond/execution_config.hpp new file mode 100644 index 00000000..53ff8d70 --- /dev/null +++ b/include/respond/execution_config.hpp @@ -0,0 +1,28 @@ +//////////////////////////////////////////////////////////////////////////////// +// File: execution_config.hpp // +// Project: respond // +// Created Date: 2026-09-22 // +// Author: Matthew Carroll // +// ----- // +// Copyright (c) 2026 Syndemics Lab at Boston Medical Center // +//////////////////////////////////////////////////////////////////////////////// +#ifndef RESPOND_EXECUTION_CONFIG_HPP_ +#define RESPOND_EXECUTION_CONFIG_HPP_ + +namespace respond { +/// @brief Controls how a Simulation distributes execution resources. +struct ExecutionConfig { + /// @brief Total worker threads available for concurrent model execution. + /// A value of 0 selects the implementation's automatic default. + unsigned int total_threads = 0; + + /// @brief Eigen worker threads used within a model. + /// Use 1 when models are executed concurrently to avoid oversubscription. + unsigned int eigen_threads = 1; + + /// @brief Whether models may be executed concurrently by the simulation. + bool run_models_concurrently = false; +}; +} // namespace respond + +#endif // RESPOND_EXECUTION_CONFIG_HPP_ diff --git a/include/respond/logging.hpp b/include/respond/logging.hpp index 58878527..67ae6990 100644 --- a/include/respond/logging.hpp +++ b/include/respond/logging.hpp @@ -14,6 +14,8 @@ #include +#include + namespace respond { /// @brief Logging levels for the logger. @@ -56,6 +58,11 @@ enum class LogPattern : int { CreationStatus CreateFileLogger(const std::string &logger_name, const std::string &filepath); +/// @brief Initializes a logger from a logging configuration. +/// @param config Logging name, destination, and shared-sink policy. +/// @return CreationStatus indicating the result of logger creation. +CreationStatus ConfigureLogger(const LoggingConfig &config); + // ============================================================================ // Parallel Execution Support: Shared File Sink // ============================================================================ diff --git a/include/respond/logging_config.hpp b/include/respond/logging_config.hpp new file mode 100644 index 00000000..c14ff7c9 --- /dev/null +++ b/include/respond/logging_config.hpp @@ -0,0 +1,28 @@ +//////////////////////////////////////////////////////////////////////////////// +// File: logging_config.hpp // +// Project: respond // +// Created Date: 2026-09-22 // +// Author: Matthew Carroll // +// ----- // +// Last Modified: 2026-09-22 // +// Modified By: Matthew Carroll // +// ----- // +// Copyright (c) 2026 Syndemics Lab at Boston Medical Center // +//////////////////////////////////////////////////////////////////////////////// +#ifndef RESPOND_LOGGING_CONFIG_HPP_ +#define RESPOND_LOGGING_CONFIG_HPP_ + +#include + +#include + +namespace respond { +/// @brief Describes the logging destination used by a library object. +struct LoggingConfig { + std::string logger_name = RESPOND_DEFAULT_LOG; + std::string file_path = RESPOND_DEFAULT_LOG_FILE; + bool use_shared_sink = false; +}; +} // namespace respond + +#endif // RESPOND_LOGGING_CONFIG_HPP_ diff --git a/include/respond/model.hpp b/include/respond/model.hpp index 1376a154..b44fb473 100644 --- a/include/respond/model.hpp +++ b/include/respond/model.hpp @@ -20,6 +20,7 @@ #include #include +#include #include namespace respond { @@ -44,6 +45,7 @@ class Model { /// @param log_name Name of the logger for this model (default: "console"). /// @param log_filepath File path for the log file (default: "respond.log"). /// @return A unique_ptr to the newly created Model instance. + [[deprecated("Use Model::Create(name, RuntimeConfig) instead")]] static std::unique_ptr Create(const std::string &name, const std::string &log_name = RESPOND_DEFAULT_LOG, @@ -60,11 +62,28 @@ class Model { /// @param log_name Name of the logger for this model (default: "console"). /// @param log_filepath File path for the log file (default: "respond.log"). /// @return A unique_ptr to the newly created Model instance. + [[deprecated("Use Model::Create(name, RuntimeConfig) instead")]] static std::unique_ptr Create(const std::string &name, const unsigned int processor_count, const std::string &log_name = RESPOND_DEFAULT_LOG, const std::string &log_filepath = RESPOND_DEFAULT_LOG_FILE); + /// @brief Creates a Model with explicit execution settings. + /// @param name The name identifier for the model to create. + /// @param execution_config Resource settings for model execution. + /// @param log_name Name of the logger for this model (default: "console"). + /// @param log_filepath File path for the log file (default: "respond.log"). + /// @return A unique_ptr to the newly created Model instance. + [[deprecated("Use Model::Create(name, RuntimeConfig) instead")]] + static std::unique_ptr + Create(const std::string &name, const ExecutionConfig &execution_config, + const std::string &log_name = RESPOND_DEFAULT_LOG, + const std::string &log_filepath = RESPOND_DEFAULT_LOG_FILE); + + /// @brief Creates a Model with shared runtime settings. + static std::unique_ptr + Create(const std::string &name, const RuntimeConfig &runtime_config); + /// @brief Virtual destructor for proper polymorphic cleanup. virtual ~Model() = default; diff --git a/include/respond/respond.hpp b/include/respond/respond.hpp index e0b20aad..98fb9487 100644 --- a/include/respond/respond.hpp +++ b/include/respond/respond.hpp @@ -13,9 +13,12 @@ #define RESPOND_RESPOND_HPP_ #include +#include #include #include +#include #include +#include #include #include #include diff --git a/include/respond/runtime_config.hpp b/include/respond/runtime_config.hpp new file mode 100644 index 00000000..26bafc4f --- /dev/null +++ b/include/respond/runtime_config.hpp @@ -0,0 +1,23 @@ +//////////////////////////////////////////////////////////////////////////////// +// File: runtime_config.hpp // +// Project: respond // +// Created Date: 2026-09-22 // +// Author: Matthew Carroll // +// ----- // +// Copyright (c) 2026 Syndemics Lab at Boston Medical Center // +//////////////////////////////////////////////////////////////////////////////// +#ifndef RESPOND_RUNTIME_CONFIG_HPP_ +#define RESPOND_RUNTIME_CONFIG_HPP_ + +#include +#include + +namespace respond { +/// @brief Groups execution and logging settings shared by runtime objects. +struct RuntimeConfig { + ExecutionConfig execution; + LoggingConfig logging; +}; +} // namespace respond + +#endif // RESPOND_RUNTIME_CONFIG_HPP_ diff --git a/include/respond/simulation.hpp b/include/respond/simulation.hpp index bd0d0eaf..8eb22415 100644 --- a/include/respond/simulation.hpp +++ b/include/respond/simulation.hpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-05 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-22 // +// Last Modified: 2026-09-22 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // @@ -16,6 +16,7 @@ #include #include #include +#include #include #include @@ -63,7 +64,7 @@ class Simulation { /// @throws std::invalid_argument if model is nullptr. ModelSlotProxy &operator=(const std::unique_ptr &model) { if (!model) { - LogError(_owner->_log_name, + LogError(_owner->_runtime_config.logging.logger_name, "Cannot assign null model pointer to simulation " "slot."); throw std::invalid_argument( @@ -87,19 +88,40 @@ class Simulation { /// @brief Default constructor for a Simulation instance. /// Initializes the simulation with the default logger. - Simulation() : Simulation(RESPOND_DEFAULT_LOG) {} + Simulation() : Simulation(RuntimeConfig{}) {} /// @brief Constructs a Simulation with a specified logger. /// @param log_name The name of the logger to use for simulation output. + [[deprecated("Use Simulation(RuntimeConfig) instead")]] Simulation(const std::string &log_name) - : Simulation(log_name, RESPOND_DEFAULT_LOG_FILE) {} + : Simulation(RuntimeConfig{ + ExecutionConfig{}, + LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, false}}) {} /// @brief Constructs a Simulation with a specified logger and log file. /// @param log_name The name of the logger to use for simulation output. /// @param log_filepath The file path for the logger output. + [[deprecated("Use Simulation(RuntimeConfig) instead")]] Simulation(const std::string &log_name, const std::string &log_filepath) - : _log_name(log_name) { - CreateFileLogger(log_name, log_filepath); + : Simulation( + RuntimeConfig{ExecutionConfig{}, + LoggingConfig{log_name, log_filepath, false}}) {} + + /// @brief Constructs a Simulation with logger and execution settings. + /// @param log_name The name of the logger to use for simulation output. + /// @param log_filepath The file path for the logger output. + /// @param execution_config Resource settings for simulation execution. + [[deprecated("Use Simulation(RuntimeConfig) instead")]] + Simulation(const std::string &log_name, const std::string &log_filepath, + const ExecutionConfig &execution_config) + : Simulation(RuntimeConfig{ + execution_config, LoggingConfig{log_name, log_filepath, false}}) { + } + + /// @brief Constructs a Simulation with shared runtime settings. + explicit Simulation(const RuntimeConfig &runtime_config) + : _runtime_config(runtime_config) { + ConfigureLogger(_runtime_config.logging); } /// @brief Virtual destructor for polymorphic cleanup. @@ -110,7 +132,7 @@ class Simulation { /// affect the original. /// @param other The Simulation instance to copy from. Simulation(const Simulation &other) { - _log_name = other._log_name; + _runtime_config = other._runtime_config; for (const auto &m : other._models) { _models.push_back(m->clone()); } @@ -128,7 +150,7 @@ class Simulation { /// @return Reference to this simulation after assignment. Simulation &operator=(const Simulation &other) { if (this != &other) { - _log_name = other._log_name; + _runtime_config = other._runtime_config; _models.clear(); for (const auto &m : other._models) { _models.push_back(m->clone()); @@ -147,7 +169,8 @@ class Simulation { /// @brief Move constructor for transferring simulation ownership. /// @param other The simulation to move from. Simulation(Simulation &&other) noexcept - : _log_name(std::move(other._log_name)), _duration(other._duration), + : _runtime_config(std::move(other._runtime_config)), + _duration(other._duration), _parameter_change_times(std::move(other._parameter_change_times)), _stratify_entering_cohort(other._stratify_entering_cohort), _build_summary_stats(other._build_summary_stats), @@ -165,7 +188,7 @@ class Simulation { /// @return Reference to this simulation after assignment. Simulation &operator=(Simulation &&other) noexcept { if (this != &other) { - _log_name = std::move(other._log_name); + _runtime_config = std::move(other._runtime_config); _duration = other._duration; _parameter_change_times = std::move(other._parameter_change_times); _stratify_entering_cohort = other._stratify_entering_cohort; @@ -194,7 +217,7 @@ class Simulation { /// @return A deep-copied model instance representing the newly created /// model. std::unique_ptr CreateNewModel(const std::string &model_name) { - _models.push_back(Model::Create(model_name, _log_name)); + _models.push_back(Model::Create(model_name, _runtime_config)); return _models.back()->clone(); } @@ -214,8 +237,9 @@ class Simulation { if (duration > 0) { _duration = duration; } - LogInfo(_log_name, "Running simulation for duration of " + - std::to_string(_duration) + " timesteps."); + LogInfo(_runtime_config.logging.logger_name, + "Running simulation for duration of " + + std::to_string(_duration) + " timesteps."); for (const auto &model : _models) { model->SetFinalTimestep(_duration); model->RunTimesteps(); @@ -279,11 +303,12 @@ class Simulation { /// @throws std::out_of_range if no models exist or idx is out of range. std::unique_ptr GetModel(int idx) const { if (_models.empty()) { - LogError(_log_name, "No models available in GetModel."); + LogError(_runtime_config.logging.logger_name, + "No models available in GetModel."); throw std::out_of_range("Error attempting to GetModel: no models."); } if (idx < -1 || idx >= static_cast(_models.size())) { - LogError(_log_name, + LogError(_runtime_config.logging.logger_name, "Index out of range in GetModel: " + std::to_string(idx)); throw std::out_of_range("Error attempting to GetModel by index."); } @@ -310,8 +335,9 @@ class Simulation { /// @return Map of history names to history records for the selected model. const std::map &GetModelHistory(size_t idx) const { if (idx >= _models.size()) { - LogError(_log_name, "Index out of range in GetModelHistory: " + - std::to_string(idx)); + LogError(_runtime_config.logging.logger_name, + "Index out of range in GetModelHistory: " + + std::to_string(idx)); throw std::out_of_range( "Error attempting to GetModelHistory by index."); } @@ -323,8 +349,9 @@ class Simulation { /// @return Vector of history names for the selected model. const std::vector GetModelHistoryNames(size_t idx) const { if (idx >= _models.size()) { - LogError(_log_name, "Index out of range in GetModelHistoryNames: " + - std::to_string(idx)); + LogError(_runtime_config.logging.logger_name, + "Index out of range in GetModelHistoryNames: " + + std::to_string(idx)); throw std::out_of_range( "Error attempting to GetModelHistoryNames by index."); } @@ -337,11 +364,31 @@ class Simulation { void SetDuration(int duration) { _duration = duration; } + /// @brief Retrieves the simulation execution settings. + /// @return The current execution configuration. + const ExecutionConfig &GetExecutionConfig() const { + return _runtime_config.execution; + } + + /// @brief Sets the simulation execution settings. + /// @param execution_config Resource settings for simulation execution. + void SetExecutionConfig(const ExecutionConfig &execution_config) { + _runtime_config.execution = execution_config; + } + + const RuntimeConfig &GetRuntimeConfig() const { return _runtime_config; } + + void SetRuntimeConfig(const RuntimeConfig &runtime_config) { + _runtime_config = runtime_config; + ConfigureLogger(_runtime_config.logging); + } + private: Model &GetModelRefOrThrow(size_t idx) { if (idx >= _models.size()) { - LogError(_log_name, "Index out of range in model access: " + - std::to_string(idx)); + LogError(_runtime_config.logging.logger_name, + "Index out of range in model access: " + + std::to_string(idx)); throw std::out_of_range("Error attempting to access model by " "index."); } @@ -350,15 +397,16 @@ class Simulation { const Model &GetModelRefOrThrow(size_t idx) const { if (idx >= _models.size()) { - LogError(_log_name, "Index out of range in model access: " + - std::to_string(idx)); + LogError(_runtime_config.logging.logger_name, + "Index out of range in model access: " + + std::to_string(idx)); throw std::out_of_range("Error attempting to access model by " "index."); } return *_models[idx]; } - std::string _log_name; + RuntimeConfig _runtime_config; std::vector> _models; int _duration = 1; // Default simulation duration in timesteps diff --git a/src/internals/logging_internals.hpp b/src/internals/logging_internals.hpp index db03a828..db0a88ca 100644 --- a/src/internals/logging_internals.hpp +++ b/src/internals/logging_internals.hpp @@ -28,10 +28,10 @@ namespace respond { -class LoggingConfig { +class LoggingRegistry { public: - static LoggingConfig &GetInstance() { - static LoggingConfig instance; + static LoggingRegistry &GetInstance() { + static LoggingRegistry instance; return instance; } @@ -92,7 +92,7 @@ class LoggingConfig { } private: - LoggingConfig() + LoggingRegistry() : current_pattern_(LogPattern::kStandard), flush_interval_(3), default_sink_path_("respond.log") { spdlog::cfg::load_env_levels(); @@ -142,7 +142,7 @@ void log(const std::string &logger_name, const std::string &message, logger->info(message); break; } - if (LoggingConfig::GetFlushInterval() == 0) { + if (LoggingRegistry::GetFlushInterval() == 0) { logger->flush(); } } else { diff --git a/src/internals/markov.hpp b/src/internals/markov.hpp index ad5f1781..fee6429f 100644 --- a/src/internals/markov.hpp +++ b/src/internals/markov.hpp @@ -4,8 +4,8 @@ // Created Date: 2026-02-05 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-09-18 // -// Modified By: Dimitri Baptiste // +// Last Modified: 2026-09-22 // +// Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // //////////////////////////////////////////////////////////////////////////////// @@ -14,6 +14,7 @@ #include +#include #include #include #include @@ -23,6 +24,7 @@ #include #include +#include #include namespace respond { @@ -35,11 +37,12 @@ class Markov : public virtual Model { //////////////////////////////////////////////////////////////////////////// /// @brief Default constructor for Markov model. Initializes with default.. - Markov() : Markov("markov", RESPOND_DEFAULT_LOG) {} + Markov() : Markov("markov", RuntimeConfig{}) {} /// @brief Constructs a Markov model with specified name and logger. /// @param name The identifier for this model. /// @param log_name The logger name for error reporting. + [[deprecated("Use Markov(name, RuntimeConfig) instead")]] Markov(const std::string &name, const std::string &log_name) : Markov(name, log_name, RESPOND_DEFAULT_LOG_FILE) {} @@ -51,21 +54,42 @@ class Markov : public virtual Model { /// model. /// @param processor_count The number of threads to use when running this /// model. + [[deprecated("Use Markov(name, RuntimeConfig) instead")]] Markov(const std::string &name, const std::string &log_name, const std::string &log_filepath, const unsigned int processor_count = std::thread::hardware_concurrency()) - : _name(name), _log_name(log_name), _current_timestep(0), + : Markov(name, log_name, log_filepath, + ExecutionConfig{0, processor_count, false}) {} + + Markov(const std::string &name, const RuntimeConfig &runtime_config) + : _name(name), _log_name(runtime_config.logging.logger_name), + _runtime_config(runtime_config), _current_timestep(0), _history_capture_interval(1), _final_timestep(-1), _initial_history_recorded(false) { - CreateFileLogger(log_name, log_filepath); - // ensure that the number of threads cannot exceed the hardware capacity + ConfigureLogger(_runtime_config.logging); const unsigned int thread_limit = std::thread::hardware_concurrency(); const unsigned int threads = - processor_count > thread_limit ? thread_limit : processor_count; + thread_limit == 0 + ? _runtime_config.execution.eigen_threads + : std::min(_runtime_config.execution.eigen_threads, + thread_limit); Eigen::setNbThreads(threads); } + /// @brief Constructs a Markov model with explicit execution settings. + /// @param name The identifier for this model. + /// @param log_name The logger name for error reporting. + /// @param log_filepath The file path for the log file used by this model. + /// @param execution_config Resource settings for model execution. + [[deprecated("Use Markov(name, RuntimeConfig) instead")]] + Markov(const std::string &name, const std::string &log_name, + const std::string &log_filepath, + const ExecutionConfig &execution_config) + : Markov(name, RuntimeConfig{execution_config, + LoggingConfig{log_name, log_filepath, + false}}) {} + /// @brief Destructor for Markov model. Default implementation. ~Markov() = default; @@ -73,6 +97,7 @@ class Markov : public virtual Model { _state = other._state; _name = other._name; _log_name = other._log_name; + _runtime_config = other._runtime_config; _current_timestep = other._current_timestep; _history_capture_interval = other._history_capture_interval; _final_timestep = other._final_timestep; @@ -91,6 +116,7 @@ class Markov : public virtual Model { _state = other._state; _name = other._name; _log_name = other._log_name; + _runtime_config = other._runtime_config; _current_timestep = other._current_timestep; _history_capture_interval = other._history_capture_interval; _final_timestep = other._final_timestep; @@ -113,7 +139,7 @@ class Markov : public virtual Model { /// @return A unique_ptr to a new Markov instance that is a deep copy of /// this instance. std::unique_ptr clone() const override { - auto np = Model::Create(_name, _log_name); + auto np = Model::Create(_name, _runtime_config); np->SetState(GetState()); np->SetHistoryCaptureInterval(GetHistoryCaptureInterval()); np->SetFinalTimestep(GetFinalTimestep()); @@ -232,7 +258,7 @@ class Markov : public virtual Model { RecordHistoryAtCurrentTimestep(); } size_t duration = _timestep_vector.size(); - if (_timestep_vector.size() > static_cast(_final_timestep) && + if (duration > static_cast(_final_timestep) && _final_timestep >= 0) { std::string warning_msg = "Duration is less than available timesteps for model: " + @@ -313,6 +339,7 @@ class Markov : public virtual Model { Eigen::VectorXd _state; std::string _name; std::string _log_name; + RuntimeConfig _runtime_config; std::map _histories; int _current_timestep; int _history_capture_interval; diff --git a/src/logging.cpp b/src/logging.cpp index 74fcd724..586063ca 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -26,7 +26,7 @@ CreationStatus CreateFileLogger(const std::string &logger_name, try { spdlog::cfg::load_env_levels(); std::string pattern = - LoggingConfig::GetPatternString(LoggingConfig::GetPattern()); + LoggingRegistry::GetPatternString(LoggingRegistry::GetPattern()); spdlog::set_pattern(pattern); spdlog::basic_logger_mt(logger_name, filepath); } catch (const spdlog::spdlog_ex &ex) { @@ -42,9 +42,9 @@ CreationStatus CreateFileLogger(const std::string &logger_name, CreationStatus CreateSharedFileSink(const std::string &filepath) { try { - auto sink = LoggingConfig::GetSharedSink(filepath); + auto sink = LoggingRegistry::GetSharedSink(filepath); if (sink) { - LoggingConfig::SetDefaultSinkPath(filepath); + LoggingRegistry::SetDefaultSinkPath(filepath); return CreationStatus::kSuccess; } std::string error_msg = @@ -67,8 +67,8 @@ CreationStatus CreateSharedLogger(const std::string &logger_name) { } try { - std::string filepath = LoggingConfig::GetDefaultSinkPath(); - auto sink = LoggingConfig::GetSharedSink(filepath); + std::string filepath = LoggingRegistry::GetDefaultSinkPath(); + auto sink = LoggingRegistry::GetSharedSink(filepath); if (!sink) { std::string error_msg = "Failed to create shared logger '" + logger_name + @@ -79,7 +79,7 @@ CreationStatus CreateSharedLogger(const std::string &logger_name) { spdlog::cfg::load_env_levels(); std::string pattern = - LoggingConfig::GetPatternString(LoggingConfig::GetPattern()); + LoggingRegistry::GetPatternString(LoggingRegistry::GetPattern()); auto logger = std::make_shared(logger_name, sink); logger->set_pattern(pattern); @@ -96,11 +96,24 @@ CreationStatus CreateSharedLogger(const std::string &logger_name) { } } -void SetLogPattern(LogPattern pattern) { LoggingConfig::SetPattern(pattern); } +CreationStatus ConfigureLogger(const LoggingConfig &config) { + if (config.use_shared_sink) { + const auto sink_status = CreateSharedFileSink(config.file_path); + if (sink_status == CreationStatus::kError) { + return sink_status; + } + return CreateSharedLogger(config.logger_name); + } + return CreateFileLogger(config.logger_name, config.file_path); +} -LogPattern GetLogPattern() { return LoggingConfig::GetPattern(); } +void SetLogPattern(LogPattern pattern) { LoggingRegistry::SetPattern(pattern); } -void SetFlushInterval(int seconds) { LoggingConfig::SetFlushInterval(seconds); } +LogPattern GetLogPattern() { return LoggingRegistry::GetPattern(); } + +void SetFlushInterval(int seconds) { + LoggingRegistry::SetFlushInterval(seconds); +} void FlushAllLoggers() { spdlog::apply_all( diff --git a/src/model_factory.cpp b/src/model_factory.cpp index 9d882f7b..1d9869ae 100644 --- a/src/model_factory.cpp +++ b/src/model_factory.cpp @@ -33,4 +33,16 @@ std::unique_ptr Model::Create(const std::string &name, return std::make_unique(name, log_name, log_filepath, processor_count); } + +std::unique_ptr Model::Create( + const std::string &name, const ExecutionConfig &execution_config, + const std::string &log_name, const std::string &log_filepath) { + return std::make_unique(name, log_name, log_filepath, + execution_config); +} + +std::unique_ptr Model::Create(const std::string &name, + const RuntimeConfig &runtime_config) { + return std::make_unique(name, runtime_config); +} } // namespace respond diff --git a/tests/unit/logging_test.cpp b/tests/unit/logging_test.cpp index 7a172219..c3b40de5 100644 --- a/tests/unit/logging_test.cpp +++ b/tests/unit/logging_test.cpp @@ -120,6 +120,13 @@ TEST_F(LoggingTest, CreateMultipleFileLoggers) { EXPECT_NE(spdlog::get("logger2"), nullptr); } +TEST_F(LoggingTest, ConfigureLoggerCreatesFileLogger) { + LoggingConfig config{"configured_logger", test_log_file_, false}; + + EXPECT_EQ(ConfigureLogger(config), CreationStatus::kSuccess); + EXPECT_NE(spdlog::get(config.logger_name), nullptr); +} + // ============================================================================ // Test: Shared File Sink Functionality // ============================================================================ @@ -129,6 +136,13 @@ TEST_F(LoggingTest, CreateSharedFileSink) { EXPECT_EQ(status, CreationStatus::kSuccess); } +TEST_F(LoggingTest, ConfigureLoggerCreatesSharedLogger) { + LoggingConfig config{"configured_shared_logger", shared_log_file_, true}; + + EXPECT_EQ(ConfigureLogger(config), CreationStatus::kSuccess); + EXPECT_NE(spdlog::get(config.logger_name), nullptr); +} + TEST_F(LoggingTest, CreateSharedFileSinkCaching) { // Create sink first time CreationStatus status1 = CreateSharedFileSink(shared_log_file_); diff --git a/tests/unit/markov_test.cpp b/tests/unit/markov_test.cpp index 5e754b97..829deee5 100644 --- a/tests/unit/markov_test.cpp +++ b/tests/unit/markov_test.cpp @@ -100,6 +100,14 @@ TEST_F(MarkovTest, CreateMarkovModelProcessorCount) { CreationStatus::kExists); } +TEST_F(MarkovTest, CreateMarkovModelExecutionConfig) { + ExecutionConfig config; + config.eigen_threads = 2; + + auto markov = Model::Create("markov", config); + ASSERT_NE(markov, nullptr); +} + TEST_F(MarkovTest, MoveConstructor) { Markov markov("markov_source", RESPOND_DEFAULT_LOG); markov.SetState(state); diff --git a/tests/unit/simulation_test.cpp b/tests/unit/simulation_test.cpp index 21b26d8e..578a0711 100644 --- a/tests/unit/simulation_test.cpp +++ b/tests/unit/simulation_test.cpp @@ -94,6 +94,55 @@ TEST_F(SimulationTest, ConstructorWithLogNameAndLogFile) { CreationStatus::kExists); } +TEST_F(SimulationTest, StoresExecutionConfig) { + ExecutionConfig config; + config.total_threads = 4; + config.eigen_threads = 1; + config.run_models_concurrently = true; + + Simulation s("custom_log", test_log_file_, config); + EXPECT_EQ(s.GetExecutionConfig().total_threads, 4); + EXPECT_EQ(s.GetExecutionConfig().eigen_threads, 1); + EXPECT_TRUE(s.GetExecutionConfig().run_models_concurrently); + + ExecutionConfig updated; + updated.total_threads = 2; + updated.eigen_threads = 2; + s.SetExecutionConfig(updated); + + EXPECT_EQ(s.GetExecutionConfig().total_threads, 2); + EXPECT_EQ(s.GetExecutionConfig().eigen_threads, 2); + EXPECT_FALSE(s.GetExecutionConfig().run_models_concurrently); +} + +TEST_F(SimulationTest, PreservesExecutionConfigWhenCopied) { + ExecutionConfig config; + config.total_threads = 4; + config.eigen_threads = 1; + config.run_models_concurrently = true; + + Simulation original("custom_log", test_log_file_, config); + Simulation copy(original); + + EXPECT_EQ(copy.GetExecutionConfig().total_threads, 4); + EXPECT_EQ(copy.GetExecutionConfig().eigen_threads, 1); + EXPECT_TRUE(copy.GetExecutionConfig().run_models_concurrently); +} + +TEST_F(SimulationTest, StoresRuntimeConfig) { + RuntimeConfig config; + config.execution.total_threads = 4; + config.logging.logger_name = "runtime_simulation"; + config.logging.file_path = test_log_file_; + + Simulation s(config); + + EXPECT_EQ(s.GetRuntimeConfig().execution.total_threads, 4); + EXPECT_EQ(s.GetRuntimeConfig().logging.logger_name, + "runtime_simulation"); + EXPECT_EQ(s.GetRuntimeConfig().logging.file_path, test_log_file_); +} + TEST_F(SimulationTest, CreateNewModel) { Simulation s; std::string model_name = "test_model"; From 0d30b321cf0c0fd41e70f3fcc70d3baaf86e66ff Mon Sep 17 00:00:00 2001 From: Matthew Carroll <28577806+MJC598@users.noreply.github.com> Date: Tue, 22 Sep 2026 15:32:23 -0400 Subject: [PATCH 03/11] Removing the eager warning log for adding timesteps. We instead want to validate during runtime --- src/internals/markov.hpp | 31 ++++++++++++++----------------- tests/unit/markov_test.cpp | 4 +++- 2 files changed, 17 insertions(+), 18 deletions(-) diff --git a/src/internals/markov.hpp b/src/internals/markov.hpp index fee6429f..b9bc0c35 100644 --- a/src/internals/markov.hpp +++ b/src/internals/markov.hpp @@ -59,8 +59,8 @@ class Markov : public virtual Model { const std::string &log_filepath, const unsigned int processor_count = std::thread::hardware_concurrency()) - : Markov(name, log_name, log_filepath, - ExecutionConfig{0, processor_count, false}) {} + : Markov(name, log_name, log_filepath, + ExecutionConfig{0, processor_count, false}) {} Markov(const std::string &name, const RuntimeConfig &runtime_config) : _name(name), _log_name(runtime_config.logging.logger_name), @@ -77,18 +77,18 @@ class Markov : public virtual Model { Eigen::setNbThreads(threads); } - /// @brief Constructs a Markov model with explicit execution settings. - /// @param name The identifier for this model. - /// @param log_name The logger name for error reporting. - /// @param log_filepath The file path for the log file used by this model. - /// @param execution_config Resource settings for model execution. - [[deprecated("Use Markov(name, RuntimeConfig) instead")]] - Markov(const std::string &name, const std::string &log_name, - const std::string &log_filepath, - const ExecutionConfig &execution_config) - : Markov(name, RuntimeConfig{execution_config, - LoggingConfig{log_name, log_filepath, - false}}) {} + /// @brief Constructs a Markov model with explicit execution settings. + /// @param name The identifier for this model. + /// @param log_name The logger name for error reporting. + /// @param log_filepath The file path for the log file used by this model. + /// @param execution_config Resource settings for model execution. + [[deprecated("Use Markov(name, RuntimeConfig) instead")]] + Markov(const std::string &name, const std::string &log_name, + const std::string &log_filepath, + const ExecutionConfig &execution_config) + : Markov(name, + RuntimeConfig{execution_config, + LoggingConfig{log_name, log_filepath, false}}) {} /// @brief Destructor for Markov model. Default implementation. ~Markov() = default; @@ -215,9 +215,6 @@ class Markov : public virtual Model { void AddTimestep(const Timestep ×tep) override { _timestep_vector.push_back(timestep); - if (static_cast(_timestep_vector.size()) > _final_timestep) { - LogWarning(_log_name, "Final timestep exceeded by added timestep."); - } } void RunTimestep() override { RunTimestep(_current_timestep); } diff --git a/tests/unit/markov_test.cpp b/tests/unit/markov_test.cpp index 829deee5..329dd61d 100644 --- a/tests/unit/markov_test.cpp +++ b/tests/unit/markov_test.cpp @@ -290,9 +290,11 @@ TEST_F(MarkovTest, AddTimestepBeyondFinalTimestep) { Timestep timestep2(RESPOND_DEFAULT_LOG); markov.AddTimestep(timestep1); markov.AddTimestep(timestep2); + markov.SetInitialHistoryRecorded(true); + markov.RunTimesteps(); FlushAllLoggers(); EXPECT_TRUE(FileContains(RESPOND_DEFAULT_LOG_FILE, - "Final timestep exceeded by added timestep.")); + "Only running timesteps up to duration value.")); } TEST_F(MarkovTest, RunTimestep) { From 942168547cd796db467c949aac996b885c6a1bdd Mon Sep 17 00:00:00 2001 From: Matthew Carroll <28577806+MJC598@users.noreply.github.com> Date: Tue, 22 Sep 2026 16:27:32 -0400 Subject: [PATCH 04/11] fixing move/copy semantics --- include/respond/history.hpp | 21 +++++++--------- include/respond/simulation.hpp | 37 +++++++++++++-------------- include/respond/timestep.hpp | 19 ++++++++++---- src/internals/markov.hpp | 46 ++++++++++++---------------------- tests/unit/markov_test.cpp | 18 +++++++++++++ tests/unit/simulation_test.cpp | 20 +++++++++++++++ tests/unit/timestep_test.cpp | 30 ++++++++++++++++++++++ 7 files changed, 125 insertions(+), 66 deletions(-) diff --git a/include/respond/history.hpp b/include/respond/history.hpp index 9a73ab7e..a2e8ea1f 100644 --- a/include/respond/history.hpp +++ b/include/respond/history.hpp @@ -138,16 +138,13 @@ class History { } /// @brief Move constructor implementing the Rule of Five. - /// @param other The history to move from (leaves original state unchanged - /// per current implementation). - History(History &&other) noexcept { - _timesteps = std::move(other._timesteps); - _states = std::move(other._states); - _name = other._name; - _log_name = other._log_name; - _mode = other._mode; - _pending_state = std::move(other._pending_state); - } + /// @param other The history to move from. The moved-from history remains + /// valid but its contents are unspecified. + History(History &&other) noexcept + : _timesteps(std::move(other._timesteps)), + _states(std::move(other._states)), _name(std::move(other._name)), + _log_name(std::move(other._log_name)), _mode(other._mode), + _pending_state(std::move(other._pending_state)) {} /// @brief Move assignment operator implementing the Rule of Five. /// @param other The history to move from. @@ -156,8 +153,8 @@ class History { if (this != &other) { _timesteps = std::move(other._timesteps); _states = std::move(other._states); - _name = other._name; - _log_name = other._log_name; + _name = std::move(other._name); + _log_name = std::move(other._log_name); _mode = other._mode; _pending_state = std::move(other._pending_state); } diff --git a/include/respond/simulation.hpp b/include/respond/simulation.hpp index 8eb22415..5b525d6a 100644 --- a/include/respond/simulation.hpp +++ b/include/respond/simulation.hpp @@ -150,11 +150,12 @@ class Simulation { /// @return Reference to this simulation after assignment. Simulation &operator=(const Simulation &other) { if (this != &other) { - _runtime_config = other._runtime_config; - _models.clear(); + std::vector> models; for (const auto &m : other._models) { - _models.push_back(m->clone()); + models.push_back(m->clone()); } + _runtime_config = other._runtime_config; + _models = std::move(models); _duration = other._duration; _parameter_change_times = other._parameter_change_times; _stratify_entering_cohort = other._stratify_entering_cohort; @@ -170,18 +171,14 @@ class Simulation { /// @param other The simulation to move from. Simulation(Simulation &&other) noexcept : _runtime_config(std::move(other._runtime_config)), + _models(std::move(other._models)), _duration(other._duration), _parameter_change_times(std::move(other._parameter_change_times)), _stratify_entering_cohort(other._stratify_entering_cohort), _build_summary_stats(other._build_summary_stats), _save_state_history(other._save_state_history), - _timesteps_to_report(std::move(other._timesteps_to_report)), - _pivot_long(other._pivot_long) { - for (const auto &m : other._models) { - _models.push_back(m->clone()); - } - other._models.clear(); - } + _timesteps_to_report(std::move(other._timesteps_to_report)), + _pivot_long(other._pivot_long) {} /// @brief Move assignment operator for transferring simulation ownership. /// @param other The simulation to move from. @@ -189,6 +186,7 @@ class Simulation { Simulation &operator=(Simulation &&other) noexcept { if (this != &other) { _runtime_config = std::move(other._runtime_config); + _models = std::move(other._models); _duration = other._duration; _parameter_change_times = std::move(other._parameter_change_times); _stratify_entering_cohort = other._stratify_entering_cohort; @@ -196,11 +194,6 @@ class Simulation { _save_state_history = other._save_state_history; _timesteps_to_report = std::move(other._timesteps_to_report); _pivot_long = other._pivot_long; - - for (const auto &m : other._models) { - _models.push_back(m->clone()); - } - other._models.clear(); } return *this; } @@ -228,6 +221,12 @@ class Simulation { /// The model is cloned and managed by the simulation. /// @param model A unique_ptr to a Model instance to add. void AddModel(const std::unique_ptr &model) { + if (!model) { + LogError(_runtime_config.logging.logger_name, + "Cannot add a null model to the simulation."); + throw std::invalid_argument( + "Error attempting to add a null model to simulation."); + } _models.push_back(model->clone()); } @@ -411,12 +410,12 @@ class Simulation { int _duration = 1; // Default simulation duration in timesteps std::vector _parameter_change_times; - bool _stratify_entering_cohort; + bool _stratify_entering_cohort = false; - bool _build_summary_stats; - bool _save_state_history; + bool _build_summary_stats = false; + bool _save_state_history = false; std::vector _timesteps_to_report; - bool _pivot_long; + bool _pivot_long = false; }; } // namespace respond diff --git a/include/respond/timestep.hpp b/include/respond/timestep.hpp index c8e3c177..93d2132e 100644 --- a/include/respond/timestep.hpp +++ b/include/respond/timestep.hpp @@ -116,8 +116,7 @@ class Timestep { /// @brief Copy constructor for Timestep. Creates a deep copy of the /// transitions. /// @param other The Timestep instance to copy from. - Timestep(const Timestep &other) { - _transitions.clear(); + Timestep(const Timestep &other) : _log_name(other._log_name) { for (const auto &t : other._transitions) { _transitions.push_back(std::move(t->clone())); } @@ -129,10 +128,12 @@ class Timestep { /// @return Reference to this Timestep instance after assignment. Timestep &operator=(const Timestep &other) { if (this != &other) { - _transitions.clear(); + std::vector> transitions; for (const auto &t : other._transitions) { - _transitions.push_back(std::move(t->clone())); + transitions.push_back(t->clone()); } + _log_name = other._log_name; + _transitions = std::move(transitions); } return *this; } @@ -141,7 +142,8 @@ class Timestep { /// transitions. /// @param other The Timestep instance to move from. Timestep(Timestep &&other) noexcept - : _transitions(std::move(other._transitions)) { + : _log_name(std::move(other._log_name)), + _transitions(std::move(other._transitions)) { other._transitions.clear(); } @@ -151,6 +153,7 @@ class Timestep { /// @return Reference to this Timestep instance after assignment. Timestep &operator=(Timestep &&other) noexcept { if (this != &other) { + _log_name = std::move(other._log_name); _transitions = std::move(other._transitions); other._transitions.clear(); } @@ -185,6 +188,12 @@ class Timestep { /// The transition is cloned and managed by the timestep. /// @param transition A unique_ptr to a Transition instance to add. void AddTransition(const std::unique_ptr &transition) { + if (!transition) { + LogError(_log_name, + "Cannot add a null transition to the timestep."); + throw std::invalid_argument( + "Error attempting to add a null transition to timestep."); + } _transitions.push_back(transition->clone()); } diff --git a/src/internals/markov.hpp b/src/internals/markov.hpp index b9bc0c35..c1c96575 100644 --- a/src/internals/markov.hpp +++ b/src/internals/markov.hpp @@ -93,42 +93,28 @@ class Markov : public virtual Model { /// @brief Destructor for Markov model. Default implementation. ~Markov() = default; - Markov(Markov &&other) noexcept { - _state = other._state; - _name = other._name; - _log_name = other._log_name; - _runtime_config = other._runtime_config; - _current_timestep = other._current_timestep; - _history_capture_interval = other._history_capture_interval; - _final_timestep = other._final_timestep; - _initial_history_recorded = other._initial_history_recorded; - for (const auto &h : other._histories) { - _histories[h.first] = h.second; - } - other._histories.clear(); - for (const auto &t : other._timestep_vector) { - _timestep_vector.push_back(std::move(t)); - } - other.ClearTimesteps(); - } + Markov(Markov &&other) noexcept + : _timestep_vector(std::move(other._timestep_vector)), + _state(std::move(other._state)), _name(std::move(other._name)), + _log_name(std::move(other._log_name)), + _runtime_config(std::move(other._runtime_config)), + _histories(std::move(other._histories)), + _current_timestep(other._current_timestep), + _history_capture_interval(other._history_capture_interval), + _final_timestep(other._final_timestep), + _initial_history_recorded(other._initial_history_recorded) {} Markov &operator=(Markov &&other) noexcept { if (this != &other) { - _state = other._state; - _name = other._name; - _log_name = other._log_name; - _runtime_config = other._runtime_config; + _state = std::move(other._state); + _name = std::move(other._name); + _log_name = std::move(other._log_name); + _runtime_config = std::move(other._runtime_config); + _timestep_vector = std::move(other._timestep_vector); + _histories = std::move(other._histories); _current_timestep = other._current_timestep; _history_capture_interval = other._history_capture_interval; _final_timestep = other._final_timestep; _initial_history_recorded = other._initial_history_recorded; - for (const auto &h : other._histories) { - _histories[h.first] = h.second; - } - other._histories.clear(); - for (const auto &t : other._timestep_vector) { - _timestep_vector.push_back(std::move(t)); - } - other.ClearTimesteps(); } return *this; } diff --git a/tests/unit/markov_test.cpp b/tests/unit/markov_test.cpp index 329dd61d..81aae369 100644 --- a/tests/unit/markov_test.cpp +++ b/tests/unit/markov_test.cpp @@ -393,5 +393,23 @@ TEST_F(MarkovTest, ClearHistories) { markov.ClearHistories(); EXPECT_TRUE(markov.GetHistories().empty()); } + +TEST_F(MarkovTest, MoveAssignmentReplacesDestinationState) { + Markov source("source", RESPOND_DEFAULT_LOG); + source.AddTimestep(Timestep(RESPOND_DEFAULT_LOG)); + source.AddTimestep(Timestep(RESPOND_DEFAULT_LOG)); + source.CreateDefaultHistories(); + + Markov destination("destination", RESPOND_DEFAULT_LOG); + destination.AddTimestep(Timestep(RESPOND_DEFAULT_LOG)); + destination.ClearHistories(); + destination = std::move(source); + + EXPECT_EQ(destination.GetName(), "source"); + EXPECT_NO_THROW((void)destination.GetTimestepAtIndex(1)); + EXPECT_THROW((void)destination.GetTimestepAtIndex(2), std::out_of_range); + EXPECT_FALSE(destination.GetHistories().empty()); + EXPECT_TRUE(source.GetHistories().empty()); +} } // namespace testing } // namespace respond diff --git a/tests/unit/simulation_test.cpp b/tests/unit/simulation_test.cpp index 578a0711..be73b522 100644 --- a/tests/unit/simulation_test.cpp +++ b/tests/unit/simulation_test.cpp @@ -186,6 +186,19 @@ TEST_F(SimulationTest, ClearModels) { ASSERT_EQ(s.GetModels().size(), 0); } +TEST_F(SimulationTest, MoveAssignmentReplacesDestinationModels) { + Simulation source; + source.CreateNewModel("source_model"); + + Simulation destination; + destination.CreateNewModel("destination_model"); + destination = std::move(source); + + ASSERT_EQ(destination.GetModelNames(), + std::vector{"source_model"}); + ASSERT_TRUE(source.GetModels().empty()); +} + TEST_F(SimulationTest, AddModel) { Simulation s; auto mock_model = std::make_unique>(); @@ -197,6 +210,13 @@ TEST_F(SimulationTest, AddModel) { ASSERT_EQ(s.GetModels().size(), 1); } +TEST_F(SimulationTest, AddNullModelThrows) { + Simulation s; + std::unique_ptr null_model; + + EXPECT_THROW(s.AddModel(null_model), std::invalid_argument); +} + TEST_F(SimulationTest, Run) { Simulation s; auto mock_model = std::make_unique>(); diff --git a/tests/unit/timestep_test.cpp b/tests/unit/timestep_test.cpp index 4dc2d33c..fc2f6f9e 100644 --- a/tests/unit/timestep_test.cpp +++ b/tests/unit/timestep_test.cpp @@ -47,6 +47,7 @@ class TimestepTest : public ::testing::Test { std::string test_log_file_; std::string shared_log_file_; std::string default_log_file_; + }; TEST_F(TimestepTest, DefaultConstructor) { @@ -100,6 +101,13 @@ TEST_F(TimestepTest, AddTransitionClonesInputTransition) { ASSERT_TRUE(stored->GetMatrices()[0].isApprox(m1)); } +TEST_F(TimestepTest, AddNullTransitionThrows) { + Timestep ts("test_log", test_log_file_); + std::unique_ptr null_transition; + + EXPECT_THROW(ts.AddTransition(null_transition), std::invalid_argument); +} + TEST_F(TimestepTest, AddMatrixToTransitionByIndex) { Timestep ts("test_log", test_log_file_); const std::unique_ptr &transition = @@ -180,6 +188,28 @@ TEST_F(TimestepTest, CopyConstructor) { ASSERT_EQ(names[0], "migration"); } +TEST_F(TimestepTest, CopyPreservesLoggerName) { + Timestep original("test_log", test_log_file_); + Timestep copy(original); + spdlog::drop("test_log"); + + EXPECT_THROW(copy.AddMatrixToTransition(0, Eigen::MatrixXd::Identity(1, 1)), + std::out_of_range); + + EXPECT_EQ(CheckLoggerExists("test_log"), CreationStatus::kExists); +} + +TEST_F(TimestepTest, MovePreservesLoggerName) { + Timestep original("test_log", test_log_file_); + Timestep moved(std::move(original)); + spdlog::drop("test_log"); + + EXPECT_THROW(moved.AddMatrixToTransition(0, Eigen::MatrixXd::Identity(1, 1)), + std::out_of_range); + + EXPECT_EQ(CheckLoggerExists("test_log"), CreationStatus::kExists); +} + TEST_F(TimestepTest, CopyAssignment) { Timestep ts1("test_log", test_log_file_); ts1.CreateTransition("migration"); From d776ad45daec686bcdef7fbafbc649d1cf6070c3 Mon Sep 17 00:00:00 2001 From: Matthew Carroll <28577806+MJC598@users.noreply.github.com> Date: Wed, 23 Sep 2026 16:11:42 -0400 Subject: [PATCH 05/11] Fixing the CMake build for exporting all public headers and fixing the race condition in creating shared logger sinks --- CMakeLists.txt | 5 + include/respond/execution_config.hpp | 2 + include/respond/simulation.hpp | 77 +++++++++++++- include/respond/timestep.hpp | 25 +++-- include/respond/transition.hpp | 12 ++- src/internals/background.hpp | 16 ++- src/internals/behavior.hpp | 15 ++- src/internals/intervention.hpp | 16 ++- src/internals/migration.hpp | 15 ++- src/internals/overdose.hpp | 15 ++- src/internals/transition_base.hpp | 13 ++- src/logging.cpp | 86 ++++++++------- src/transition_factory.cpp | 18 ++-- tests/unit/logging_test.cpp | 38 +++++++ tests/unit/simulation_test.cpp | 152 +++++++++++++++++++++++++++ tests/unit/timestep_test.cpp | 30 ++++++ 16 files changed, 451 insertions(+), 84 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index ae62eb39..c2b9c4c3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -98,10 +98,13 @@ target_sources(respond_model FILES include/respond/constants.hpp include/respond/cost_effectiveness.hpp + include/respond/execution_config.hpp include/respond/history.hpp include/respond/logging.hpp + include/respond/logging_config.hpp include/respond/model.hpp include/respond/respond.hpp + include/respond/runtime_config.hpp include/respond/simulation.hpp include/respond/timestep.hpp include/respond/transition.hpp @@ -112,6 +115,7 @@ target_sources(respond_model # Linking Libraries #------------------------------------------------------------------------------- include(${PRIVATE_MODULE_PATH}/MakeDependenciesAvailable.cmake) +find_package(Threads REQUIRED) set(private_deps spdlog::spdlog) if (RESPOND_CALCULATE_COVERAGE) list(APPEND private_deps gcov) @@ -120,6 +124,7 @@ endif() target_link_libraries(respond_model PUBLIC Eigen3::Eigen + Threads::Threads PRIVATE ${private_deps} ) diff --git a/include/respond/execution_config.hpp b/include/respond/execution_config.hpp index 53ff8d70..944285cc 100644 --- a/include/respond/execution_config.hpp +++ b/include/respond/execution_config.hpp @@ -21,6 +21,8 @@ struct ExecutionConfig { unsigned int eigen_threads = 1; /// @brief Whether models may be executed concurrently by the simulation. + /// When multiple models actually run in parallel, eigen_threads must be 1 + /// or less because Eigen's worker setting is process-global. bool run_models_concurrently = false; }; } // namespace respond diff --git a/include/respond/simulation.hpp b/include/respond/simulation.hpp index 5b525d6a..0dbcb982 100644 --- a/include/respond/simulation.hpp +++ b/include/respond/simulation.hpp @@ -18,9 +18,14 @@ #include #include +#include +#include #include #include +#include +#include #include +#include #include #include @@ -230,8 +235,13 @@ class Simulation { _models.push_back(model->clone()); } - /// @brief Executes one step of the simulation for all models. - /// Calls RunTransitions() on each registered model in sequence. + /// @brief Executes the simulation for all registered models. + /// Models run sequentially by default. When model concurrency is enabled, + /// independent models may run in parallel using the configured worker + /// limit. + /// @throws std::invalid_argument if multiple models would execute in + /// parallel while more than one Eigen worker thread is configured. + /// @throws Any exception raised by a model after all workers have joined. void Run(int duration = -1) { if (duration > 0) { _duration = duration; @@ -239,9 +249,70 @@ class Simulation { LogInfo(_runtime_config.logging.logger_name, "Running simulation for duration of " + std::to_string(_duration) + " timesteps."); - for (const auto &model : _models) { + const auto run_model = [this](const std::unique_ptr &model) { model->SetFinalTimestep(_duration); model->RunTimesteps(); + }; + + const auto &execution = _runtime_config.execution; + if (!execution.run_models_concurrently || _models.size() < 2) { + for (const auto &model : _models) { + run_model(model); + } + return; + } + + if (execution.eigen_threads > 1) { + throw std::invalid_argument( + "Concurrent model execution requires eigen_threads <= 1."); + } + + unsigned int worker_limit = execution.total_threads; + if (worker_limit == 0) { + worker_limit = std::thread::hardware_concurrency(); + if (worker_limit == 0) { + worker_limit = 1; + } + } + const auto worker_count = std::min(worker_limit, _models.size()); + if (worker_count <= 1) { + for (const auto &model : _models) { + run_model(model); + } + return; + } + + std::atomic next_model{0}; + std::exception_ptr first_exception; + std::mutex exception_mutex; + std::vector workers; + workers.reserve(worker_count); + + for (size_t worker = 0; worker < worker_count; ++worker) { + workers.emplace_back([&]() { + while (true) { + const size_t index = next_model.fetch_add(1); + if (index >= _models.size()) { + return; + } + try { + run_model(_models[index]); + } catch (...) { + std::lock_guard lock(exception_mutex); + if (!first_exception) { + first_exception = std::current_exception(); + } + return; + } + } + }); + } + + for (auto &worker : workers) { + worker.join(); + } + if (first_exception) { + std::rethrow_exception(first_exception); } } diff --git a/include/respond/timestep.hpp b/include/respond/timestep.hpp index 93d2132e..d6b647b5 100644 --- a/include/respond/timestep.hpp +++ b/include/respond/timestep.hpp @@ -18,6 +18,7 @@ #include #include +#include #include namespace respond { @@ -92,11 +93,12 @@ class Timestep { /// @brief Default constructor for Timestep. Initializes with default /// logger. - Timestep() : Timestep(RESPOND_DEFAULT_LOG) {} + Timestep() : Timestep(LoggingConfig{}) {} /// @brief Default constructor for Timestep with specified logger name. /// Initializes with default log file path. /// @param log_name String name for the logger to be used by this timestep. + [[deprecated("Use Timestep(LoggingConfig) instead")]] Timestep(const std::string &log_name) : Timestep(log_name, RESPOND_DEFAULT_LOG_FILE) {} @@ -105,9 +107,14 @@ class Timestep { /// @param log_name String name for the logger to be used by this timestep. /// @param log_filepath String path for the log file to be used by this /// timestep. + [[deprecated("Use Timestep(LoggingConfig) instead")]] Timestep(const std::string &log_name, const std::string &log_filepath) - : _log_name(log_name) { - CreateFileLogger(log_name, log_filepath); + : Timestep(LoggingConfig{log_name, log_filepath, false}) {} + + explicit Timestep(const LoggingConfig &logging_config) + : _log_name(logging_config.logger_name), + _logging_config(logging_config) { + ConfigureLogger(_logging_config); } /// @brief Destructor for Timestep. Default implementation. @@ -116,7 +123,8 @@ class Timestep { /// @brief Copy constructor for Timestep. Creates a deep copy of the /// transitions. /// @param other The Timestep instance to copy from. - Timestep(const Timestep &other) : _log_name(other._log_name) { + Timestep(const Timestep &other) + : _log_name(other._log_name), _logging_config(other._logging_config) { for (const auto &t : other._transitions) { _transitions.push_back(std::move(t->clone())); } @@ -133,6 +141,7 @@ class Timestep { transitions.push_back(t->clone()); } _log_name = other._log_name; + _logging_config = other._logging_config; _transitions = std::move(transitions); } return *this; @@ -142,7 +151,8 @@ class Timestep { /// transitions. /// @param other The Timestep instance to move from. Timestep(Timestep &&other) noexcept - : _log_name(std::move(other._log_name)), + : _log_name(std::move(other._log_name)), + _logging_config(std::move(other._logging_config)), _transitions(std::move(other._transitions)) { other._transitions.clear(); } @@ -154,6 +164,7 @@ class Timestep { Timestep &operator=(Timestep &&other) noexcept { if (this != &other) { _log_name = std::move(other._log_name); + _logging_config = std::move(other._logging_config); _transitions = std::move(other._transitions); other._transitions.clear(); } @@ -180,7 +191,8 @@ class Timestep { const std::unique_ptr & CreateTransition(const std::string &transition_name) { _transitions.push_back( - Transition::Create(transition_name, transition_name, _log_name)); + Transition::Create(transition_name, transition_name, + _logging_config)); return _transitions.back(); } @@ -416,6 +428,7 @@ class Timestep { } std::string _log_name; + LoggingConfig _logging_config; std::vector> _transitions; }; } // namespace respond diff --git a/include/respond/transition.hpp b/include/respond/transition.hpp index 1bed2c06..1bb3aa9d 100644 --- a/include/respond/transition.hpp +++ b/include/respond/transition.hpp @@ -22,6 +22,7 @@ #include #include +#include namespace respond { @@ -81,14 +82,21 @@ class Transition { /// - "overdose": Overdose-related transitions /// - "background_death": Background mortality transitions /// @param log_name The logger name for error reporting (e.g., "console"). - /// @return A unique_ptr to the created Transition, or nullptr if type is - /// unsupported. + /// @return A unique_ptr to the created Transition. + /// @throws std::invalid_argument if type is unsupported. The error is also + /// written through the logger identified by log_name. + [[deprecated( + "Use Transition::Create(type, name, LoggingConfig) instead")]] static std::unique_ptr Create(const std::string &type, const std::string &name = RESPOND_DEFAULT_TRANSITION_NAME, const std::string &log_name = RESPOND_DEFAULT_LOG, const std::string &log_file = RESPOND_DEFAULT_LOG_FILE); + static std::unique_ptr + Create(const std::string &type, const std::string &name, + const LoggingConfig &logging_config); + /// @brief Helper function to overload to the stream insertion operator for /// Transition serialization. /// @details This function is intended to be overridden by subclasses to diff --git a/src/internals/background.hpp b/src/internals/background.hpp index 5845f317..6db492df 100644 --- a/src/internals/background.hpp +++ b/src/internals/background.hpp @@ -25,14 +25,20 @@ namespace respond { class BackgroundDeath : public virtual TransitionBase { public: - BackgroundDeath() : BackgroundDeath("background_death") {} + BackgroundDeath() : BackgroundDeath("background_death", LoggingConfig{}) {} BackgroundDeath(const std::string &name) - : BackgroundDeath(name, RESPOND_DEFAULT_LOG) {} + : BackgroundDeath(name, LoggingConfig{}) {} + [[deprecated("Use BackgroundDeath(name, LoggingConfig) instead")]] BackgroundDeath(const std::string &name, const std::string &log_name) - : BackgroundDeath(name, log_name, RESPOND_DEFAULT_LOG_FILE) {} + : BackgroundDeath(name, LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, + false}) {} + [[deprecated("Use BackgroundDeath(name, LoggingConfig) instead")]] BackgroundDeath(const std::string &name, const std::string &log_name, const std::string &log_file) - : TransitionBase(name, log_name, log_file) {} + : TransitionBase(name, LoggingConfig{log_name, log_file, false}) {} + BackgroundDeath(const std::string &name, + const LoggingConfig &logging_config) + : TransitionBase(name, logging_config) {} // Run the execute function and return the final state. Do not edit the // parameter state, but do edit the history provided. Nothing in the @@ -42,7 +48,7 @@ class BackgroundDeath : public virtual TransitionBase { // Clone std::unique_ptr clone() const override { - auto ret = std::make_unique(GetName(), _log_name); + auto ret = std::make_unique(GetName(), _logging_config); for (const auto &t : GetMatrices()) { ret->AddMatrix(t); } diff --git a/src/internals/behavior.hpp b/src/internals/behavior.hpp index d1e4c2da..6aea2b25 100644 --- a/src/internals/behavior.hpp +++ b/src/internals/behavior.hpp @@ -25,13 +25,18 @@ namespace respond { class Behavior : public virtual TransitionBase { public: - Behavior() : Behavior("behavior") {} - Behavior(const std::string &name) : Behavior(name, RESPOND_DEFAULT_LOG) {} + Behavior() : Behavior("behavior", LoggingConfig{}) {} + Behavior(const std::string &name) : Behavior(name, LoggingConfig{}) {} + [[deprecated("Use Behavior(name, LoggingConfig) instead")]] Behavior(const std::string &name, const std::string &log_name) - : Behavior(name, log_name, RESPOND_DEFAULT_LOG_FILE) {} + : Behavior(name, LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, + false}) {} + [[deprecated("Use Behavior(name, LoggingConfig) instead")]] Behavior(const std::string &name, const std::string &log_name, const std::string &log_file) - : TransitionBase(name, log_name, log_file) {} + : TransitionBase(name, LoggingConfig{log_name, log_file, false}) {} + Behavior(const std::string &name, const LoggingConfig &logging_config) + : TransitionBase(name, logging_config) {} // Run the execute function and return the final state. Do not edit the // parameter state, but do edit the history provided. Nothing in the @@ -41,7 +46,7 @@ class Behavior : public virtual TransitionBase { // Clone std::unique_ptr clone() const override { - auto ret = std::make_unique(GetName(), _log_name); + auto ret = std::make_unique(GetName(), _logging_config); for (const auto &t : GetMatrices()) { ret->AddMatrix(t); } diff --git a/src/internals/intervention.hpp b/src/internals/intervention.hpp index 7f6da082..a5500c77 100644 --- a/src/internals/intervention.hpp +++ b/src/internals/intervention.hpp @@ -25,14 +25,20 @@ namespace respond { class Intervention : public virtual TransitionBase { public: - Intervention() : Intervention("intervention") {} + Intervention() : Intervention("intervention", LoggingConfig{}) {} Intervention(const std::string &name) - : Intervention(name, RESPOND_DEFAULT_LOG) {} + : Intervention(name, LoggingConfig{}) {} + [[deprecated("Use Intervention(name, LoggingConfig) instead")]] Intervention(const std::string &name, const std::string &log_name) - : Intervention(name, log_name, RESPOND_DEFAULT_LOG_FILE) {} + : Intervention(name, LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, + false}) {} + [[deprecated("Use Intervention(name, LoggingConfig) instead")]] Intervention(const std::string &name, const std::string &log_name, const std::string &log_file) - : TransitionBase(name, log_name, log_file) {} + : TransitionBase(name, LoggingConfig{log_name, log_file, false}) {} + Intervention(const std::string &name, + const LoggingConfig &logging_config) + : TransitionBase(name, logging_config) {} // Run the execute function and return the final state. Do not edit the // parameter state, but do edit the history provided. Nothing in the @@ -42,7 +48,7 @@ class Intervention : public virtual TransitionBase { // Clone std::unique_ptr clone() const override { - auto ret = std::make_unique(GetName(), _log_name); + auto ret = std::make_unique(GetName(), _logging_config); for (const auto &t : GetMatrices()) { ret->AddMatrix(t); } diff --git a/src/internals/migration.hpp b/src/internals/migration.hpp index 4c220cf0..8c9c2425 100644 --- a/src/internals/migration.hpp +++ b/src/internals/migration.hpp @@ -25,13 +25,18 @@ namespace respond { class Migration : public virtual TransitionBase { public: - Migration() : Migration("migration") {} - Migration(const std::string &name) : Migration(name, RESPOND_DEFAULT_LOG) {} + Migration() : Migration("migration", LoggingConfig{}) {} + Migration(const std::string &name) : Migration(name, LoggingConfig{}) {} + [[deprecated("Use Migration(name, LoggingConfig) instead")]] Migration(const std::string &name, const std::string &log_name) - : Migration(name, log_name, RESPOND_DEFAULT_LOG_FILE) {} + : Migration(name, LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, + false}) {} + [[deprecated("Use Migration(name, LoggingConfig) instead")]] Migration(const std::string &name, const std::string &log_name, const std::string &log_file) - : TransitionBase(name, log_name, log_file) {} + : TransitionBase(name, LoggingConfig{log_name, log_file, false}) {} + Migration(const std::string &name, const LoggingConfig &logging_config) + : TransitionBase(name, logging_config) {} // Run the execute function and return the final state. Do not edit the // parameter state, but do edit the history provided. Nothing in the @@ -41,7 +46,7 @@ class Migration : public virtual TransitionBase { // Clone std::unique_ptr clone() const override { - auto ret = std::make_unique(GetName(), _log_name); + auto ret = std::make_unique(GetName(), _logging_config); for (const auto &t : GetMatrices()) { ret->AddMatrix(t); } diff --git a/src/internals/overdose.hpp b/src/internals/overdose.hpp index a2128e93..cfdfadff 100644 --- a/src/internals/overdose.hpp +++ b/src/internals/overdose.hpp @@ -25,13 +25,18 @@ namespace respond { class Overdose : public virtual TransitionBase { public: - Overdose() : Overdose("overdose") {} - Overdose(const std::string &name) : Overdose(name, RESPOND_DEFAULT_LOG) {} + Overdose() : Overdose("overdose", LoggingConfig{}) {} + Overdose(const std::string &name) : Overdose(name, LoggingConfig{}) {} + [[deprecated("Use Overdose(name, LoggingConfig) instead")]] Overdose(const std::string &name, const std::string &log_name) - : Overdose(name, log_name, RESPOND_DEFAULT_LOG_FILE) {} + : Overdose(name, LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, + false}) {} + [[deprecated("Use Overdose(name, LoggingConfig) instead")]] Overdose(const std::string &name, const std::string &log_name, const std::string &log_file) - : TransitionBase(name, log_name, log_file) {} + : TransitionBase(name, LoggingConfig{log_name, log_file, false}) {} + Overdose(const std::string &name, const LoggingConfig &logging_config) + : TransitionBase(name, logging_config) {} // Run the execute function and return the final state. Do not edit the // parameter state, but do edit the history provided. Nothing in the @@ -41,7 +46,7 @@ class Overdose : public virtual TransitionBase { // Clone std::unique_ptr clone() const override { - auto ret = std::make_unique(GetName(), _log_name); + auto ret = std::make_unique(GetName(), _logging_config); for (const auto &t : GetMatrices()) { ret->AddMatrix(t); } diff --git a/src/internals/transition_base.hpp b/src/internals/transition_base.hpp index baf213ef..600d874a 100644 --- a/src/internals/transition_base.hpp +++ b/src/internals/transition_base.hpp @@ -13,6 +13,7 @@ #define RESPOND_INTERNALS_TRANSITION_BASE_HPP_ #include +#include #include #include @@ -24,11 +25,16 @@ namespace respond { class TransitionBase : public virtual Transition { public: + [[deprecated("Use TransitionBase(name, LoggingConfig) instead")]] TransitionBase(const std::string &name, const std::string &log_name, const std::string &log_file) - : _name(name), _log_name(log_name) { - CreateFileLogger(log_name, log_file); - } + : TransitionBase(name, LoggingConfig{log_name, log_file, false}) {} + TransitionBase(const std::string &name, + const LoggingConfig &logging_config) + : _name(name), _log_name(logging_config.logger_name), + _logging_config(logging_config) { + ConfigureLogger(_logging_config); + } virtual ~TransitionBase() = default; // Add a Transition Matrix to the set. We have no need to edit it once it's // been added, just use it. Thus, we don't need full ownership (reference) @@ -52,6 +58,7 @@ class TransitionBase : public virtual Transition { protected: const std::string _log_name; + const LoggingConfig _logging_config; void TestMatrixSizes(const Eigen::Ref &m1, const Eigen::Ref &m2) const { diff --git a/src/logging.cpp b/src/logging.cpp index 586063ca..337c2a7d 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -18,6 +18,47 @@ namespace respond { +namespace { + +CreationStatus CreateSharedLogger( + const std::string &logger_name, + const std::shared_ptr &sink) { + if (CheckIfExists(logger_name) == CreationStatus::kExists) { + std::cout << "Shared logger " << logger_name << " already exists" + << std::endl; + return CreationStatus::kExists; + } + + if (!sink) { + std::string error_msg = + "Failed to create shared logger '" + logger_name + + "': shared sink is null"; + std::cerr << error_msg << std::endl; + return CreationStatus::kError; + } + + try { + spdlog::cfg::load_env_levels(); + std::string pattern = + LoggingRegistry::GetPatternString(LoggingRegistry::GetPattern()); + + auto logger = std::make_shared(logger_name, sink); + logger->set_pattern(pattern); + logger->set_level(spdlog::level::trace); + + spdlog::register_logger(logger); + + return CreationStatus::kSuccess; + } catch (const spdlog::spdlog_ex &ex) { + std::string error_msg = "Failed to create shared logger '" + + logger_name + "': " + ex.what(); + std::cerr << error_msg << std::endl; + return CreationStatus::kError; + } +} + +} // namespace + CreationStatus CreateFileLogger(const std::string &logger_name, const std::string &filepath) { if (CheckIfExists(logger_name) == CreationStatus::kExists) { @@ -60,49 +101,16 @@ CreationStatus CreateSharedFileSink(const std::string &filepath) { } CreationStatus CreateSharedLogger(const std::string &logger_name) { - if (CheckIfExists(logger_name) == CreationStatus::kExists) { - std::cout << "Shared logger " << logger_name << " already exists" - << std::endl; - return CreationStatus::kExists; - } - - try { - std::string filepath = LoggingRegistry::GetDefaultSinkPath(); - auto sink = LoggingRegistry::GetSharedSink(filepath); - if (!sink) { - std::string error_msg = - "Failed to create shared logger '" + logger_name + - "': could not get or create shared sink at '" + filepath + "'"; - std::cerr << error_msg << std::endl; - return CreationStatus::kError; - } - - spdlog::cfg::load_env_levels(); - std::string pattern = - LoggingRegistry::GetPatternString(LoggingRegistry::GetPattern()); - - auto logger = std::make_shared(logger_name, sink); - logger->set_pattern(pattern); - logger->set_level(spdlog::level::trace); - - spdlog::register_logger(logger); - - return CreationStatus::kSuccess; - } catch (const spdlog::spdlog_ex &ex) { - std::string error_msg = "Failed to create shared logger '" + - logger_name + "': " + ex.what(); - std::cerr << error_msg << std::endl; - return CreationStatus::kError; - } + const std::string filepath = LoggingRegistry::GetDefaultSinkPath(); + return CreateSharedLogger(logger_name, + LoggingRegistry::GetSharedSink(filepath)); } CreationStatus ConfigureLogger(const LoggingConfig &config) { if (config.use_shared_sink) { - const auto sink_status = CreateSharedFileSink(config.file_path); - if (sink_status == CreationStatus::kError) { - return sink_status; - } - return CreateSharedLogger(config.logger_name); + return CreateSharedLogger( + config.logger_name, + LoggingRegistry::GetSharedSink(config.file_path)); } return CreateFileLogger(config.logger_name, config.file_path); } diff --git a/src/transition_factory.cpp b/src/transition_factory.cpp index eb87e726..4f25e297 100644 --- a/src/transition_factory.cpp +++ b/src/transition_factory.cpp @@ -28,27 +28,33 @@ std::unique_ptr Transition::Create(const std::string &type, const std::string &name, const std::string &log_name, const std::string &log_file) { + return Create(type, name, LoggingConfig{log_name, log_file, false}); +} + +std::unique_ptr +Transition::Create(const std::string &type, const std::string &name, + const LoggingConfig &logging_config) { std::string type_copy = type; std::transform(type_copy.begin(), type_copy.end(), type_copy.begin(), [](unsigned char c) { return std::tolower(c); }); if (type_copy == "migration") { - return std::make_unique(name, log_name, log_file); + return std::make_unique(name, logging_config); } else if (type_copy == "behavior") { - return std::make_unique(name, log_name, log_file); + return std::make_unique(name, logging_config); } else if (type_copy == "intervention") { - return std::make_unique(name, log_name, log_file); + return std::make_unique(name, logging_config); } else if (type_copy == "overdose") { - return std::make_unique(name, log_name, log_file); + return std::make_unique(name, logging_config); } else if (type_copy == "background_death") { - return std::make_unique(name, log_name, log_file); + return std::make_unique(name, logging_config); } // Invalid transition type std::string error_msg = "Invalid transition type: '" + type + "'. Supported types: migration, behavior, " "intervention, overdose, background_death"; - LogError(log_name, error_msg); + LogError(logging_config.logger_name, error_msg); throw std::invalid_argument(error_msg); } } // namespace respond \ No newline at end of file diff --git a/tests/unit/logging_test.cpp b/tests/unit/logging_test.cpp index c3b40de5..b73b83bf 100644 --- a/tests/unit/logging_test.cpp +++ b/tests/unit/logging_test.cpp @@ -143,6 +143,44 @@ TEST_F(LoggingTest, ConfigureLoggerCreatesSharedLogger) { EXPECT_NE(spdlog::get(config.logger_name), nullptr); } +TEST_F(LoggingTest, ConcurrentSharedLoggerConfigurationUsesRequestedSinks) { + constexpr size_t logger_count = 8; + std::vector log_files; + std::vector logger_names; + std::vector statuses(logger_count, + CreationStatus::kError); + std::vector workers; + + for (size_t i = 0; i < logger_count; ++i) { + log_files.push_back("/tmp/respond_concurrent_shared_" + + std::to_string(i) + ".log"); + logger_names.push_back("concurrent_shared_logger_" + + std::to_string(i)); + std::remove(log_files.back().c_str()); + } + + for (size_t i = 0; i < logger_count; ++i) { + workers.emplace_back([&, i] { + const LoggingConfig config{logger_names[i], log_files[i], true}; + statuses[i] = ConfigureLogger(config); + if (statuses[i] == CreationStatus::kSuccess) { + LogInfo(logger_names[i], "message-" + std::to_string(i)); + } + }); + } + + for (auto &worker : workers) { + worker.join(); + } + FlushAllLoggers(); + + for (size_t i = 0; i < logger_count; ++i) { + EXPECT_EQ(statuses[i], CreationStatus::kSuccess); + EXPECT_TRUE(FileContains(log_files[i], "message-" + std::to_string(i))); + std::remove(log_files[i].c_str()); + } +} + TEST_F(LoggingTest, CreateSharedFileSinkCaching) { // Create sink first time CreationStatus status1 = CreateSharedFileSink(shared_log_file_); diff --git a/tests/unit/simulation_test.cpp b/tests/unit/simulation_test.cpp index be73b522..c086ba1b 100644 --- a/tests/unit/simulation_test.cpp +++ b/tests/unit/simulation_test.cpp @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -247,6 +248,157 @@ TEST_F(SimulationTest, RunMultipleModels) { s.Run(); } +TEST_F(SimulationTest, RunsModelsConcurrentlyWhenEnabled) { + ExecutionConfig config; + config.total_threads = 2; + config.eigen_threads = 1; + config.run_models_concurrently = true; + Simulation simulation(RuntimeConfig{config, LoggingConfig{}}); + + std::atomic entered{0}; + std::atomic maximum_active{0}; + auto run_model = [&]() { + const int active = entered.fetch_add(1) + 1; + int observed_maximum = maximum_active.load(); + while (active > observed_maximum && + !maximum_active.compare_exchange_weak(observed_maximum, + active)) { + } + while (entered.load() < 2) { + std::this_thread::yield(); + } + }; + + auto first_source = std::make_unique>(); + auto first_model = std::make_unique>(); + EXPECT_CALL(*first_model, RunTimesteps()).WillOnce(run_model); + EXPECT_CALL(*first_source, clone()) + .WillOnce(Return(::testing::ByMove(std::move(first_model)))); + simulation.AddModel(std::move(first_source)); + + auto second_source = std::make_unique>(); + auto second_model = std::make_unique>(); + EXPECT_CALL(*second_model, RunTimesteps()).WillOnce(run_model); + EXPECT_CALL(*second_source, clone()) + .WillOnce(Return(::testing::ByMove(std::move(second_model)))); + simulation.AddModel(std::move(second_source)); + + simulation.Run(); + + EXPECT_EQ(entered.load(), 2); + EXPECT_EQ(maximum_active.load(), 2); +} + +TEST_F(SimulationTest, TotalThreadsOneRunsModelsSequentially) { + ExecutionConfig config; + config.total_threads = 1; + config.run_models_concurrently = true; + Simulation simulation(RuntimeConfig{config, LoggingConfig{}}); + std::atomic active{0}; + std::atomic maximum_active{0}; + + auto run_model = [&]() { + const int current = active.fetch_add(1) + 1; + maximum_active.store(std::max(maximum_active.load(), current)); + active.fetch_sub(1); + }; + + for (int index = 0; index < 2; ++index) { + auto source = std::make_unique>(); + auto model = std::make_unique>(); + EXPECT_CALL(*model, RunTimesteps()).WillOnce(run_model); + EXPECT_CALL(*source, clone()) + .WillOnce(Return(::testing::ByMove(std::move(model)))); + simulation.AddModel(std::move(source)); + } + + simulation.Run(); + + EXPECT_EQ(maximum_active.load(), 1); +} + +TEST_F(SimulationTest, AllowsEigenThreadsWhenConcurrencyFallsBackToSequential) { + ExecutionConfig config; + config.eigen_threads = 2; + config.run_models_concurrently = false; + Simulation simulation(RuntimeConfig{config, LoggingConfig{}}); + + for (int index = 0; index < 2; ++index) { + auto source = std::make_unique>(); + auto model = std::make_unique>(); + EXPECT_CALL(*model, RunTimesteps()).Times(1); + EXPECT_CALL(*source, clone()) + .WillOnce(Return(::testing::ByMove(std::move(model)))); + simulation.AddModel(std::move(source)); + } + + EXPECT_NO_THROW(simulation.Run()); +} + +TEST_F(SimulationTest, AllowsEigenThreadsForSingleConcurrentModel) { + ExecutionConfig config; + config.eigen_threads = 2; + config.run_models_concurrently = true; + Simulation simulation(RuntimeConfig{config, LoggingConfig{}}); + + auto source = std::make_unique>(); + auto model = std::make_unique>(); + EXPECT_CALL(*model, RunTimesteps()).Times(1); + EXPECT_CALL(*source, clone()) + .WillOnce(Return(::testing::ByMove(std::move(model)))); + simulation.AddModel(std::move(source)); + + EXPECT_NO_THROW(simulation.Run()); +} + +TEST_F(SimulationTest, RejectsConcurrentEigenOversubscription) { + ExecutionConfig config; + config.eigen_threads = 2; + config.run_models_concurrently = true; + Simulation simulation(RuntimeConfig{config, LoggingConfig{}}); + + auto first_source = std::make_unique>(); + auto first_model = std::make_unique>(); + EXPECT_CALL(*first_source, clone()) + .WillOnce(Return(::testing::ByMove(std::move(first_model)))); + simulation.AddModel(std::move(first_source)); + + auto second_source = std::make_unique>(); + auto second_model = std::make_unique>(); + EXPECT_CALL(*second_source, clone()) + .WillOnce(Return(::testing::ByMove(std::move(second_model)))); + simulation.AddModel(std::move(second_source)); + + EXPECT_THROW(simulation.Run(), std::invalid_argument); +} + +TEST_F(SimulationTest, RethrowsWorkerExceptionAfterJoining) { + ExecutionConfig config; + config.total_threads = 2; + config.run_models_concurrently = true; + Simulation simulation(RuntimeConfig{config, LoggingConfig{}}); + std::atomic completed{0}; + + auto throwing_source = std::make_unique>(); + auto throwing_model = std::make_unique>(); + EXPECT_CALL(*throwing_model, RunTimesteps()) + .WillOnce(::testing::Throw(std::runtime_error("worker failure"))); + EXPECT_CALL(*throwing_source, clone()) + .WillOnce(Return(::testing::ByMove(std::move(throwing_model)))); + simulation.AddModel(std::move(throwing_source)); + + auto completing_source = std::make_unique>(); + auto completing_model = std::make_unique>(); + EXPECT_CALL(*completing_model, RunTimesteps()) + .WillOnce([&]() { completed.fetch_add(1); }); + EXPECT_CALL(*completing_source, clone()) + .WillOnce(Return(::testing::ByMove(std::move(completing_model)))); + simulation.AddModel(std::move(completing_source)); + + EXPECT_THROW(simulation.Run(), std::runtime_error); + EXPECT_EQ(completed.load(), 1); +} + TEST_F(SimulationTest, GetModels) { Simulation s; auto mock_model = std::make_unique>(); diff --git a/tests/unit/timestep_test.cpp b/tests/unit/timestep_test.cpp index fc2f6f9e..b9d81d75 100644 --- a/tests/unit/timestep_test.cpp +++ b/tests/unit/timestep_test.cpp @@ -77,6 +77,36 @@ TEST_F(TimestepTest, CreateTransition) { ASSERT_EQ(transition->GetName(), "migration"); } +TEST_F(TimestepTest, CreateTransitionUsesLoggingConfig) { + const LoggingConfig logging_config{"configured_transition", test_log_file_, + false}; + Timestep ts(logging_config); + + const auto &transition = ts.CreateTransition("migration"); + + ASSERT_NE(transition, nullptr); + EXPECT_EQ(CheckLoggerExists("configured_transition"), + CreationStatus::kExists); +} + +TEST_F(TimestepTest, ClonedTransitionPreservesLoggingConfig) { + const LoggingConfig logging_config{"clone_transition", test_log_file_, + false}; + auto transition = Transition::Create("migration", "migration", + logging_config); + auto clone = transition->clone(); + + ASSERT_NE(clone, nullptr); + EXPECT_EQ(CheckLoggerExists("clone_transition"), CreationStatus::kExists); +} + +TEST_F(TimestepTest, CreateTransitionRejectsUnsupportedType) { + EXPECT_THROW( + Transition::Create("unsupported", "unsupported", "test_log", + test_log_file_), + std::invalid_argument); +} + TEST_F(TimestepTest, AddTransitionClonesInputTransition) { Timestep ts("test_log", test_log_file_); From ff6e05771d0189a037f4fc2ada43fd46223a797b Mon Sep 17 00:00:00 2001 From: Matthew Carroll <28577806+MJC598@users.noreply.github.com> Date: Thu, 24 Sep 2026 09:49:19 -0400 Subject: [PATCH 06/11] updating install location, shared sink ambiguity, history insert order issues, and discount calculations --- cmake/InstallRespond.cmake | 10 +- docs/src/api-guide.md | 191 ++++++++++++++++--------- include/respond/cost_effectiveness.hpp | 6 +- include/respond/history.hpp | 22 +-- include/respond/logging.hpp | 7 +- src/internals/logging_internals.hpp | 20 ++- src/logging.cpp | 66 +++++++-- tests/CMakeLists.txt | 2 +- tests/unit/cost_effectiveness_test.cpp | 6 +- tests/unit/history_test.cpp | 18 +++ tests/unit/logging_test.cpp | 63 +++++++- tests/unit/markov_test.cpp | 4 +- tests/unit/timestep_test.cpp | 5 +- 13 files changed, 317 insertions(+), 103 deletions(-) diff --git a/cmake/InstallRespond.cmake b/cmake/InstallRespond.cmake index 27df799f..71fbb4ca 100644 --- a/cmake/InstallRespond.cmake +++ b/cmake/InstallRespond.cmake @@ -6,11 +6,11 @@ message(STATUS "Installing respond version ${RESPOND_VERSION} to ${CMAKE_INSTALL # CMAKE_INSTALL_PREFIX variable. install(TARGETS respond_model EXPORT respondTargets - LIBRARY DESTINATION lib - ARCHIVE DESTINATION lib + LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} + ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR} RUNTIME DESTINATION bin - INCLUDES DESTINATION include - FILE_SET HEADERS DESTINATION include + INCLUDES DESTINATION ${CMAKE_INSTALL_INCLUDEDIR} + FILE_SET HEADERS DESTINATION ${CMAKE_INSTALL_INCLUDEDIR} ) include(CMakePackageConfigHelpers) @@ -38,7 +38,7 @@ configure_file( # Define where to install the config files and install the targets before the # config files. This is so that the config files can actually find the targets -set(ConfigPackageLocation lib/cmake/respond) +set(ConfigPackageLocation ${CMAKE_INSTALL_LIBDIR}/cmake/respond) install( EXPORT respondTargets FILE respondTargets.cmake diff --git a/docs/src/api-guide.md b/docs/src/api-guide.md index 91d59034..f6556512 100644 --- a/docs/src/api-guide.md +++ b/docs/src/api-guide.md @@ -12,6 +12,45 @@ The RESPOND library provides a flexible framework for building opioid use disord - **Transition**: Abstract base for specific transition types - **History**: Tracks state vectors over time +### Runtime Configuration + +`RuntimeConfig` groups the logging and execution settings used by `Simulation` +and `Model`. Use the configuration-based constructors and factory overloads for +new code. + +```cpp +#include + +respond::RuntimeConfig runtime_config; +runtime_config.logging.logger_name = "respond"; +runtime_config.logging.file_path = "respond.log"; +runtime_config.logging.use_shared_sink = true; + +runtime_config.execution.run_models_concurrently = true; +runtime_config.execution.total_threads = 4; +runtime_config.execution.eigen_threads = 1; +``` + +When multiple models run concurrently, keep `eigen_threads` at `1` because +Eigen's worker setting is process-global. For sequential execution, including a +single-model simulation, `eigen_threads` may be greater than `1`. + +### Migrating from Legacy Constructors + +The string-based `Model::Create`, `Simulation`, `Timestep`, and transition +factory overloads are deprecated. Replace separate logger-name and file-path +arguments with `LoggingConfig`, and pass it through `RuntimeConfig` when both +logging and execution settings are needed: + +```cpp +// Legacy: +// respond::Simulation sim("app", "simulation.log"); + +respond::RuntimeConfig config; +config.logging = {"app", "simulation.log", false}; +respond::Simulation sim(config); +``` + ## Core Concepts ### State Vectors @@ -37,12 +76,14 @@ History objects record state vectors at timesteps, enabling analysis of state tr The Model class is the abstract base for all models in RESPOND. ```cpp -#include -#include -#include +#include + +respond::RuntimeConfig runtime_config; +runtime_config.logging.logger_name = "model_logger"; +runtime_config.logging.file_path = "model.log"; // Create a model -auto model = respond::Model::Create("markov", "logger_name"); +auto model = respond::Model::Create("markov", runtime_config); // Set the initial state Eigen::VectorXd initial_state(50); @@ -50,11 +91,12 @@ initial_state.setZero(); model->SetState(initial_state); // Build one timestep with transitions -respond::Timestep step("logger_name"); +respond::Timestep step(runtime_config.logging); auto &behavior_transition = step.CreateTransition("behavior"); behavior_transition->AddMatrix(some_matrix); -auto migration_transition = respond::Transition::Create("migration"); +auto migration_transition = respond::Transition::Create( + "migration", "migration", runtime_config.logging); step.AddTransition(migration_transition); // Mutable index access to owned transition slots @@ -91,14 +133,22 @@ auto histories = model->GetHistories(); The Simulation class manages multiple models and coordinates their execution. ```cpp -#include +#include -// Create a simulation -respond::Simulation sim("my_logger"); +respond::RuntimeConfig runtime_config; +runtime_config.logging.logger_name = "simulation_logger"; +runtime_config.logging.file_path = "simulation.log"; +runtime_config.logging.use_shared_sink = true; +runtime_config.execution.run_models_concurrently = true; +runtime_config.execution.total_threads = 2; +runtime_config.execution.eigen_threads = 1; + +// Create a simulation with explicit logging and execution settings +respond::Simulation sim(runtime_config); // Add models -auto model1 = respond::Model::Create("model1", "my_logger"); -auto model2 = respond::Model::Create("model2", "my_logger"); +auto model1 = respond::Model::Create("model1", runtime_config); +auto model2 = respond::Model::Create("model2", runtime_config); sim.AddModel(model1); sim.AddModel(model2); @@ -138,17 +188,19 @@ The Timestep class owns transitions for one model step and supports both transition creation and clone-based insertion. ```cpp -#include -#include +#include -respond::Timestep step("my_logger"); +respond::LoggingConfig logging_config{ + "timestep_logger", "timestep.log", false}; +respond::Timestep step(logging_config); // Build transition in-place auto &behavior = step.CreateTransition("behavior"); behavior->AddMatrix(behavior_matrix); // Add an existing transition by clone -auto migration = respond::Transition::Create("migration"); +auto migration = respond::Transition::Create( + "migration", "migration", logging_config); step.AddTransition(migration); // Mutable slot access (in-place edits) @@ -233,13 +285,16 @@ hist.Clear(); The Transition class is abstract; use `Transition::Create(...)` to create concrete instances. ```cpp -#include +#include + +respond::LoggingConfig logging_config{ + "transition_logger", "transitions.log", false}; // Create a transition auto transition = respond::Transition::Create( "behavior", // Type "behavior_name", // Instance name - "my_logger" // Logger name + logging_config // Logger configuration ); // Add transformation matrices @@ -269,42 +324,41 @@ transition->ClearMatrices(); ## Logging Integration -RESPOND uses the spdlog library for logging. Models and transitions accept a logger name: +RESPOND uses the spdlog library for logging. Models, timesteps, and transitions +accept `LoggingConfig`, which specifies the logger name, file path, and whether +the logger uses a shared sink: ```cpp -// All logging is handled by passing logger names -auto model = respond::Model::Create("my_model", "my_logger"); - -// The model will use this logger for any errors or warnings -// Create loggers separately using respond::CreateFileLogger -respond::CreateFileLogger("my_logger", "path/to/logfile.log"); +respond::LoggingConfig logging_config{ + "my_logger", "path/to/logfile.log", false}; +auto model = respond::Model::Create( + "my_model", respond::RuntimeConfig{{}, logging_config}); + +// Reusing the same logger name and destination is idempotent. Reusing the +// name with a different destination returns CreationStatus::kError. +respond::ConfigureLogger(logging_config); ``` ## Complete Example ```cpp -#include -#include -#include -#include +#include int main() { - // Create logger - respond::CreateFileLogger("app", "simulation.log"); + respond::RuntimeConfig config; + config.logging = {"app", "simulation.log", false}; - // Create simulation - respond::Simulation sim("app"); + // Create simulation and model with the same runtime settings + respond::Simulation sim(config); + auto model = respond::Model::Create("markov", config); - // Create and configure a model - auto model = respond::Model::Create("markov", "app"); - // Set initial state (e.g., 1000 individuals across 50 states) Eigen::VectorXd initial_state = Eigen::VectorXd::Zero(50); initial_state(0) = 1000; // All in first state model->SetState(initial_state); // Create a reusable timestep with transitions - respond::Timestep step("app"); + respond::Timestep step(config.logging); auto &behavior_transition = step.CreateTransition("behavior"); // behavior_transition->AddMatrix(...); @@ -359,9 +413,12 @@ RESPOND uses `std::unique_ptr` for ownership management: ```cpp for (int run = 0; run < num_runs; ++run) { - respond::Simulation sim("logger_" + std::to_string(run)); - - auto model = respond::Model::Create("markov", "logger_" + std::to_string(run)); + respond::RuntimeConfig config; + config.logging.logger_name = "logger_" + std::to_string(run); + config.logging.file_path = "run_" + std::to_string(run) + ".log"; + respond::Simulation sim(config); + + auto model = respond::Model::Create("markov", config); // Configure model... sim.AddModel(model); @@ -386,7 +443,9 @@ model->CreateDefaultHistories(); ### Copying Simulations ```cpp -respond::Simulation sim1("logger"); +respond::RuntimeConfig config; +config.logging.logger_name = "logger"; +respond::Simulation sim1(config); // ... configure sim1 ... // Create independent copy @@ -402,20 +461,18 @@ When running multiple models in parallel, all loggers can safely write to the sa ### Basic Parallel Logging Setup ```cpp -#include -#include +#include #include #include int main() { - // Configure shared logging (all loggers write to same file) + // Configure shared logging (all loggers write to the same file) + respond::ConfigureLogger({"model_1", "unified.log", true}); respond::SetLogPattern(respond::LogPattern::kThreadSafe); respond::SetFlushInterval(3); // Auto-flush every 3 seconds - // Create multiple loggers that share the same file sink - respond::CreateSharedLogger("model_1"); - respond::CreateSharedLogger("model_2"); - respond::CreateSharedLogger("model_3"); + respond::ConfigureLogger({"model_2", "unified.log", true}); + respond::ConfigureLogger({"model_3", "unified.log", true}); // Now multiple threads can safely write to shared log return 0; @@ -425,20 +482,19 @@ int main() { ### Running Models in Parallel with Unified Logging ```cpp -#include -#include +#include #include #include void RunSimulation(int id, const std::string& log_file) { std::string logger_name = "model_" + std::to_string(id); - // Create logger that uses shared sink - respond::CreateSharedLogger(logger_name); - + respond::RuntimeConfig config; + config.logging = {logger_name, log_file, true}; + // Create and run simulation - respond::Simulation sim(logger_name); - auto model = respond::Model::Create("markov", logger_name); + respond::Simulation sim(config); + auto model = respond::Model::Create("markov", config); // Configure model... Eigen::VectorXd initial_state = Eigen::VectorXd::Zero(50); @@ -495,7 +551,7 @@ respond::SetLogPattern(respond::LogPattern::kDetailed); auto current = respond::GetLogPattern(); // Get pattern as string for programmatic use -std::string pattern_str = respond::LoggingConfig::GetPatternString(current); +// The active pattern is available through GetLogPattern(). ``` ### Monitoring Shared Loggers @@ -509,7 +565,7 @@ std::string info = respond::GetLoggerInfo("model_1"); // Returns: "Logger: model_1\n Level: debug\n Sinks: 1" // Set individual logger level -respond::SetLoggerLevel("model_1", spdlog::level::info); +respond::SetLoggerLevel("model_1", 2); // 2 = info // Flush all loggers immediately respond::FlushAllLoggers(); @@ -517,23 +573,27 @@ respond::FlushAllLoggers(); ### Thread-Safe File Sink Management -The `CreateSharedFileSink` function creates file sinks that are automatically cached and reused: +Shared sinks are automatically cached and reused by `ConfigureLogger` when +`LoggingConfig::use_shared_sink` is `true`: ```cpp -// Create or get cached sink for filepath -auto sink = respond::CreateSharedFileSink("logs/simulation.log"); -// If called again with same path, returns existing sink (no duplicate file handles) +respond::ConfigureLogger({"logger_1", "logs/simulation.log", true}); +respond::ConfigureLogger({"logger_2", "logs/simulation.log", true}); -// Multiple loggers using same sink (no file conflicts) -respond::CreateSharedLogger("logger_1"); // Uses default sink -respond::CreateSharedLogger("logger_2"); // Uses same sink +// Both loggers use the cached sink for the same path. // Both logger_1 and logger_2 write to same file safely ``` +The legacy `CreateSharedFileSink` and `CreateSharedLogger` functions remain +available for existing code. New code should use `ConfigureLogger` so the +destination is explicit. Reusing a logger name with the same destination +returns `CreationStatus::kExists`; using a different destination returns +`CreationStatus::kError`. + ### Best Practices for Parallel Logging 1. **Call `SetLogPattern()` once** at program startup, before creating any loggers -2. **Call `CreateSharedLogger()` instead of `CreateFileLogger()`** when using parallel execution +2. **Set `LoggingConfig::use_shared_sink` to `true`** when parallel loggers should write to one file 3. **Use `kThreadSafe` pattern** when logs will have high concurrent write volume 4. **Set `FlushInterval(0)`** for critical logging; use `FlushInterval(3-5)` for performance 5. **Call `FlushAllLoggers()`** at end of main before exit to ensure all writes complete @@ -543,7 +603,8 @@ respond::CreateSharedLogger("logger_2"); // Uses same sink - **Assertion failures**: Ensure matrix dimensions match state vector size before adding to transitions - **Empty histories**: Call `CreateDefaultHistories()` after model setup or manually add histories -- **Logger errors**: Ensure logger names exist (create with `CreateFileLogger` if needed) +- **Logger errors**: Ensure each logger name has one consistent destination; use + `ConfigureLogger` with the intended `LoggingConfig` - **Memory issues**: Verify no circular unique_ptr references; models own transitions For more information, see the [Doxygen-generated API documentation](../doxygen/html/index.html) or the [Architecture and Design guide](architecture.md). diff --git a/include/respond/cost_effectiveness.hpp b/include/respond/cost_effectiveness.hpp index c5a2f8a9..ec6617cd 100644 --- a/include/respond/cost_effectiveness.hpp +++ b/include/respond/cost_effectiveness.hpp @@ -4,7 +4,7 @@ // Created Date: 2025-08-05 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-09 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2025-2026 Syndemics Lab at Boston Medical Center // @@ -19,7 +19,7 @@ namespace respond { -/// @brief A function to calculate the discoutn for the given data. +/// @brief A function to calculate the discount for the given data. /// @param data Data to be discounted. /// @param discountRate Discount rate to apply to the data. /// @param N Number of weeks to discount over. @@ -32,7 +32,7 @@ inline Eigen::VectorXd Discount(const Eigen::Ref &data, double discount = (is_discrete) ? (1 / pow((1.0 + (discount_rate) / total_weeks), week)) : (exp(-discount_rate * (week / total_weeks))); - return data - Eigen::VectorXd::Constant(data.size(), discount); + return data * discount; } /// @brief A cwise multiplier. This is used for all costs and mult utility. diff --git a/include/respond/history.hpp b/include/respond/history.hpp index a2e8ea1f..b585b0c4 100644 --- a/include/respond/history.hpp +++ b/include/respond/history.hpp @@ -16,6 +16,7 @@ #include #include +#include #include #include #include @@ -171,25 +172,28 @@ class History { /// @param state The state vector to record. /// @param timestep The timestep index for this state (default: -1 for /// automatic next timestep). If timestep is negative, the next sequential - /// timestep is used automatically. If timestep already exists, it is - /// considered invalid but is currently overwritten. + /// timestep is used automatically. Explicit timesteps are kept in + /// ascending order; inserting an earlier timestep shifts later records. + /// If timestep already exists, its state is overwritten. void AddState(const Eigen::Ref &state, int timestep = -1) { if (timestep < 0) { timestep = GetNextTimestep(); } - const auto existing = - std::find(_timesteps.begin(), _timesteps.end(), timestep); - if (existing != _timesteps.end()) { - const auto index = - static_cast(existing - _timesteps.begin()); + const auto insertion_point = + std::lower_bound(_timesteps.begin(), _timesteps.end(), timestep); + const auto index = static_cast( + insertion_point - _timesteps.begin()); + if (insertion_point != _timesteps.end() && + *insertion_point == timestep) { _states[index] = state; return; } - _timesteps.push_back(timestep); - _states.push_back(state); + _timesteps.insert(insertion_point, timestep); + _states.insert(_states.begin() + static_cast(index), + state); } /// @brief Adds a contribution to an accumulated history. diff --git a/include/respond/logging.hpp b/include/respond/logging.hpp index 67ae6990..d87bcd4d 100644 --- a/include/respond/logging.hpp +++ b/include/respond/logging.hpp @@ -54,13 +54,17 @@ enum class LogPattern : int { /// @param logger_name Unique identifier for this logger. /// @param filepath File path where the logger will write logs. /// @return CreationStatus indicating the result of logger creation. -/// @note If a logger with the same name already exists, kExists is returned. +/// @note If a logger with the same name and destination already exists, +/// kExists is returned. If the existing logger uses a different destination, +/// kError is returned. CreationStatus CreateFileLogger(const std::string &logger_name, const std::string &filepath); /// @brief Initializes a logger from a logging configuration. /// @param config Logging name, destination, and shared-sink policy. /// @return CreationStatus indicating the result of logger creation. +/// @note Reusing a logger name with the same destination and sink policy +/// returns kExists. A different destination or sink policy returns kError. CreationStatus ConfigureLogger(const LoggingConfig &config); // ============================================================================ @@ -85,6 +89,7 @@ CreationStatus CreateSharedFileSink(const std::string &filepath); /// @note Requires CreateSharedFileSink() to be called first with a file path. /// @note If CreateSharedFileSink() wasn't called, creates a default sink to /// "respond.log". +/// @note Reusing a logger name with a different sink returns kError. CreationStatus CreateSharedLogger(const std::string &logger_name); /// @brief Sets the logging pattern template for all subsequent logger diff --git a/src/internals/logging_internals.hpp b/src/internals/logging_internals.hpp index db0a88ca..f4164fb1 100644 --- a/src/internals/logging_internals.hpp +++ b/src/internals/logging_internals.hpp @@ -36,8 +36,11 @@ class LoggingRegistry { } static std::shared_ptr - GetSharedSink(const std::string &filepath) { + GetSharedSink(const std::string &filepath, bool *created = nullptr) { std::lock_guard lock(GetInstance().sink_mutex_); + if (created) { + *created = false; + } auto key = filepath; if (GetInstance().shared_sinks_.find(key) == GetInstance().shared_sinks_.end()) { @@ -45,6 +48,9 @@ class LoggingRegistry { GetInstance().shared_sinks_[key] = std::make_shared( filepath, false); + if (created) { + *created = true; + } } catch (const spdlog::spdlog_ex &ex) { std::cerr << "Failed to create shared sink: " << ex.what() << std::endl; @@ -54,9 +60,13 @@ class LoggingRegistry { return GetInstance().shared_sinks_[key]; } - static LogPattern GetPattern() { return GetInstance().current_pattern_; } + static LogPattern GetPattern() { + std::lock_guard lock(GetInstance().config_mutex_); + return GetInstance().current_pattern_; + } static void SetPattern(LogPattern pattern) { + std::lock_guard lock(GetInstance().config_mutex_); GetInstance().current_pattern_ = pattern; } @@ -75,9 +85,13 @@ class LoggingRegistry { } } - static int GetFlushInterval() { return GetInstance().flush_interval_; } + static int GetFlushInterval() { + std::lock_guard lock(GetInstance().config_mutex_); + return GetInstance().flush_interval_; + } static void SetFlushInterval(int seconds) { + std::lock_guard lock(GetInstance().config_mutex_); GetInstance().flush_interval_ = seconds; } diff --git a/src/logging.cpp b/src/logging.cpp index 337c2a7d..c136903e 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -14,21 +14,68 @@ #include "internals/logging_internals.hpp" +#include #include namespace respond { namespace { -CreationStatus CreateSharedLogger( - const std::string &logger_name, +bool LoggerUsesFile(const std::shared_ptr &logger, + const std::string &filepath) { + if (!logger) { + return false; + } + + for (const auto &sink : logger->sinks()) { + auto file_sink = + std::dynamic_pointer_cast(sink); + if (file_sink && + std::filesystem::path(file_sink->filename()) == + std::filesystem::path(filepath)) { + return true; + } + } + return false; +} + +bool LoggerUsesSink( + const std::shared_ptr &logger, const std::shared_ptr &sink) { - if (CheckIfExists(logger_name) == CreationStatus::kExists) { - std::cout << "Shared logger " << logger_name << " already exists" + if (!logger || !sink) { + return false; + } + + for (const auto &logger_sink : logger->sinks()) { + if (logger_sink == sink) { + return true; + } + } + return false; +} + +CreationStatus ExistingLoggerStatus(const std::string &logger_name, + bool same_configuration) { + if (same_configuration) { + std::cout << "Logger " << logger_name << " already exists" << std::endl; return CreationStatus::kExists; } + std::string error_msg = "Logger '" + logger_name + + "' already exists with a different destination"; + std::cerr << error_msg << std::endl; + return CreationStatus::kError; +} + +CreationStatus CreateSharedLogger( + const std::string &logger_name, + const std::shared_ptr &sink) { + if (auto existing_logger = spdlog::get(logger_name)) { + return ExistingLoggerStatus(logger_name, + LoggerUsesSink(existing_logger, sink)); + } + if (!sink) { std::string error_msg = "Failed to create shared logger '" + logger_name + @@ -61,8 +108,9 @@ CreationStatus CreateSharedLogger( CreationStatus CreateFileLogger(const std::string &logger_name, const std::string &filepath) { - if (CheckIfExists(logger_name) == CreationStatus::kExists) { - return CreationStatus::kExists; + if (auto existing_logger = spdlog::get(logger_name)) { + return ExistingLoggerStatus(logger_name, + LoggerUsesFile(existing_logger, filepath)); } try { spdlog::cfg::load_env_levels(); @@ -83,10 +131,12 @@ CreationStatus CreateFileLogger(const std::string &logger_name, CreationStatus CreateSharedFileSink(const std::string &filepath) { try { - auto sink = LoggingRegistry::GetSharedSink(filepath); + bool created = false; + auto sink = LoggingRegistry::GetSharedSink(filepath, &created); if (sink) { LoggingRegistry::SetDefaultSinkPath(filepath); - return CreationStatus::kSuccess; + return created ? CreationStatus::kSuccess + : CreationStatus::kExists; } std::string error_msg = "Failed to create shared file sink: sink is null"; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 84aba9c2..ac9a3e62 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -2,7 +2,7 @@ cmake_minimum_required(VERSION 3.27) project(respond_tests LANGUAGES CXX) -set(CMAKE_CXX_STANDARD 17 CACHE STRING "The C++ standard to compile with, 17") +set(CMAKE_CXX_STANDARD 20 CACHE STRING "The C++ standard to compile with, 20") set(CMAKE_CXX_STANDARD_REQUIRED ON) set(CMAKE_CXX_EXTENSIONS OFF) if(CMAKE_SYSTEM_NAME MATCHES "CYGWIN" OR CMAKE_SYSTEM_NAME MATCHES "MSYS" OR CMAKE_SYSTEM_NAME MATCHES "MINGW") diff --git a/tests/unit/cost_effectiveness_test.cpp b/tests/unit/cost_effectiveness_test.cpp index 8eeaf7ff..439b4055 100644 --- a/tests/unit/cost_effectiveness_test.cpp +++ b/tests/unit/cost_effectiveness_test.cpp @@ -42,7 +42,7 @@ class CostEffectivenessTest : public ::testing::Test { TEST_F(CostEffectivenessTest, ContinuousDiscount) { double discount = exp(-discount_rate * (week / total_weeks)); - auto expected = data - Eigen::VectorXd::Constant(data.size(), discount); + auto expected = data * discount; auto result = Discount(data, discount_rate, week, is_discrete, total_weeks); ASSERT_TRUE(expected.isApprox(result)); @@ -52,7 +52,7 @@ TEST_F(CostEffectivenessTest, DiscreteDiscount) { is_discrete = true; double discount = 1 / pow((1.0 + (discount_rate) / total_weeks), week); - auto expected = data - Eigen::VectorXd::Constant(data.size(), discount); + auto expected = data * discount; auto result = Discount(data, discount_rate, week, is_discrete, total_weeks); ASSERT_TRUE(expected.isApprox(result)); @@ -88,7 +88,7 @@ TEST_F(CostEffectivenessTest, CalculateLifeYearsDiscountHistory) { double result = CalculateLifeYears(h, true, discount_rate, 1.0); double discount = 1.0; - auto discounted_result = v - Eigen::VectorXd::Constant(v.size(), discount); + auto discounted_result = v * discount; double expected = (discounted_result.sum()) / 52.0; ASSERT_DOUBLE_EQ(result, expected); diff --git a/tests/unit/history_test.cpp b/tests/unit/history_test.cpp index 21002e1f..bfefce2d 100644 --- a/tests/unit/history_test.cpp +++ b/tests/unit/history_test.cpp @@ -368,6 +368,24 @@ TEST_F(HistoryTest, GetLatestRecordedTimestep) { EXPECT_EQ(history.GetLatestRecordedTimestep(), 10); } +TEST_F(HistoryTest, OutOfOrderStateIsInsertedChronologically) { + History history("test_history"); + Eigen::VectorXd state_at_two = Eigen::VectorXd::Constant(1, 2.0); + Eigen::VectorXd state_at_five = Eigen::VectorXd::Constant(1, 5.0); + + history.AddState(state_at_five, 5); + history.AddState(state_at_two, 2); + + ASSERT_EQ(history.GetRecordedTimesteps(), + (std::vector{2, 5})); + ASSERT_EQ(history.GetLatestRecordedTimestep(), 5); + + const auto states = history.GetStateAsVector(); + ASSERT_EQ(states.size(), 6); + EXPECT_EQ(states[2](0), 2.0); + EXPECT_EQ(states[5](0), 5.0); +} + TEST_F(HistoryTest, GetLatestRecordedTimestepOnEmptyHistoryReturnsNegativeOne) { History history("state"); EXPECT_EQ(history.GetLatestRecordedTimestep(), -1); diff --git a/tests/unit/logging_test.cpp b/tests/unit/logging_test.cpp index b73b83bf..195ecb89 100644 --- a/tests/unit/logging_test.cpp +++ b/tests/unit/logging_test.cpp @@ -109,6 +109,19 @@ TEST_F(LoggingTest, CreateFileLoggerAlreadyExists) { EXPECT_EQ(status, CreationStatus::kExists); } +TEST_F(LoggingTest, CreateFileLoggerRejectsDifferentDestination) { + const std::string other_log_file = "/tmp/respond_other_test.log"; + std::remove(other_log_file.c_str()); + ASSERT_EQ(CreateFileLogger("test_logger", test_log_file_), + CreationStatus::kSuccess); + + EXPECT_EQ(CreateFileLogger("test_logger", other_log_file), + CreationStatus::kError); + EXPECT_FALSE(FileContains(other_log_file, "unexpected destination")); + + std::remove(other_log_file.c_str()); +} + TEST_F(LoggingTest, CreateMultipleFileLoggers) { CreationStatus status1 = CreateFileLogger("logger1", test_log_file_); CreationStatus status2 = CreateFileLogger("logger2", test_log_file_); @@ -143,6 +156,22 @@ TEST_F(LoggingTest, ConfigureLoggerCreatesSharedLogger) { EXPECT_NE(spdlog::get(config.logger_name), nullptr); } +TEST_F(LoggingTest, ConfigureLoggerRejectsSharedDestinationChange) { + const std::string other_log_file = "/tmp/respond_other_shared.log"; + std::remove(other_log_file.c_str()); + ASSERT_EQ(ConfigureLogger( + LoggingConfig{"configured_shared_logger", shared_log_file_, + true}), + CreationStatus::kSuccess); + + EXPECT_EQ(ConfigureLogger( + LoggingConfig{"configured_shared_logger", other_log_file, + true}), + CreationStatus::kError); + + std::remove(other_log_file.c_str()); +} + TEST_F(LoggingTest, ConcurrentSharedLoggerConfigurationUsesRequestedSinks) { constexpr size_t logger_count = 8; std::vector log_files; @@ -188,7 +217,7 @@ TEST_F(LoggingTest, CreateSharedFileSinkCaching) { // Create sink again with same path (should reuse cached) CreationStatus status2 = CreateSharedFileSink(shared_log_file_); - EXPECT_EQ(status2, CreationStatus::kSuccess); + EXPECT_EQ(status2, CreationStatus::kExists); } TEST_F(LoggingTest, CreateSharedLogger) { @@ -250,6 +279,38 @@ TEST_F(LoggingTest, ChangeLogPatternMultipleTimes) { EXPECT_EQ(GetLogPattern(), LogPattern::kThreadSafe); } +TEST_F(LoggingTest, ConcurrentLogConfigurationAccessIsSafe) { + constexpr int iterations = 1000; + std::vector workers; + + CreateFileLogger("concurrent_config_logger", test_log_file_); + + workers.emplace_back([] { + for (int i = 0; i < iterations; ++i) { + SetLogPattern(i % 2 == 0 ? LogPattern::kSimple + : LogPattern::kDetailed); + } + }); + workers.emplace_back([&] { + for (int i = 0; i < iterations; ++i) { + SetFlushInterval(i % 2); + LogInfo("concurrent_config_logger", "configuration update"); + } + }); + workers.emplace_back([] { + for (int i = 0; i < iterations; ++i) { + static_cast(GetLogPattern()); + } + }); + + for (auto &worker : workers) { + worker.join(); + } + + EXPECT_TRUE(GetLogPattern() == LogPattern::kSimple || + GetLogPattern() == LogPattern::kDetailed); +} + // ============================================================================ // Test: Flush Interval Configuration // ============================================================================ diff --git a/tests/unit/markov_test.cpp b/tests/unit/markov_test.cpp index 81aae369..d707926a 100644 --- a/tests/unit/markov_test.cpp +++ b/tests/unit/markov_test.cpp @@ -85,7 +85,7 @@ class MarkovTest : public ::testing::Test { TEST_F(MarkovTest, CreateMarkovModel) { auto markov = Model::Create("markov"); ASSERT_NE(markov, nullptr); - ASSERT_EQ(CreateFileLogger(RESPOND_DEFAULT_LOG, ""), + ASSERT_EQ(CreateFileLogger(RESPOND_DEFAULT_LOG, default_log_file_), CreationStatus::kExists); } @@ -96,7 +96,7 @@ TEST_F(MarkovTest, CreateMarkovModelProcessorCount) { : 1; auto markov = Model::Create("markov", processor_count); ASSERT_NE(markov, nullptr); - ASSERT_EQ(CreateFileLogger(RESPOND_DEFAULT_LOG, ""), + ASSERT_EQ(CreateFileLogger(RESPOND_DEFAULT_LOG, default_log_file_), CreationStatus::kExists); } diff --git a/tests/unit/timestep_test.cpp b/tests/unit/timestep_test.cpp index b9d81d75..e5efde70 100644 --- a/tests/unit/timestep_test.cpp +++ b/tests/unit/timestep_test.cpp @@ -52,14 +52,15 @@ class TimestepTest : public ::testing::Test { TEST_F(TimestepTest, DefaultConstructor) { Timestep ts; - ASSERT_EQ(CreateFileLogger(RESPOND_DEFAULT_LOG, ""), + ASSERT_EQ(CreateFileLogger(RESPOND_DEFAULT_LOG, default_log_file_), CreationStatus::kExists); } TEST_F(TimestepTest, DefaultConstructorWithLogName) { std::string log_name = "temp"; Timestep ts(log_name); - ASSERT_EQ(CreateFileLogger(log_name, ""), CreationStatus::kExists); + ASSERT_EQ(CreateFileLogger(log_name, default_log_file_), + CreationStatus::kExists); } TEST_F(TimestepTest, DefaultConstructorWithLogNameAndFile) { From 3d8e449be704131a08f7feee59dd2f3c98d41763 Mon Sep 17 00:00:00 2001 From: Matthew Carroll <28577806+MJC598@users.noreply.github.com> Date: Thu, 24 Sep 2026 11:17:52 -0400 Subject: [PATCH 07/11] updating documentation --- docs/src/api-guide.md | 5 +++-- docs/src/architecture.md | 23 +++++++++++++---------- include/respond/logging.hpp | 14 +++++++------- 3 files changed, 23 insertions(+), 19 deletions(-) diff --git a/docs/src/api-guide.md b/docs/src/api-guide.md index f6556512..8762372a 100644 --- a/docs/src/api-guide.md +++ b/docs/src/api-guide.md @@ -469,7 +469,7 @@ int main() { // Configure shared logging (all loggers write to the same file) respond::ConfigureLogger({"model_1", "unified.log", true}); respond::SetLogPattern(respond::LogPattern::kThreadSafe); - respond::SetFlushInterval(3); // Auto-flush every 3 seconds + respond::SetFlushInterval(0); // Flush each log message immediately respond::ConfigureLogger({"model_2", "unified.log", true}); respond::ConfigureLogger({"model_3", "unified.log", true}); @@ -566,6 +566,7 @@ std::string info = respond::GetLoggerInfo("model_1"); // Set individual logger level respond::SetLoggerLevel("model_1", 2); // 2 = info +// SetLoggerLevel returns void and does nothing if the logger is not found. // Flush all loggers immediately respond::FlushAllLoggers(); @@ -595,7 +596,7 @@ returns `CreationStatus::kExists`; using a different destination returns 1. **Call `SetLogPattern()` once** at program startup, before creating any loggers 2. **Set `LoggingConfig::use_shared_sink` to `true`** when parallel loggers should write to one file 3. **Use `kThreadSafe` pattern** when logs will have high concurrent write volume -4. **Set `FlushInterval(0)`** for critical logging; use `FlushInterval(3-5)` for performance +4. **Set `FlushInterval(0)`** when each message must be flushed immediately; positive values are currently reserved and do not enable periodic flushing 5. **Call `FlushAllLoggers()`** at end of main before exit to ensure all writes complete 6. **Monitor logger levels** with `GetLoggerInfo()` when debugging multi-model runs diff --git a/docs/src/architecture.md b/docs/src/architecture.md index 75ce7ce6..343a4308 100644 --- a/docs/src/architecture.md +++ b/docs/src/architecture.md @@ -239,16 +239,19 @@ if (type == "custom") { ## Threading and Concurrency -Current implementation is **not thread-safe**: - -- No internal locking mechanisms -- State modification is not atomic -- Multiple simulations can run independently (each with own state) - -For concurrent execution: -- Create separate Simulation instances -- Each thread manages its own simulation -- Synchronize result collection externally +Simulation execution has explicit concurrency boundaries: + +- `Simulation::Run()` may execute independent models concurrently when its + execution configuration enables it. +- Models run independently, but a `Simulation` instance must not be mutated or + queried for results concurrently with `Run()`; callers must provide external + synchronization for that access. +- Logging configuration and logger operations provide internal synchronization + for concurrent logging, including multiple loggers using a shared sink. + +For concurrent execution, configure the worker pool through `ExecutionConfig`, +keep Eigen worker settings at one when models run concurrently, and synchronize +any shared simulation mutation or result collection externally. ## Performance Considerations diff --git a/include/respond/logging.hpp b/include/respond/logging.hpp index d87bcd4d..f8522895 100644 --- a/include/respond/logging.hpp +++ b/include/respond/logging.hpp @@ -103,9 +103,9 @@ void SetLogPattern(LogPattern pattern); /// @return The active LogPattern enum value. LogPattern GetLogPattern(); -/// @brief Sets the global flush interval for automatic buffer flushing. -/// @param seconds Interval in seconds for automatic flush (0 to disable -/// auto-flush). +/// @brief Sets the flush policy used by logging calls. +/// @param seconds Use 0 to flush each log message immediately. Positive values +/// are stored for compatibility but do not currently enable periodic flushing. /// @note Thread-safe configuration change. void SetFlushInterval(int seconds); @@ -153,9 +153,10 @@ void LogDebug(const std::string &logger_name, const std::string &message); /// @note Thread-safe query. CreationStatus CheckLoggerExists(const std::string &logger_name); -/// @brief Retrieve detailed information about a logger. +/// @brief Retrieve basic information about a logger. /// @param logger_name Logger identifier to query. -/// @return String containing logger name, file path, level, and thread info. +/// @return String containing the logger name, level, and sink count, or a +/// not-found message. /// @note Thread-safe operation. std::string GetLoggerInfo(const std::string &logger_name); @@ -163,8 +164,7 @@ std::string GetLoggerInfo(const std::string &logger_name); /// @param logger_name Logger identifier to configure. /// @param level Log level: 0=trace, 1=debug, 2=info, 3=warn, 4=error, /// 5=critical. -/// @return CreationStatus::kSuccess if level was set, kNotCreated if logger -/// doesn't exist. +/// @note Does nothing when the logger does not exist. void SetLoggerLevel(const std::string &logger_name, int level); } // namespace respond From 3041910dc83f6321aa5e8ff3dfa2e4a4b1f3079a Mon Sep 17 00:00:00 2001 From: Matthew Carroll <28577806+MJC598@users.noreply.github.com> Date: Thu, 24 Sep 2026 12:05:29 -0400 Subject: [PATCH 08/11] Updating documentation for true behavior --- docs/src/api-guide.md | 6 +++++ docs/src/limitations.md | 14 +++++++++-- include/respond/model.hpp | 25 +++++++++--------- include/respond/simulation.hpp | 23 ++++++++++++++--- include/respond/timestep.hpp | 9 ++++--- src/internals/markov.hpp | 11 +++----- tests/unit/markov_test.cpp | 28 ++++++++++++++++++++- tests/unit/simulation_test.cpp | 46 ++++++++++++++++++++++++++++++++++ tests/unit/timestep_test.cpp | 8 ++++++ 9 files changed, 139 insertions(+), 31 deletions(-) diff --git a/docs/src/api-guide.md b/docs/src/api-guide.md index 8762372a..8f833593 100644 --- a/docs/src/api-guide.md +++ b/docs/src/api-guide.md @@ -113,6 +113,11 @@ Eigen::VectorXd current_state = model->GetState(); auto histories = model->GetHistories(); ``` +`Model::Create()` currently creates the Markov implementation. Its string +argument is an instance name, not a model-type selector, and any string is +accepted as that name. Additional model implementations may be exposed by the +factory in the future. + ### Key Methods - `SetState(const Eigen::Ref &state)`: Sets the model's state vector @@ -171,6 +176,7 @@ auto history_names = sim.GetModelHistoryNames(0); - `Run(int duration = -1)`: Runs all models for the configured duration - `SetDuration(int duration)`: Sets default duration used by `Run()` when no argument is provided +- `CreateNewModel(const std::string &name)`: Creates and manages a model, then returns an editable deep copy; assign the edited copy through `sim[idx]` - `AddModel(const std::unique_ptr &model)`: Adds a model (cloned internally) - `operator[](size_t idx)`: Mutable index access to owned model slot (`sim[idx]->Method()`) - `operator[](size_t idx) const`: Const index access to owned model diff --git a/docs/src/limitations.md b/docs/src/limitations.md index 79a986b5..c40f3474 100644 --- a/docs/src/limitations.md +++ b/docs/src/limitations.md @@ -6,10 +6,20 @@ RESPOND is under active development. The following limitations reflect the curre The core API is stable enough for integration, but there are important constraints: -- `Model::Create(...)` currently returns the Markov implementation; additional model families are not yet exposed through the public factory. +- `Model::Create(...)` currently returns the Markov implementation; its string + argument is an instance name, not a model-type selector. Additional model + families are not yet exposed through the public factory. - Execution is timestep-driven; users must construct timesteps and transitions explicitly. - Transition creation is string-based (`Transition::Create(...)`), so invalid type names fail at runtime. -- The library is not internally synchronized for shared mutable use across threads. +- `Simulation` mutation and result access are not synchronized. Callers must + externally synchronize configuration, model mutation, and result access when + sharing a simulation across threads, and must not mutate or inspect models + while `Simulation::Run()` is executing. +- `Simulation::Run()` may execute independent models concurrently when enabled + through `ExecutionConfig`; the configured Eigen worker limit must still be + observed because Eigen's worker setting is process-global. +- Logging registries and shared sinks are internally synchronized for + concurrent logger creation and logging. - GPU execution is not supported. - Legacy standalone executable workflows are maintained separately from the modern library API. diff --git a/include/respond/model.hpp b/include/respond/model.hpp index b44fb473..aebcf94a 100644 --- a/include/respond/model.hpp +++ b/include/respond/model.hpp @@ -37,11 +37,10 @@ class Model { //////////////////////////////////////////////////////////////////////////// /// @brief Factory method to create a Model instance. - /// @details This method creates a new instance of a Model subclass based on - /// the provided name. It initializes logging for the model and returns a - /// unique_ptr to the created instance. Throws an exception if the model - /// name is unsupported. - /// @param name The name identifier for the model to create. + /// @details This method creates a Markov model instance and uses the + /// provided name as its instance identifier. It initializes logging for + /// the model and returns a unique_ptr to the created instance. + /// @param name The instance name for the model. /// @param log_name Name of the logger for this model (default: "console"). /// @param log_filepath File path for the log file (default: "respond.log"). /// @return A unique_ptr to the newly created Model instance. @@ -52,12 +51,11 @@ class Model { const std::string &log_filepath = RESPOND_DEFAULT_LOG_FILE); /// @brief Alternate factory method to create a Model instance. - /// @details This method creates a new instance of a Model subclass based on - /// the provided name. It sets the number of threads to be used by the - /// model, initializes logging for the model, and returns a unique_ptr to - /// the created instance. Throws an exception if the model name is - /// unsupported. - /// @param name The name identifier for the model to create. + /// @details This method creates a Markov model instance and uses the + /// provided name as its instance identifier. It sets the number of threads + /// to be used by the model, initializes logging, and returns a unique_ptr + /// to the created instance. + /// @param name The instance name for the model. /// @param processor_count The number of threads to be used by the model. /// @param log_name Name of the logger for this model (default: "console"). /// @param log_filepath File path for the log file (default: "respond.log"). @@ -69,7 +67,7 @@ class Model { const std::string &log_filepath = RESPOND_DEFAULT_LOG_FILE); /// @brief Creates a Model with explicit execution settings. - /// @param name The name identifier for the model to create. + /// @param name The instance name for the model. /// @param execution_config Resource settings for model execution. /// @param log_name Name of the logger for this model (default: "console"). /// @param log_filepath File path for the log file (default: "respond.log"). @@ -80,7 +78,8 @@ class Model { const std::string &log_name = RESPOND_DEFAULT_LOG, const std::string &log_filepath = RESPOND_DEFAULT_LOG_FILE); - /// @brief Creates a Model with shared runtime settings. + /// @brief Creates a Markov model with shared runtime settings. + /// @param name The instance name for the model. static std::unique_ptr Create(const std::string &name, const RuntimeConfig &runtime_config); diff --git a/include/respond/simulation.hpp b/include/respond/simulation.hpp index 0dbcb982..9258ba9f 100644 --- a/include/respond/simulation.hpp +++ b/include/respond/simulation.hpp @@ -212,8 +212,10 @@ class Simulation { /// @brief Creates a new model instance and adds it to the simulation. /// @param model_name The name identifier for the model to create. This name /// is used to identify the model type and initialize it accordingly. - /// @return A deep-copied model instance representing the newly created - /// model. + /// @return An editable deep copy of the newly created model. Changes to + /// this copy do not affect the model managed by the simulation until it is + /// assigned through the mutable model slot, for example + /// `simulation[0] = *model`. std::unique_ptr CreateNewModel(const std::string &model_name) { _models.push_back(Model::Create(model_name, _runtime_config)); return _models.back()->clone(); @@ -241,11 +243,26 @@ class Simulation { /// limit. /// @throws std::invalid_argument if multiple models would execute in /// parallel while more than one Eigen worker thread is configured. + /// @throws std::invalid_argument if no models have been added. /// @throws Any exception raised by a model after all workers have joined. void Run(int duration = -1) { + if (_models.empty()) { + LogError(_runtime_config.logging.logger_name, + "Cannot run a simulation with no models."); + throw std::invalid_argument( + "Error attempting to run simulation with no models."); + } if (duration > 0) { _duration = duration; } + const auto &execution = _runtime_config.execution; + const unsigned int thread_limit = std::thread::hardware_concurrency(); + const unsigned int eigen_threads = + thread_limit == 0 + ? execution.eigen_threads + : std::min(execution.eigen_threads, thread_limit); + Eigen::setNbThreads(eigen_threads); + LogInfo(_runtime_config.logging.logger_name, "Running simulation for duration of " + std::to_string(_duration) + " timesteps."); @@ -253,8 +270,6 @@ class Simulation { model->SetFinalTimestep(_duration); model->RunTimesteps(); }; - - const auto &execution = _runtime_config.execution; if (!execution.run_models_concurrently || _models.size() < 2) { for (const auto &model : _models) { run_model(model); diff --git a/include/respond/timestep.hpp b/include/respond/timestep.hpp index d6b647b5..f61d2391 100644 --- a/include/respond/timestep.hpp +++ b/include/respond/timestep.hpp @@ -248,6 +248,7 @@ class Timestep { /// @param transition_name The name of the transition to which the matrix /// will be added. /// @param m The transition matrix to add (not modified by this transition). + /// @throws std::invalid_argument if no transition has the requested name. void AddMatrixToTransition(const std::string &transition_name, const Eigen::Ref &m) { for (size_t i = 0; i < _transitions.size(); ++i) { @@ -256,9 +257,11 @@ class Timestep { return; } } - LogWarning(_log_name, - "Transition not found in AddMatrixToTransition: " + - transition_name); + LogError(_log_name, "Transition not found in AddMatrixToTransition: " + + transition_name); + throw std::invalid_argument( + "Error attempting to AddMatrixToTransition by name: " + + transition_name); } //////////////////////////////////////////////////////////////////////////// diff --git a/src/internals/markov.hpp b/src/internals/markov.hpp index c1c96575..fce22569 100644 --- a/src/internals/markov.hpp +++ b/src/internals/markov.hpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-05 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-09-22 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // @@ -221,17 +221,12 @@ class Markov : public virtual Model { return; } + _current_timestep = static_cast(idx); + auto transitions = _timestep_vector[idx].GetTransitions(); for (const auto &t : transitions) { _state = t->Execute(_state, _histories); } - if (idx != _current_timestep) { - LogWarning(_log_name, - "Ran timestep out of order. Current timestep: " + - std::to_string(_current_timestep) + - ", run timestep: " + std::to_string(idx)); - return; - } _current_timestep++; } diff --git a/tests/unit/markov_test.cpp b/tests/unit/markov_test.cpp index d707926a..fdfdd69b 100644 --- a/tests/unit/markov_test.cpp +++ b/tests/unit/markov_test.cpp @@ -317,13 +317,39 @@ TEST_F(MarkovTest, RunTimestepEmptyTimestepVector) { TEST_F(MarkovTest, RunTimestepIndex) { Markov markov("markov", RESPOND_DEFAULT_LOG); + markov.SetState(state); + Timestep timestep1(RESPOND_DEFAULT_LOG); Timestep timestep2(RESPOND_DEFAULT_LOG); + Timestep timestep3(RESPOND_DEFAULT_LOG); + + Eigen::VectorXd timestep1_change(3); + timestep1_change << 1.0, 0.0, 0.0; + timestep1.CreateTransition("migration")->AddMatrix(timestep1_change); + + Eigen::VectorXd timestep2_change(3); + timestep2_change << 10.0, 0.0, 0.0; + timestep2.CreateTransition("migration")->AddMatrix(timestep2_change); + + Eigen::VectorXd timestep3_change(3); + timestep3_change << 0.0, 20.0, 0.0; + timestep3.CreateTransition("migration")->AddMatrix(timestep3_change); + markov.AddTimestep(timestep1); markov.AddTimestep(timestep2); + markov.AddTimestep(timestep3); EXPECT_EQ(markov.GetTimestep(), 0); + markov.RunTimestep(1); - EXPECT_EQ(markov.GetTimestep(), 0); // Current timestep does not change + EXPECT_EQ(markov.GetTimestep(), 2); + Eigen::VectorXd expected_state = state; + expected_state(0) += 10.0; + EXPECT_TRUE(markov.GetState().isApprox(expected_state)); + + markov.RunTimestep(); + EXPECT_EQ(markov.GetTimestep(), 3); + expected_state(1) += 20.0; + EXPECT_TRUE(markov.GetState().isApprox(expected_state)); } TEST_F(MarkovTest, RunTimestepIndexOutOfRange) { diff --git a/tests/unit/simulation_test.cpp b/tests/unit/simulation_test.cpp index c086ba1b..2acb74f3 100644 --- a/tests/unit/simulation_test.cpp +++ b/tests/unit/simulation_test.cpp @@ -83,6 +83,18 @@ TEST_F(SimulationTest, DefaultConstructor) { CreationStatus::kExists); } +TEST_F(SimulationTest, RunRejectsSimulationWithNoModels) { + Simulation simulation; + + try { + simulation.Run(); + FAIL() << "Expected Run() to reject a simulation with no models."; + } catch (const std::invalid_argument &error) { + EXPECT_STREQ(error.what(), + "Error attempting to run simulation with no models."); + } +} + TEST_F(SimulationTest, ConstructorWithLogName) { Simulation s("custom_log"); ASSERT_EQ(CreateFileLogger("custom_log", default_log_file_), @@ -153,6 +165,18 @@ TEST_F(SimulationTest, CreateNewModel) { ASSERT_EQ(s.GetModels().size(), 1); } +TEST_F(SimulationTest, EditsCreatedModelCopyBeforeManagingIt) { + Simulation simulation; + auto model_copy = simulation.CreateNewModel("test_model"); + model_copy->SetFinalTimestep(12); + + EXPECT_EQ(simulation[0]->GetFinalTimestep(), -1); + + simulation[0] = *model_copy; + + EXPECT_EQ(simulation[0]->GetFinalTimestep(), 12); +} + TEST_F(SimulationTest, CreateMultipleModels) { Simulation s; std::string model_name1 = "test_model1"; @@ -351,6 +375,28 @@ TEST_F(SimulationTest, AllowsEigenThreadsForSingleConcurrentModel) { EXPECT_NO_THROW(simulation.Run()); } +TEST_F(SimulationTest, AppliesUpdatedEigenThreadsWhenRunning) { + const int previous_eigen_threads = Eigen::nbThreads(); + ExecutionConfig config; + config.eigen_threads = 2; + Simulation simulation(RuntimeConfig{config, LoggingConfig{}}); + + auto source = std::make_unique>(); + auto model = std::make_unique>(); + EXPECT_CALL(*model, RunTimesteps()).Times(1); + EXPECT_CALL(*source, clone()) + .WillOnce(Return(::testing::ByMove(std::move(model)))); + simulation.AddModel(std::move(source)); + + Eigen::setNbThreads(2); + config.eigen_threads = 1; + simulation.SetExecutionConfig(config); + simulation.Run(); + + EXPECT_EQ(Eigen::nbThreads(), 1); + Eigen::setNbThreads(previous_eigen_threads); +} + TEST_F(SimulationTest, RejectsConcurrentEigenOversubscription) { ExecutionConfig config; config.eigen_threads = 2; diff --git a/tests/unit/timestep_test.cpp b/tests/unit/timestep_test.cpp index e5efde70..0b51fb45 100644 --- a/tests/unit/timestep_test.cpp +++ b/tests/unit/timestep_test.cpp @@ -161,6 +161,14 @@ TEST_F(TimestepTest, AddMatrixToTransitionByName) { ASSERT_TRUE(transition->GetMatrices()[0].isApprox(m)); } +TEST_F(TimestepTest, AddMatrixToTransitionByMissingNameThrows) { + Timestep ts("test_log", test_log_file_); + Eigen::MatrixXd m = Eigen::MatrixXd::Identity(2, 2); + + EXPECT_THROW(ts.AddMatrixToTransition("missing", m), + std::invalid_argument); +} + TEST_F(TimestepTest, RemoveTransition) { Timestep ts("test_log", test_log_file_); ts.CreateTransition("migration"); From f2f143ea91136fc3d6a166ebcada623fd116eb87 Mon Sep 17 00:00:00 2001 From: Matthew Carroll <28577806+MJC598@users.noreply.github.com> Date: Thu, 24 Sep 2026 12:27:12 -0400 Subject: [PATCH 09/11] matrix validation, missing loggers, and simulation durations --- CMakePresets.json | 3 +-- cmake/options.cmake | 3 +-- docs/src/api-guide.md | 4 +-- docs/src/limitations.md | 5 ++-- include/respond/logging.hpp | 12 +++++++-- include/respond/simulation.hpp | 30 ++++++++++++++++++++-- src/background.cpp | 5 ++-- src/internals/logging_internals.hpp | 23 +++++++---------- src/internals/transition_base.hpp | 24 +++++++++++++++-- src/migration.cpp | 5 ++-- src/overdose.cpp | 9 ++++--- tests/unit/background_test.cpp | 31 ++++++++++++++++++++++ tests/unit/logging_test.cpp | 7 +++++ tests/unit/simulation_test.cpp | 40 +++++++++++++++++++++++++++++ tests/unit/timestep_test.cpp | 8 +++--- 15 files changed, 169 insertions(+), 40 deletions(-) diff --git a/CMakePresets.json b/CMakePresets.json index f523ba62..bd806737 100644 --- a/CMakePresets.json +++ b/CMakePresets.json @@ -28,8 +28,7 @@ "RESPOND_CALCULATE_COVERAGE": "OFF", "RESPOND_BUILD_DOCS": "OFF", "RESPOND_INSTALL": "OFF", - "RESPOND_SYSTEM_INCLUDES": "OFF", - "RESPOND_NO_EXCEPTIONS": "OFF" + "RESPOND_SYSTEM_INCLUDES": "OFF" } }, { diff --git a/cmake/options.cmake b/cmake/options.cmake index 821b5ad7..8329a4c7 100644 --- a/cmake/options.cmake +++ b/cmake/options.cmake @@ -18,9 +18,8 @@ option(RESPOND_CALCULATE_COVERAGE "Calculate Code Coverage" OFF) # bench options option(RESPOND_BUILD_BENCH "Build benchmarks" OFF) -# compile level warning and exception options +# compile level warning options option(RESPOND_BUILD_WARNINGS "Enable compiler warnings" OFF) -option(RESPOND_NO_EXCEPTIONS "Compile with -fno-exceptions. Call abort() on any simdemics exceptions" OFF) # install options option(RESPOND_SYSTEM_INCLUDES "Include as system headers (skip for clang-tidy)." OFF) diff --git a/docs/src/api-guide.md b/docs/src/api-guide.md index 8f833593..5df2f8ba 100644 --- a/docs/src/api-guide.md +++ b/docs/src/api-guide.md @@ -174,8 +174,8 @@ auto history_names = sim.GetModelHistoryNames(0); ### Key Methods -- `Run(int duration = -1)`: Runs all models for the configured duration -- `SetDuration(int duration)`: Sets default duration used by `Run()` when no argument is provided +- `Run(int duration = -1)`: Runs all models; `-1` uses the configured duration and positive values override it +- `SetDuration(int duration)`: Sets a positive default duration used by `Run()` when no argument is provided - `CreateNewModel(const std::string &name)`: Creates and manages a model, then returns an editable deep copy; assign the edited copy through `sim[idx]` - `AddModel(const std::unique_ptr &model)`: Adds a model (cloned internally) - `operator[](size_t idx)`: Mutable index access to owned model slot (`sim[idx]->Method()`) diff --git a/docs/src/limitations.md b/docs/src/limitations.md index c40f3474..cac12d2c 100644 --- a/docs/src/limitations.md +++ b/docs/src/limitations.md @@ -16,8 +16,9 @@ The core API is stable enough for integration, but there are important constrain sharing a simulation across threads, and must not mutate or inspect models while `Simulation::Run()` is executing. - `Simulation::Run()` may execute independent models concurrently when enabled - through `ExecutionConfig`; the configured Eigen worker limit must still be - observed because Eigen's worker setting is process-global. + through `ExecutionConfig`. Separate `Simulation::Run()` calls are serialized + because Eigen's worker setting is process-global; the configured Eigen worker + limit must still be observed within each run. - Logging registries and shared sinks are internally synchronized for concurrent logger creation and logging. - GPU execution is not supported. diff --git a/include/respond/logging.hpp b/include/respond/logging.hpp index f8522895..95659aba 100644 --- a/include/respond/logging.hpp +++ b/include/respond/logging.hpp @@ -87,8 +87,8 @@ CreationStatus CreateSharedFileSink(const std::string &filepath); /// @return CreationStatus indicating the result of logger creation. /// @note Thread-safe: Can be called concurrently from multiple threads. /// @note Requires CreateSharedFileSink() to be called first with a file path. -/// @note If CreateSharedFileSink() wasn't called, creates a default sink to -/// "respond.log". +/// @note If CreateSharedFileSink() wasn't called, creates a shared sink for +/// the configured default sink path. /// @note Reusing a logger name with a different sink returns kError. CreationStatus CreateSharedLogger(const std::string &logger_name); @@ -123,24 +123,32 @@ void FlushAllLoggers(); /// @param logger_name Logger identifier (created via CreateFileLogger or /// CreateSharedLogger). /// @param message Message to log. +/// @note If logger_name is not configured, the message is written to stderr +/// and is not persisted by RESPOND. void LogInfo(const std::string &logger_name, const std::string &message); /// @brief Log a message as warning level. /// Thread-safe for concurrent calls from multiple threads. /// @param logger_name Logger identifier. /// @param message Message to log. +/// @note If logger_name is not configured, the message is written to stderr +/// and is not persisted by RESPOND. void LogWarning(const std::string &logger_name, const std::string &message); /// @brief Log a message as error level. /// Thread-safe for concurrent calls from multiple threads. /// @param logger_name Logger identifier. /// @param message Message to log. +/// @note If logger_name is not configured, the message is written to stderr +/// and is not persisted by RESPOND. void LogError(const std::string &logger_name, const std::string &message); /// @brief Log a message as debug level. /// Thread-safe for concurrent calls from multiple threads. /// @param logger_name Logger identifier. /// @param message Message to log. +/// @note If logger_name is not configured, the message is written to stderr +/// and is not persisted by RESPOND. void LogDebug(const std::string &logger_name, const std::string &message); // ============================================================================ diff --git a/include/respond/simulation.hpp b/include/respond/simulation.hpp index 9258ba9f..af424de3 100644 --- a/include/respond/simulation.hpp +++ b/include/respond/simulation.hpp @@ -244,14 +244,20 @@ class Simulation { /// @throws std::invalid_argument if multiple models would execute in /// parallel while more than one Eigen worker thread is configured. /// @throws std::invalid_argument if no models have been added. + /// @throws std::invalid_argument if duration is zero or less than -1. /// @throws Any exception raised by a model after all workers have joined. void Run(int duration = -1) { + if (duration == 0 || duration < -1) { + throw std::invalid_argument( + "Simulation duration must be positive or -1."); + } if (_models.empty()) { LogError(_runtime_config.logging.logger_name, "Cannot run a simulation with no models."); throw std::invalid_argument( "Error attempting to run simulation with no models."); } + std::lock_guard execution_lock(GetEigenExecutionMutex()); if (duration > 0) { _duration = duration; } @@ -447,7 +453,16 @@ class Simulation { return ret; } - void SetDuration(int duration) { _duration = duration; } + /// @brief Sets the default duration used by Run(). + /// @param duration A positive number of timesteps. + /// @throws std::invalid_argument if duration is not positive. + void SetDuration(int duration) { + if (duration <= 0) { + throw std::invalid_argument( + "Simulation duration must be positive."); + } + _duration = duration; + } /// @brief Retrieves the simulation execution settings. /// @return The current execution configuration. @@ -464,11 +479,22 @@ class Simulation { const RuntimeConfig &GetRuntimeConfig() const { return _runtime_config; } void SetRuntimeConfig(const RuntimeConfig &runtime_config) { + if (ConfigureLogger(runtime_config.logging) == CreationStatus::kError) { + LogError(_runtime_config.logging.logger_name, + "Unable to apply simulation runtime logging config."); + throw std::invalid_argument( + "Error attempting to apply simulation runtime logging " + "configuration."); + } _runtime_config = runtime_config; - ConfigureLogger(_runtime_config.logging); } private: + static std::mutex &GetEigenExecutionMutex() { + static std::mutex mutex; + return mutex; + } + Model &GetModelRefOrThrow(size_t idx) { if (idx >= _models.size()) { LogError(_runtime_config.logging.logger_name, diff --git a/src/background.cpp b/src/background.cpp index 46c49061..7f815f1b 100644 --- a/src/background.cpp +++ b/src/background.cpp @@ -24,8 +24,9 @@ Eigen::VectorXd BackgroundDeath::Execute(const Eigen::Ref &state, std::map &h) const { TestCorrectNumberMatrices(1); - TestMatrixSizes(state, GetMatrices()[0]); - Eigen::VectorXd deaths = state.cwiseProduct(GetMatrices()[0]); + auto matrix = GetMatrices()[0]; + TestMatrixSizes(state, matrix); + Eigen::VectorXd deaths = AsVector(state).cwiseProduct(AsVector(matrix)); TestLessThanState(state, deaths, "BackgroundDeath transition produced more deaths than " "available in state."); diff --git a/src/internals/logging_internals.hpp b/src/internals/logging_internals.hpp index f4164fb1..dab68ba3 100644 --- a/src/internals/logging_internals.hpp +++ b/src/internals/logging_internals.hpp @@ -129,17 +129,15 @@ CreationStatus CheckIfExists(const std::string &logger_name) { void log(const std::string &logger_name, const std::string &message, LogType type = LogType::kInfo) { - CreationStatus status = CheckIfExists(logger_name); - if ((status == CreationStatus::kNotCreated) && - (CreateFileLogger(logger_name, "respond.log") == - CreationStatus::kError)) { - std::cerr << "Failed to create logger: " << logger_name << std::endl; + auto logger = spdlog::get(logger_name); + if (!logger) { + std::cerr << "Logger '" << logger_name + << "' is not configured; message was not persisted: " + << message << std::endl; return; } - auto logger = spdlog::get(logger_name); - if (logger) { - switch (type) { + switch (type) { case LogType::kInfo: logger->info(message); break; @@ -155,12 +153,9 @@ void log(const std::string &logger_name, const std::string &message, default: logger->info(message); break; - } - if (LoggingRegistry::GetFlushInterval() == 0) { - logger->flush(); - } - } else { - spdlog::error("Logger {} not found", logger_name); + } + if (LoggingRegistry::GetFlushInterval() == 0) { + logger->flush(); } } diff --git a/src/internals/transition_base.hpp b/src/internals/transition_base.hpp index 600d874a..cfd94048 100644 --- a/src/internals/transition_base.hpp +++ b/src/internals/transition_base.hpp @@ -74,6 +74,23 @@ class TransitionBase : public virtual Transition { LogError(_log_name, error_msg); throw std::runtime_error(error_msg); } + if (m1.rows() != m2.rows() || m1.cols() != m2.cols()) { + LogWarning(_log_name, + "Transition warning - matrix shapes differ but " + "contain the same number of elements. Matrix 1 size " + "is (" + + std::to_string(m1.rows()) + ", " + + std::to_string(m1.cols()) + ") but Matrix 2 size " + "is (" + + std::to_string(m2.rows()) + ", " + + std::to_string(m2.cols()) + "). Comparing values " + "in column-major order."); + } + } + + Eigen::VectorXd AsVector( + const Eigen::Ref &matrix) const { + return Eigen::Map(matrix.data(), matrix.size()); } void TestSquareMatrix(const Eigen::Ref &m) const { @@ -131,10 +148,13 @@ class TransitionBase : public virtual Transition { void TestLessThanState(const Eigen::Ref &state, const Eigen::Ref &m1, std::string extra_msg = "") const { - if (!(state.array() >= m1.array()).all()) { + const auto state_vector = AsVector(state); + const auto value_vector = AsVector(m1); + if (!(state_vector.array() >= value_vector.array()).all()) { std::string error_msg = "Transition error - State contains values less than m1! " + - std::to_string((state.array() < m1.array()).count()) + + std::to_string( + (state_vector.array() < value_vector.array()).count()) + " elements affected. Verify that the transition matrix is " "correct and that the state vector is valid."; if (!extra_msg.empty()) { diff --git a/src/migration.cpp b/src/migration.cpp index d588c454..fcc966df 100644 --- a/src/migration.cpp +++ b/src/migration.cpp @@ -23,8 +23,9 @@ Eigen::VectorXd Migration::Execute(const Eigen::Ref &state, std::map &h) const { TestCorrectNumberMatrices(1); - TestMatrixSizes(state, GetMatrices()[0]); - Eigen::VectorXd subtracted = state + GetMatrices()[0]; + auto matrix = GetMatrices()[0]; + TestMatrixSizes(state, matrix); + Eigen::VectorXd subtracted = AsVector(state) + AsVector(matrix); Eigen::VectorXd zero_stop = subtracted.array().max( Eigen::VectorXd::Zero(subtracted.size()).array()); return zero_stop; diff --git a/src/overdose.cpp b/src/overdose.cpp index 2d59b8b4..78dee767 100644 --- a/src/overdose.cpp +++ b/src/overdose.cpp @@ -25,8 +25,9 @@ Overdose::Execute(const Eigen::Ref &state, TestCorrectNumberMatrices(2); auto matrices = GetMatrices(); - TestMatrixSizes(state, GetMatrices()[0]); - Eigen::VectorXd overdoses = state.cwiseProduct(GetMatrices()[0]); + TestMatrixSizes(state, matrices[0]); + Eigen::VectorXd overdoses = + AsVector(state).cwiseProduct(AsVector(matrices[0])); TestLessThanState(state, overdoses, "Overdose transition produced more total overdoses than " "available in state."); @@ -34,8 +35,8 @@ Overdose::Execute(const Eigen::Ref &state, h["total_overdose"].AccumulateState(overdoses); } - TestMatrixSizes(overdoses, GetMatrices()[1]); - Eigen::VectorXd fods = overdoses.cwiseProduct(GetMatrices()[1]); + TestMatrixSizes(overdoses, matrices[1]); + Eigen::VectorXd fods = overdoses.cwiseProduct(AsVector(matrices[1])); TestLessThanState(state, fods, "Overdose transition produced more fatal overdoses than " "available in state."); diff --git a/tests/unit/background_test.cpp b/tests/unit/background_test.cpp index 1077f787..ecf083c9 100644 --- a/tests/unit/background_test.cpp +++ b/tests/unit/background_test.cpp @@ -161,6 +161,37 @@ TEST_F(BackgroundDeathTest, ExecuteWithBackgroundDeathHistory) { EXPECT_TRUE(result.isApprox(expected_state)); } +TEST_F(BackgroundDeathTest, AcceptsVectorAndRowMatrixWithWarning) { + BackgroundDeath background_death; + Eigen::MatrixXd row_matrix(1, 3); + row_matrix << 0.5, 0.1, 0.8; + background_death.AddMatrix(row_matrix); + histories["state"] = History("state"); + + const Eigen::VectorXd result = background_death.Execute(state, histories); + + EXPECT_NEAR(result(0), 0.5, 1e-12); + EXPECT_NEAR(result(1), 1.8, 1e-12); + EXPECT_NEAR(result(2), 0.6, 1e-12); +} + +TEST_F(BackgroundDeathTest, AcceptsEqualElementTransposedMatricesWithWarning) { + Eigen::VectorXd six_state(6); + six_state << 1.0, 2.0, 3.0, 4.0, 5.0, 6.0; + Eigen::MatrixXd matrix(2, 3); + matrix << 0.1, 0.2, 0.3, 0.4, 0.5, 0.6; + BackgroundDeath background_death; + background_death.AddMatrix(matrix); + histories["state"] = History("state"); + + const Eigen::VectorXd expected_state = + six_state - six_state.cwiseProduct( + Eigen::Map(matrix.data(), 6)); + const Eigen::VectorXd result = background_death.Execute(six_state, histories); + + EXPECT_TRUE(result.isApprox(expected_state)); +} + TEST_F(BackgroundDeathTest, Clone) { BackgroundDeath background_death; background_death.AddMatrix(tran_matrix); diff --git a/tests/unit/logging_test.cpp b/tests/unit/logging_test.cpp index 195ecb89..b7b1bb57 100644 --- a/tests/unit/logging_test.cpp +++ b/tests/unit/logging_test.cpp @@ -405,6 +405,13 @@ TEST_F(LoggingTest, CheckLoggerExistsFalse) { EXPECT_EQ(status, CreationStatus::kNotCreated); } +TEST_F(LoggingTest, MissingLoggerDoesNotCreateFallbackLogger) { + LogInfo("missing_logger", "message without a configured logger"); + + EXPECT_EQ(CheckLoggerExists("missing_logger"), + CreationStatus::kNotCreated); +} + TEST_F(LoggingTest, GetLoggerInfo) { CreateFileLogger("test_logger", test_log_file_); diff --git a/tests/unit/simulation_test.cpp b/tests/unit/simulation_test.cpp index 2acb74f3..24d7d33b 100644 --- a/tests/unit/simulation_test.cpp +++ b/tests/unit/simulation_test.cpp @@ -156,6 +156,18 @@ TEST_F(SimulationTest, StoresRuntimeConfig) { EXPECT_EQ(s.GetRuntimeConfig().logging.file_path, test_log_file_); } +TEST_F(SimulationTest, RejectsRuntimeLoggingReconfigurationFailure) { + Simulation simulation; + const auto original_config = simulation.GetRuntimeConfig(); + RuntimeConfig conflicting_config = original_config; + conflicting_config.logging.file_path = test_log_file_; + + EXPECT_THROW(simulation.SetRuntimeConfig(conflicting_config), + std::invalid_argument); + EXPECT_EQ(simulation.GetRuntimeConfig().logging.file_path, + original_config.logging.file_path); +} + TEST_F(SimulationTest, CreateNewModel) { Simulation s; std::string model_name = "test_model"; @@ -253,6 +265,34 @@ TEST_F(SimulationTest, Run) { s.Run(); } +TEST_F(SimulationTest, RejectsInvalidRunDurations) { + Simulation simulation; + auto source = std::make_unique>(); + auto model = std::make_unique>(); + EXPECT_CALL(*model, RunTimesteps()).Times(0); + EXPECT_CALL(*source, clone()) + .WillOnce(Return(::testing::ByMove(std::move(model)))); + simulation.AddModel(std::move(source)); + + EXPECT_THROW(simulation.Run(0), std::invalid_argument); + EXPECT_THROW(simulation.Run(-2), std::invalid_argument); + EXPECT_THROW(simulation.SetDuration(0), std::invalid_argument); + EXPECT_THROW(simulation.SetDuration(-1), std::invalid_argument); +} + +TEST_F(SimulationTest, RunAppliesPositiveDurationOverride) { + Simulation simulation; + auto source = std::make_unique>(); + auto model = std::make_unique>(); + EXPECT_CALL(*model, SetFinalTimestep(4)).Times(1); + EXPECT_CALL(*model, RunTimesteps()).Times(1); + EXPECT_CALL(*source, clone()) + .WillOnce(Return(::testing::ByMove(std::move(model)))); + simulation.AddModel(std::move(source)); + + simulation.Run(4); +} + TEST_F(SimulationTest, RunMultipleModels) { Simulation s; auto mock_model = std::make_unique>(); diff --git a/tests/unit/timestep_test.cpp b/tests/unit/timestep_test.cpp index 0b51fb45..503ae67b 100644 --- a/tests/unit/timestep_test.cpp +++ b/tests/unit/timestep_test.cpp @@ -227,7 +227,7 @@ TEST_F(TimestepTest, CopyConstructor) { ASSERT_EQ(names[0], "migration"); } -TEST_F(TimestepTest, CopyPreservesLoggerName) { +TEST_F(TimestepTest, CopyPreservesLoggerNameWithoutRecreatingLogger) { Timestep original("test_log", test_log_file_); Timestep copy(original); spdlog::drop("test_log"); @@ -235,10 +235,10 @@ TEST_F(TimestepTest, CopyPreservesLoggerName) { EXPECT_THROW(copy.AddMatrixToTransition(0, Eigen::MatrixXd::Identity(1, 1)), std::out_of_range); - EXPECT_EQ(CheckLoggerExists("test_log"), CreationStatus::kExists); + EXPECT_EQ(CheckLoggerExists("test_log"), CreationStatus::kNotCreated); } -TEST_F(TimestepTest, MovePreservesLoggerName) { +TEST_F(TimestepTest, MovePreservesLoggerNameWithoutRecreatingLogger) { Timestep original("test_log", test_log_file_); Timestep moved(std::move(original)); spdlog::drop("test_log"); @@ -246,7 +246,7 @@ TEST_F(TimestepTest, MovePreservesLoggerName) { EXPECT_THROW(moved.AddMatrixToTransition(0, Eigen::MatrixXd::Identity(1, 1)), std::out_of_range); - EXPECT_EQ(CheckLoggerExists("test_log"), CreationStatus::kExists); + EXPECT_EQ(CheckLoggerExists("test_log"), CreationStatus::kNotCreated); } TEST_F(TimestepTest, CopyAssignment) { From ec38bcc9ca46aad5deab23957bf13fc257923600 Mon Sep 17 00:00:00 2001 From: Matthew Carroll <28577806+MJC598@users.noreply.github.com> Date: Thu, 24 Sep 2026 12:48:34 -0400 Subject: [PATCH 10/11] Fixing Eigen thread race conditions between simulations --- include/respond/eigen_config.hpp | 38 ++++++++++++++++++++++++ include/respond/history.hpp | 49 ++++++++++++++++++++++++------- include/respond/simulation.hpp | 14 ++++----- include/respond/timestep.hpp | 5 +++- src/internals/markov.hpp | 8 +++-- src/internals/transition_base.hpp | 6 +++- src/logging.cpp | 5 ++++ tests/unit/history_test.cpp | 9 ++++++ tests/unit/logging_test.cpp | 26 ++++++++++++++++ tests/unit/simulation_test.cpp | 11 +++++++ 10 files changed, 150 insertions(+), 21 deletions(-) create mode 100644 include/respond/eigen_config.hpp diff --git a/include/respond/eigen_config.hpp b/include/respond/eigen_config.hpp new file mode 100644 index 00000000..e8f2869c --- /dev/null +++ b/include/respond/eigen_config.hpp @@ -0,0 +1,38 @@ +//////////////////////////////////////////////////////////////////////////////// +// File: eigen_config.hpp // +// Project: respond // +// Created Date: 2026-09-24 // +// Author: Matthew Carroll // +// ----- // +// Last Modified: 2026-09-24 // +// Modified By: Matthew Carroll // +// ----- // +// Copyright (c) 2026 Syndemics Lab at Boston Medical Center // +//////////////////////////////////////////////////////////////////////////////// + +#ifndef RESPOND_EIGEN_CONFIG_HPP_ +#define RESPOND_EIGEN_CONFIG_HPP_ + +#include + +#include + +namespace respond { +namespace detail { + +/// @brief Returns the mutex coordinating Eigen configuration and execution. +inline std::mutex &GetEigenExecutionMutex() { + static std::mutex mutex; + return mutex; +} + +/// @brief Sets Eigen's process-global worker count under shared coordination. +inline void SetEigenThreads(unsigned int thread_count) { + std::lock_guard lock(GetEigenExecutionMutex()); + Eigen::setNbThreads(thread_count); +} + +} // namespace detail +} // namespace respond + +#endif // RESPOND_EIGEN_CONFIG_HPP_ \ No newline at end of file diff --git a/include/respond/history.hpp b/include/respond/history.hpp index b585b0c4..165db958 100644 --- a/include/respond/history.hpp +++ b/include/respond/history.hpp @@ -14,9 +14,11 @@ #include #include +#include #include #include +#include #include #include #include @@ -59,43 +61,48 @@ class History { /// @brief Default constructor initializing a history with the default name /// "state" and default mode based on that name. - History() : History("state") {} + History() : History("state", GetDefaultHistoryMode("state"), LoggingConfig{}) {} /// @brief Constructs a history with a specified name, using the default /// mode based on that name. /// @param name The identifier name for this history instance. History(const std::string &name) - : History(name, GetDefaultHistoryMode(name)) {} + : History(name, GetDefaultHistoryMode(name), LoggingConfig{}) {} /// @brief Constructs a history with a specified name and mode. /// @param name The identifier name for this history instance. /// @param mode The history recording mode (snapshot or accumulated). History(const std::string &name, const HistoryMode &mode) - : History(name, mode, RESPOND_DEFAULT_LOG, RESPOND_DEFAULT_LOG_FILE) {} + : History(name, mode, LoggingConfig{}) {} /// @brief Constructs a history with a specified name, mode, and logger. /// @param name The identifier name for this history instance. /// @param mode The history recording mode (snapshot or accumulated). /// @param log_name The name of the logger to use for history output. + [[deprecated("Use History(name, mode, LoggingConfig) instead")]] History(const std::string &name, const HistoryMode &mode, const std::string &log_name) - : History(name, mode, log_name, RESPOND_DEFAULT_LOG_FILE) {} + : History(name, mode, + LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, false}) {} /// @brief Constructs a history with a specified name and logger. /// @param name The identifier name for this history instance. /// @param log_name The name of the logger to use for history output. + [[deprecated("Use History(name, LoggingConfig) instead")]] History(const std::string &name, const std::string &log_name) - : History(name, GetDefaultHistoryMode(name), log_name, - RESPOND_DEFAULT_LOG_FILE) {} + : History(name, GetDefaultHistoryMode(name), + LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, false}) {} /// @brief Constructs a history with a specified name, logger, and log /// file path. /// @param name The identifier name for this history instance. /// @param log_name The name of the logger to use for history output. /// @param log_filepath The file path for the logger output. + [[deprecated("Use History(name, LoggingConfig) instead")]] History(const std::string &name, const std::string &log_name, const std::string &log_filepath) - : History(name, GetDefaultHistoryMode(name), log_name, log_filepath) {} + : History(name, GetDefaultHistoryMode(name), + LoggingConfig{log_name, log_filepath, false}) {} /// @brief Constructs a history with a specified name, mode, logger, and /// log file path. @@ -103,10 +110,22 @@ class History { /// @param mode The history recording mode (snapshot or accumulated). /// @param log_name The name of the logger to use for history output. /// @param log_filepath The file path for the logger output. + [[deprecated("Use History(name, mode, LoggingConfig) instead")]] History(const std::string &name, const HistoryMode &mode, const std::string &log_name, const std::string &log_filepath) - : _name(name), _mode(mode), _log_name(log_name) { - CreateFileLogger(log_name, log_filepath); + : History(name, mode, LoggingConfig{log_name, log_filepath, false}) {} + + History(const std::string &name, const LoggingConfig &logging_config) + : History(name, GetDefaultHistoryMode(name), logging_config) {} + + History(const std::string &name, const HistoryMode &mode, + const LoggingConfig &logging_config) + : _name(name), _mode(mode), _log_name(logging_config.logger_name), + _logging_config(logging_config) { + if (ConfigureLogger(_logging_config) == CreationStatus::kError) { + throw std::runtime_error( + "Error attempting to initialize history logger."); + } } /// @brief Destructor (default). @@ -119,6 +138,7 @@ class History { _states = other.GetRecordedStates(); _name = other._name; _log_name = other._log_name; + _logging_config = other._logging_config; _mode = other._mode; _pending_state = other.GetPendingState(); } @@ -132,6 +152,7 @@ class History { _states = other.GetRecordedStates(); _name = other._name; _log_name = other._log_name; + _logging_config = other._logging_config; _mode = other._mode; _pending_state = other.GetPendingState(); } @@ -144,7 +165,8 @@ class History { History(History &&other) noexcept : _timesteps(std::move(other._timesteps)), _states(std::move(other._states)), _name(std::move(other._name)), - _log_name(std::move(other._log_name)), _mode(other._mode), + _log_name(std::move(other._log_name)), + _logging_config(std::move(other._logging_config)), _mode(other._mode), _pending_state(std::move(other._pending_state)) {} /// @brief Move assignment operator implementing the Rule of Five. @@ -156,6 +178,7 @@ class History { _states = std::move(other._states); _name = std::move(other._name); _log_name = std::move(other._log_name); + _logging_config = std::move(other._logging_config); _mode = other._mode; _pending_state = std::move(other._pending_state); } @@ -339,6 +362,10 @@ class History { /// @return True if all history properties and state are identical. bool operator==(const History &other) const { return _name == other._name && _log_name == other._log_name && + _logging_config.logger_name == other._logging_config.logger_name && + _logging_config.file_path == other._logging_config.file_path && + _logging_config.use_shared_sink == + other._logging_config.use_shared_sink && _mode == other._mode && GetStateMap() == other.GetStateMap() && GetPendingState().isApprox(other.GetPendingState()); } @@ -360,6 +387,8 @@ class History { private: /// @brief The logger name for this history. std::string _log_name; + /// @brief The logging configuration for this history. + LoggingConfig _logging_config; /// @brief The identifier name for this history. std::string _name; /// @brief Controls whether this history stores snapshots or aggregates. diff --git a/include/respond/simulation.hpp b/include/respond/simulation.hpp index af424de3..cd0b9f7f 100644 --- a/include/respond/simulation.hpp +++ b/include/respond/simulation.hpp @@ -13,6 +13,7 @@ #define RESPOND_SIMULATION_HPP_ #include +#include #include #include #include @@ -126,7 +127,10 @@ class Simulation { /// @brief Constructs a Simulation with shared runtime settings. explicit Simulation(const RuntimeConfig &runtime_config) : _runtime_config(runtime_config) { - ConfigureLogger(_runtime_config.logging); + if (ConfigureLogger(_runtime_config.logging) == CreationStatus::kError) { + throw std::runtime_error( + "Error attempting to initialize simulation logger."); + } } /// @brief Virtual destructor for polymorphic cleanup. @@ -257,7 +261,8 @@ class Simulation { throw std::invalid_argument( "Error attempting to run simulation with no models."); } - std::lock_guard execution_lock(GetEigenExecutionMutex()); + std::lock_guard execution_lock( + detail::GetEigenExecutionMutex()); if (duration > 0) { _duration = duration; } @@ -490,11 +495,6 @@ class Simulation { } private: - static std::mutex &GetEigenExecutionMutex() { - static std::mutex mutex; - return mutex; - } - Model &GetModelRefOrThrow(size_t idx) { if (idx >= _models.size()) { LogError(_runtime_config.logging.logger_name, diff --git a/include/respond/timestep.hpp b/include/respond/timestep.hpp index f61d2391..452cf2ab 100644 --- a/include/respond/timestep.hpp +++ b/include/respond/timestep.hpp @@ -114,7 +114,10 @@ class Timestep { explicit Timestep(const LoggingConfig &logging_config) : _log_name(logging_config.logger_name), _logging_config(logging_config) { - ConfigureLogger(_logging_config); + if (ConfigureLogger(_logging_config) == CreationStatus::kError) { + throw std::runtime_error( + "Error attempting to initialize timestep logger."); + } } /// @brief Destructor for Timestep. Default implementation. diff --git a/src/internals/markov.hpp b/src/internals/markov.hpp index fce22569..89425436 100644 --- a/src/internals/markov.hpp +++ b/src/internals/markov.hpp @@ -23,6 +23,7 @@ #include #include +#include #include #include #include @@ -67,14 +68,17 @@ class Markov : public virtual Model { _runtime_config(runtime_config), _current_timestep(0), _history_capture_interval(1), _final_timestep(-1), _initial_history_recorded(false) { - ConfigureLogger(_runtime_config.logging); + if (ConfigureLogger(_runtime_config.logging) == CreationStatus::kError) { + throw std::runtime_error( + "Error attempting to initialize model logger."); + } const unsigned int thread_limit = std::thread::hardware_concurrency(); const unsigned int threads = thread_limit == 0 ? _runtime_config.execution.eigen_threads : std::min(_runtime_config.execution.eigen_threads, thread_limit); - Eigen::setNbThreads(threads); + detail::SetEigenThreads(threads); } /// @brief Constructs a Markov model with explicit execution settings. diff --git a/src/internals/transition_base.hpp b/src/internals/transition_base.hpp index cfd94048..f291809f 100644 --- a/src/internals/transition_base.hpp +++ b/src/internals/transition_base.hpp @@ -16,6 +16,7 @@ #include #include +#include #include #include @@ -33,7 +34,10 @@ class TransitionBase : public virtual Transition { const LoggingConfig &logging_config) : _name(name), _log_name(logging_config.logger_name), _logging_config(logging_config) { - ConfigureLogger(_logging_config); + if (ConfigureLogger(_logging_config) == CreationStatus::kError) { + throw std::runtime_error( + "Error attempting to initialize transition logger."); + } } virtual ~TransitionBase() = default; // Add a Transition Matrix to the set. We have no need to edit it once it's diff --git a/src/logging.cpp b/src/logging.cpp index c136903e..41ea0824 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -16,11 +16,14 @@ #include #include +#include namespace respond { namespace { +std::mutex logger_creation_mutex; + bool LoggerUsesFile(const std::shared_ptr &logger, const std::string &filepath) { if (!logger) { @@ -71,6 +74,7 @@ CreationStatus ExistingLoggerStatus(const std::string &logger_name, CreationStatus CreateSharedLogger( const std::string &logger_name, const std::shared_ptr &sink) { + std::lock_guard lock(logger_creation_mutex); if (auto existing_logger = spdlog::get(logger_name)) { return ExistingLoggerStatus(logger_name, LoggerUsesSink(existing_logger, sink)); @@ -108,6 +112,7 @@ CreationStatus CreateSharedLogger( CreationStatus CreateFileLogger(const std::string &logger_name, const std::string &filepath) { + std::lock_guard lock(logger_creation_mutex); if (auto existing_logger = spdlog::get(logger_name)) { return ExistingLoggerStatus(logger_name, LoggerUsesFile(existing_logger, filepath)); diff --git a/tests/unit/history_test.cpp b/tests/unit/history_test.cpp index bfefce2d..05e9fa71 100644 --- a/tests/unit/history_test.cpp +++ b/tests/unit/history_test.cpp @@ -109,6 +109,15 @@ TEST_F(HistoryTest, ConstructorWithNameModeAndLogger) { EXPECT_TRUE(history.GetRecordedStates().empty()); } +TEST_F(HistoryTest, ConstructorWithLoggingConfigSupportsSharedSink) { + LoggingConfig config{"shared_history_logger", default_log_file_, true}; + History history("shared_history", HistoryMode::kSnapshot, config); + + EXPECT_EQ(history.GetName(), "shared_history"); + EXPECT_EQ(CheckLoggerExists("shared_history_logger"), + CreationStatus::kExists); +} + TEST_F(HistoryTest, ConstructorWithNameAndLogger) { History history("background_death", "test_logger"); EXPECT_EQ(history.GetName(), "background_death"); diff --git a/tests/unit/logging_test.cpp b/tests/unit/logging_test.cpp index b7b1bb57..df90d5df 100644 --- a/tests/unit/logging_test.cpp +++ b/tests/unit/logging_test.cpp @@ -519,6 +519,32 @@ TEST_F(LoggingTest, ConcurrentCreateSharedLogger) { } } +TEST_F(LoggingTest, ConcurrentSameNameLoggerCreationIsConsistent) { + constexpr int thread_count = 8; + std::vector statuses(thread_count); + std::vector threads; + + for (int index = 0; index < thread_count; ++index) { + threads.emplace_back([&, index]() { + statuses[index] = + CreateFileLogger("same_name_logger", test_log_file_); + }); + } + for (auto &thread : threads) { + thread.join(); + } + + int successful_creations = 0; + for (const auto status : statuses) { + if (status == CreationStatus::kSuccess) { + ++successful_creations; + } else { + EXPECT_EQ(status, CreationStatus::kExists); + } + } + EXPECT_EQ(successful_creations, 1); +} + // ============================================================================ // Test: Integration - Full Workflow // ============================================================================ diff --git a/tests/unit/simulation_test.cpp b/tests/unit/simulation_test.cpp index 24d7d33b..1a63a0a9 100644 --- a/tests/unit/simulation_test.cpp +++ b/tests/unit/simulation_test.cpp @@ -107,6 +107,17 @@ TEST_F(SimulationTest, ConstructorWithLogNameAndLogFile) { CreationStatus::kExists); } +TEST_F(SimulationTest, ConstructorRejectsLoggerInitializationFailure) { + ASSERT_EQ(CreateFileLogger("conflicting_simulation_logger", + default_log_file_), + CreationStatus::kSuccess); + RuntimeConfig config; + config.logging.logger_name = "conflicting_simulation_logger"; + config.logging.file_path = test_log_file_; + + EXPECT_THROW(Simulation simulation(config), std::runtime_error); +} + TEST_F(SimulationTest, StoresExecutionConfig) { ExecutionConfig config; config.total_threads = 4; From d5bb07a1d9bfecfc192db8a5610f1ed1672bcb8a Mon Sep 17 00:00:00 2001 From: Matthew Carroll <28577806+MJC598@users.noreply.github.com> Date: Thu, 24 Sep 2026 14:14:17 -0400 Subject: [PATCH 11/11] linting --- include/respond/history.hpp | 14 +++++----- include/respond/model.hpp | 36 +++++++++++++------------- include/respond/runtime_config.hpp | 2 +- include/respond/simulation.hpp | 20 +++++++------- include/respond/timestep.hpp | 15 +++++------ include/respond/transition.hpp | 11 ++++---- src/internals/background.hpp | 9 ++++--- src/internals/behavior.hpp | 6 ++--- src/internals/intervention.hpp | 9 +++---- src/internals/logging_internals.hpp | 34 ++++++++++++------------ src/internals/markov.hpp | 9 ++++--- src/internals/migration.hpp | 6 ++--- src/internals/overdose.hpp | 6 ++--- src/internals/transition_base.hpp | 29 +++++++++++---------- src/logging.cpp | 22 +++++++--------- src/model_factory.cpp | 11 ++++---- src/transition_factory.cpp | 4 +-- tests/unit/background_test.cpp | 5 ++-- tests/unit/behavior_test.cpp | 2 +- tests/unit/cost_effectiveness_test.cpp | 2 +- tests/unit/history_test.cpp | 5 ++-- tests/unit/intervention_test.cpp | 2 +- tests/unit/logging_test.cpp | 21 ++++++--------- tests/unit/markov_test.cpp | 4 +-- tests/unit/migration_test.cpp | 2 +- tests/unit/simulation_test.cpp | 22 ++++++++-------- tests/unit/timestep_test.cpp | 22 +++++++--------- 27 files changed, 161 insertions(+), 169 deletions(-) diff --git a/include/respond/history.hpp b/include/respond/history.hpp index 165db958..750f36e0 100644 --- a/include/respond/history.hpp +++ b/include/respond/history.hpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-05 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-13 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // @@ -18,8 +18,8 @@ #include #include -#include #include +#include #include #include @@ -61,7 +61,8 @@ class History { /// @brief Default constructor initializing a history with the default name /// "state" and default mode based on that name. - History() : History("state", GetDefaultHistoryMode("state"), LoggingConfig{}) {} + History() + : History("state", GetDefaultHistoryMode("state"), LoggingConfig{}) {} /// @brief Constructs a history with a specified name, using the default /// mode based on that name. @@ -206,8 +207,8 @@ class History { const auto insertion_point = std::lower_bound(_timesteps.begin(), _timesteps.end(), timestep); - const auto index = static_cast( - insertion_point - _timesteps.begin()); + const auto index = + static_cast(insertion_point - _timesteps.begin()); if (insertion_point != _timesteps.end() && *insertion_point == timestep) { _states[index] = state; @@ -362,7 +363,8 @@ class History { /// @return True if all history properties and state are identical. bool operator==(const History &other) const { return _name == other._name && _log_name == other._log_name && - _logging_config.logger_name == other._logging_config.logger_name && + _logging_config.logger_name == + other._logging_config.logger_name && _logging_config.file_path == other._logging_config.file_path && _logging_config.use_shared_sink == other._logging_config.use_shared_sink && diff --git a/include/respond/model.hpp b/include/respond/model.hpp index aebcf94a..57420294 100644 --- a/include/respond/model.hpp +++ b/include/respond/model.hpp @@ -4,8 +4,8 @@ // Created Date: 2026-02-05 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-09-14 // -// Modified By: Dimitri Baptiste // +// Last Modified: 2026-09-24 // +// Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // //////////////////////////////////////////////////////////////////////////////// @@ -66,22 +66,22 @@ class Model { const std::string &log_name = RESPOND_DEFAULT_LOG, const std::string &log_filepath = RESPOND_DEFAULT_LOG_FILE); - /// @brief Creates a Model with explicit execution settings. - /// @param name The instance name for the model. - /// @param execution_config Resource settings for model execution. - /// @param log_name Name of the logger for this model (default: "console"). - /// @param log_filepath File path for the log file (default: "respond.log"). - /// @return A unique_ptr to the newly created Model instance. - [[deprecated("Use Model::Create(name, RuntimeConfig) instead")]] - static std::unique_ptr - Create(const std::string &name, const ExecutionConfig &execution_config, - const std::string &log_name = RESPOND_DEFAULT_LOG, - const std::string &log_filepath = RESPOND_DEFAULT_LOG_FILE); - - /// @brief Creates a Markov model with shared runtime settings. - /// @param name The instance name for the model. - static std::unique_ptr - Create(const std::string &name, const RuntimeConfig &runtime_config); + /// @brief Creates a Model with explicit execution settings. + /// @param name The instance name for the model. + /// @param execution_config Resource settings for model execution. + /// @param log_name Name of the logger for this model (default: "console"). + /// @param log_filepath File path for the log file (default: "respond.log"). + /// @return A unique_ptr to the newly created Model instance. + [[deprecated("Use Model::Create(name, RuntimeConfig) instead")]] + static std::unique_ptr + Create(const std::string &name, const ExecutionConfig &execution_config, + const std::string &log_name = RESPOND_DEFAULT_LOG, + const std::string &log_filepath = RESPOND_DEFAULT_LOG_FILE); + + /// @brief Creates a Markov model with shared runtime settings. + /// @param name The instance name for the model. + static std::unique_ptr Create(const std::string &name, + const RuntimeConfig &runtime_config); /// @brief Virtual destructor for proper polymorphic cleanup. virtual ~Model() = default; diff --git a/include/respond/runtime_config.hpp b/include/respond/runtime_config.hpp index 26bafc4f..85ea6b97 100644 --- a/include/respond/runtime_config.hpp +++ b/include/respond/runtime_config.hpp @@ -1,5 +1,5 @@ //////////////////////////////////////////////////////////////////////////////// -// File: runtime_config.hpp // +// File: runtime_config.hpp // // Project: respond // // Created Date: 2026-09-22 // // Author: Matthew Carroll // diff --git a/include/respond/simulation.hpp b/include/respond/simulation.hpp index cd0b9f7f..8d66299b 100644 --- a/include/respond/simulation.hpp +++ b/include/respond/simulation.hpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-05 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-09-22 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // @@ -127,7 +127,8 @@ class Simulation { /// @brief Constructs a Simulation with shared runtime settings. explicit Simulation(const RuntimeConfig &runtime_config) : _runtime_config(runtime_config) { - if (ConfigureLogger(_runtime_config.logging) == CreationStatus::kError) { + if (ConfigureLogger(_runtime_config.logging) == + CreationStatus::kError) { throw std::runtime_error( "Error attempting to initialize simulation logger."); } @@ -180,14 +181,13 @@ class Simulation { /// @param other The simulation to move from. Simulation(Simulation &&other) noexcept : _runtime_config(std::move(other._runtime_config)), - _models(std::move(other._models)), - _duration(other._duration), + _models(std::move(other._models)), _duration(other._duration), _parameter_change_times(std::move(other._parameter_change_times)), _stratify_entering_cohort(other._stratify_entering_cohort), _build_summary_stats(other._build_summary_stats), _save_state_history(other._save_state_history), - _timesteps_to_report(std::move(other._timesteps_to_report)), - _pivot_long(other._pivot_long) {} + _timesteps_to_report(std::move(other._timesteps_to_report)), + _pivot_long(other._pivot_long) {} /// @brief Move assignment operator for transferring simulation ownership. /// @param other The simulation to move from. @@ -269,9 +269,8 @@ class Simulation { const auto &execution = _runtime_config.execution; const unsigned int thread_limit = std::thread::hardware_concurrency(); const unsigned int eigen_threads = - thread_limit == 0 - ? execution.eigen_threads - : std::min(execution.eigen_threads, thread_limit); + thread_limit == 0 ? execution.eigen_threads + : std::min(execution.eigen_threads, thread_limit); Eigen::setNbThreads(eigen_threads); LogInfo(_runtime_config.logging.logger_name, @@ -300,7 +299,8 @@ class Simulation { worker_limit = 1; } } - const auto worker_count = std::min(worker_limit, _models.size()); + const auto worker_count = + std::min(worker_limit, _models.size()); if (worker_count <= 1) { for (const auto &model : _models) { run_model(model); diff --git a/include/respond/timestep.hpp b/include/respond/timestep.hpp index 452cf2ab..9543b9a7 100644 --- a/include/respond/timestep.hpp +++ b/include/respond/timestep.hpp @@ -4,7 +4,7 @@ // Created Date: 2026-06-30 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-23 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // @@ -154,9 +154,9 @@ class Timestep { /// transitions. /// @param other The Timestep instance to move from. Timestep(Timestep &&other) noexcept - : _log_name(std::move(other._log_name)), - _logging_config(std::move(other._logging_config)), - _transitions(std::move(other._transitions)) { + : _log_name(std::move(other._log_name)), + _logging_config(std::move(other._logging_config)), + _transitions(std::move(other._transitions)) { other._transitions.clear(); } @@ -193,9 +193,8 @@ class Timestep { /// exception if the transition type is unsupported. const std::unique_ptr & CreateTransition(const std::string &transition_name) { - _transitions.push_back( - Transition::Create(transition_name, transition_name, - _logging_config)); + _transitions.push_back(Transition::Create( + transition_name, transition_name, _logging_config)); return _transitions.back(); } @@ -261,7 +260,7 @@ class Timestep { } } LogError(_log_name, "Transition not found in AddMatrixToTransition: " + - transition_name); + transition_name); throw std::invalid_argument( "Error attempting to AddMatrixToTransition by name: " + transition_name); diff --git a/include/respond/transition.hpp b/include/respond/transition.hpp index 1bb3aa9d..37b164e0 100644 --- a/include/respond/transition.hpp +++ b/include/respond/transition.hpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-02 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-14 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // @@ -85,17 +85,16 @@ class Transition { /// @return A unique_ptr to the created Transition. /// @throws std::invalid_argument if type is unsupported. The error is also /// written through the logger identified by log_name. - [[deprecated( - "Use Transition::Create(type, name, LoggingConfig) instead")]] + [[deprecated("Use Transition::Create(type, name, LoggingConfig) instead")]] static std::unique_ptr Create(const std::string &type, const std::string &name = RESPOND_DEFAULT_TRANSITION_NAME, const std::string &log_name = RESPOND_DEFAULT_LOG, const std::string &log_file = RESPOND_DEFAULT_LOG_FILE); - static std::unique_ptr - Create(const std::string &type, const std::string &name, - const LoggingConfig &logging_config); + static std::unique_ptr + Create(const std::string &type, const std::string &name, + const LoggingConfig &logging_config); /// @brief Helper function to overload to the stream insertion operator for /// Transition serialization. diff --git a/src/internals/background.hpp b/src/internals/background.hpp index 6db492df..487ed380 100644 --- a/src/internals/background.hpp +++ b/src/internals/background.hpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-05 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-13 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // @@ -30,8 +30,8 @@ class BackgroundDeath : public virtual TransitionBase { : BackgroundDeath(name, LoggingConfig{}) {} [[deprecated("Use BackgroundDeath(name, LoggingConfig) instead")]] BackgroundDeath(const std::string &name, const std::string &log_name) - : BackgroundDeath(name, LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, - false}) {} + : BackgroundDeath( + name, LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, false}) {} [[deprecated("Use BackgroundDeath(name, LoggingConfig) instead")]] BackgroundDeath(const std::string &name, const std::string &log_name, const std::string &log_file) @@ -48,7 +48,8 @@ class BackgroundDeath : public virtual TransitionBase { // Clone std::unique_ptr clone() const override { - auto ret = std::make_unique(GetName(), _logging_config); + auto ret = + std::make_unique(GetName(), _logging_config); for (const auto &t : GetMatrices()) { ret->AddMatrix(t); } diff --git a/src/internals/behavior.hpp b/src/internals/behavior.hpp index 6aea2b25..ae94cdbb 100644 --- a/src/internals/behavior.hpp +++ b/src/internals/behavior.hpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-05 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-13 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // @@ -29,8 +29,8 @@ class Behavior : public virtual TransitionBase { Behavior(const std::string &name) : Behavior(name, LoggingConfig{}) {} [[deprecated("Use Behavior(name, LoggingConfig) instead")]] Behavior(const std::string &name, const std::string &log_name) - : Behavior(name, LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, - false}) {} + : Behavior(name, + LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, false}) {} [[deprecated("Use Behavior(name, LoggingConfig) instead")]] Behavior(const std::string &name, const std::string &log_name, const std::string &log_file) diff --git a/src/internals/intervention.hpp b/src/internals/intervention.hpp index a5500c77..49d97f16 100644 --- a/src/internals/intervention.hpp +++ b/src/internals/intervention.hpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-05 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-13 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // @@ -30,14 +30,13 @@ class Intervention : public virtual TransitionBase { : Intervention(name, LoggingConfig{}) {} [[deprecated("Use Intervention(name, LoggingConfig) instead")]] Intervention(const std::string &name, const std::string &log_name) - : Intervention(name, LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, - false}) {} + : Intervention( + name, LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, false}) {} [[deprecated("Use Intervention(name, LoggingConfig) instead")]] Intervention(const std::string &name, const std::string &log_name, const std::string &log_file) : TransitionBase(name, LoggingConfig{log_name, log_file, false}) {} - Intervention(const std::string &name, - const LoggingConfig &logging_config) + Intervention(const std::string &name, const LoggingConfig &logging_config) : TransitionBase(name, logging_config) {} // Run the execute function and return the final state. Do not edit the diff --git a/src/internals/logging_internals.hpp b/src/internals/logging_internals.hpp index dab68ba3..ec690c7e 100644 --- a/src/internals/logging_internals.hpp +++ b/src/internals/logging_internals.hpp @@ -4,10 +4,10 @@ // Created Date: 2025-06-06 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2025-07-30 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // -// Copyright (c) 2025 Syndemics Lab at Boston Medical Center // +// Copyright (c) 2025-2026 Syndemics Lab at Boston Medical Center // //////////////////////////////////////////////////////////////////////////////// #ifndef RESPOND_LOGGINGINTERNALS_HPP_ @@ -138,21 +138,21 @@ void log(const std::string &logger_name, const std::string &message, } switch (type) { - case LogType::kInfo: - logger->info(message); - break; - case LogType::kWarn: - logger->warn(message); - break; - case LogType::kError: - logger->error(message); - break; - case LogType::kDebug: - logger->debug(message); - break; - default: - logger->info(message); - break; + case LogType::kInfo: + logger->info(message); + break; + case LogType::kWarn: + logger->warn(message); + break; + case LogType::kError: + logger->error(message); + break; + case LogType::kDebug: + logger->debug(message); + break; + default: + logger->info(message); + break; } if (LoggingRegistry::GetFlushInterval() == 0) { logger->flush(); diff --git a/src/internals/markov.hpp b/src/internals/markov.hpp index 89425436..2f92d639 100644 --- a/src/internals/markov.hpp +++ b/src/internals/markov.hpp @@ -68,10 +68,11 @@ class Markov : public virtual Model { _runtime_config(runtime_config), _current_timestep(0), _history_capture_interval(1), _final_timestep(-1), _initial_history_recorded(false) { - if (ConfigureLogger(_runtime_config.logging) == CreationStatus::kError) { - throw std::runtime_error( - "Error attempting to initialize model logger."); - } + if (ConfigureLogger(_runtime_config.logging) == + CreationStatus::kError) { + throw std::runtime_error( + "Error attempting to initialize model logger."); + } const unsigned int thread_limit = std::thread::hardware_concurrency(); const unsigned int threads = thread_limit == 0 diff --git a/src/internals/migration.hpp b/src/internals/migration.hpp index 8c9c2425..91a9fe9b 100644 --- a/src/internals/migration.hpp +++ b/src/internals/migration.hpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-05 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-13 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // @@ -29,8 +29,8 @@ class Migration : public virtual TransitionBase { Migration(const std::string &name) : Migration(name, LoggingConfig{}) {} [[deprecated("Use Migration(name, LoggingConfig) instead")]] Migration(const std::string &name, const std::string &log_name) - : Migration(name, LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, - false}) {} + : Migration(name, + LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, false}) {} [[deprecated("Use Migration(name, LoggingConfig) instead")]] Migration(const std::string &name, const std::string &log_name, const std::string &log_file) diff --git a/src/internals/overdose.hpp b/src/internals/overdose.hpp index cfdfadff..08f66ee5 100644 --- a/src/internals/overdose.hpp +++ b/src/internals/overdose.hpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-05 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-13 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // @@ -29,8 +29,8 @@ class Overdose : public virtual TransitionBase { Overdose(const std::string &name) : Overdose(name, LoggingConfig{}) {} [[deprecated("Use Overdose(name, LoggingConfig) instead")]] Overdose(const std::string &name, const std::string &log_name) - : Overdose(name, LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, - false}) {} + : Overdose(name, + LoggingConfig{log_name, RESPOND_DEFAULT_LOG_FILE, false}) {} [[deprecated("Use Overdose(name, LoggingConfig) instead")]] Overdose(const std::string &name, const std::string &log_name, const std::string &log_file) diff --git a/src/internals/transition_base.hpp b/src/internals/transition_base.hpp index f291809f..fbcbe58a 100644 --- a/src/internals/transition_base.hpp +++ b/src/internals/transition_base.hpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-05 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-14 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // @@ -29,16 +29,15 @@ class TransitionBase : public virtual Transition { [[deprecated("Use TransitionBase(name, LoggingConfig) instead")]] TransitionBase(const std::string &name, const std::string &log_name, const std::string &log_file) - : TransitionBase(name, LoggingConfig{log_name, log_file, false}) {} - TransitionBase(const std::string &name, - const LoggingConfig &logging_config) - : _name(name), _log_name(logging_config.logger_name), - _logging_config(logging_config) { - if (ConfigureLogger(_logging_config) == CreationStatus::kError) { - throw std::runtime_error( - "Error attempting to initialize transition logger."); - } + : TransitionBase(name, LoggingConfig{log_name, log_file, false}) {} + TransitionBase(const std::string &name, const LoggingConfig &logging_config) + : _name(name), _log_name(logging_config.logger_name), + _logging_config(logging_config) { + if (ConfigureLogger(_logging_config) == CreationStatus::kError) { + throw std::runtime_error( + "Error attempting to initialize transition logger."); } + } virtual ~TransitionBase() = default; // Add a Transition Matrix to the set. We have no need to edit it once it's // been added, just use it. Thus, we don't need full ownership (reference) @@ -84,16 +83,18 @@ class TransitionBase : public virtual Transition { "contain the same number of elements. Matrix 1 size " "is (" + std::to_string(m1.rows()) + ", " + - std::to_string(m1.cols()) + ") but Matrix 2 size " + std::to_string(m1.cols()) + + ") but Matrix 2 size " "is (" + std::to_string(m2.rows()) + ", " + - std::to_string(m2.cols()) + "). Comparing values " + std::to_string(m2.cols()) + + "). Comparing values " "in column-major order."); } } - Eigen::VectorXd AsVector( - const Eigen::Ref &matrix) const { + Eigen::VectorXd + AsVector(const Eigen::Ref &matrix) const { return Eigen::Map(matrix.data(), matrix.size()); } diff --git a/src/logging.cpp b/src/logging.cpp index 41ea0824..9692ff5a 100644 --- a/src/logging.cpp +++ b/src/logging.cpp @@ -4,7 +4,7 @@ // Created Date: 2025-06-06 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-09 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2025-2026 Syndemics Lab at Boston Medical Center // @@ -33,9 +33,8 @@ bool LoggerUsesFile(const std::shared_ptr &logger, for (const auto &sink : logger->sinks()) { auto file_sink = std::dynamic_pointer_cast(sink); - if (file_sink && - std::filesystem::path(file_sink->filename()) == - std::filesystem::path(filepath)) { + if (file_sink && std::filesystem::path(file_sink->filename()) == + std::filesystem::path(filepath)) { return true; } } @@ -60,8 +59,7 @@ bool LoggerUsesSink( CreationStatus ExistingLoggerStatus(const std::string &logger_name, bool same_configuration) { if (same_configuration) { - std::cout << "Logger " << logger_name << " already exists" - << std::endl; + std::cout << "Logger " << logger_name << " already exists" << std::endl; return CreationStatus::kExists; } @@ -77,13 +75,12 @@ CreationStatus CreateSharedLogger( std::lock_guard lock(logger_creation_mutex); if (auto existing_logger = spdlog::get(logger_name)) { return ExistingLoggerStatus(logger_name, - LoggerUsesSink(existing_logger, sink)); + LoggerUsesSink(existing_logger, sink)); } if (!sink) { - std::string error_msg = - "Failed to create shared logger '" + logger_name + - "': shared sink is null"; + std::string error_msg = "Failed to create shared logger '" + + logger_name + "': shared sink is null"; std::cerr << error_msg << std::endl; return CreationStatus::kError; } @@ -115,7 +112,7 @@ CreationStatus CreateFileLogger(const std::string &logger_name, std::lock_guard lock(logger_creation_mutex); if (auto existing_logger = spdlog::get(logger_name)) { return ExistingLoggerStatus(logger_name, - LoggerUsesFile(existing_logger, filepath)); + LoggerUsesFile(existing_logger, filepath)); } try { spdlog::cfg::load_env_levels(); @@ -140,8 +137,7 @@ CreationStatus CreateSharedFileSink(const std::string &filepath) { auto sink = LoggingRegistry::GetSharedSink(filepath, &created); if (sink) { LoggingRegistry::SetDefaultSinkPath(filepath); - return created ? CreationStatus::kSuccess - : CreationStatus::kExists; + return created ? CreationStatus::kSuccess : CreationStatus::kExists; } std::string error_msg = "Failed to create shared file sink: sink is null"; diff --git a/src/model_factory.cpp b/src/model_factory.cpp index 1d9869ae..09e1d77c 100644 --- a/src/model_factory.cpp +++ b/src/model_factory.cpp @@ -4,8 +4,8 @@ // Created Date: 2025-07-07 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-09-14 // -// Modified By: Dimitri Baptiste // +// Last Modified: 2026-09-24 // +// Modified By: Matthew Carroll // // ----- // // Copyright (c) 2025-2026 Syndemics Lab at Boston Medical Center // //////////////////////////////////////////////////////////////////////////////// @@ -34,9 +34,10 @@ std::unique_ptr Model::Create(const std::string &name, processor_count); } -std::unique_ptr Model::Create( - const std::string &name, const ExecutionConfig &execution_config, - const std::string &log_name, const std::string &log_filepath) { +std::unique_ptr Model::Create(const std::string &name, + const ExecutionConfig &execution_config, + const std::string &log_name, + const std::string &log_filepath) { return std::make_unique(name, log_name, log_filepath, execution_config); } diff --git a/src/transition_factory.cpp b/src/transition_factory.cpp index 4f25e297..fecaaf14 100644 --- a/src/transition_factory.cpp +++ b/src/transition_factory.cpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-05 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-14 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // @@ -33,7 +33,7 @@ std::unique_ptr Transition::Create(const std::string &type, std::unique_ptr Transition::Create(const std::string &type, const std::string &name, - const LoggingConfig &logging_config) { + const LoggingConfig &logging_config) { std::string type_copy = type; std::transform(type_copy.begin(), type_copy.end(), type_copy.begin(), [](unsigned char c) { return std::tolower(c); }); diff --git a/tests/unit/background_test.cpp b/tests/unit/background_test.cpp index ecf083c9..5c82529a 100644 --- a/tests/unit/background_test.cpp +++ b/tests/unit/background_test.cpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-06 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-13 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // @@ -187,7 +187,8 @@ TEST_F(BackgroundDeathTest, AcceptsEqualElementTransposedMatricesWithWarning) { const Eigen::VectorXd expected_state = six_state - six_state.cwiseProduct( Eigen::Map(matrix.data(), 6)); - const Eigen::VectorXd result = background_death.Execute(six_state, histories); + const Eigen::VectorXd result = + background_death.Execute(six_state, histories); EXPECT_TRUE(result.isApprox(expected_state)); } diff --git a/tests/unit/behavior_test.cpp b/tests/unit/behavior_test.cpp index e89b71f1..8e2a67b9 100644 --- a/tests/unit/behavior_test.cpp +++ b/tests/unit/behavior_test.cpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-06 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-13 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // diff --git a/tests/unit/cost_effectiveness_test.cpp b/tests/unit/cost_effectiveness_test.cpp index 439b4055..e60a8f8b 100644 --- a/tests/unit/cost_effectiveness_test.cpp +++ b/tests/unit/cost_effectiveness_test.cpp @@ -4,7 +4,7 @@ // Created Date: 2025-06-06 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-02-06 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2025-2026 Syndemics Lab at Boston Medical Center // diff --git a/tests/unit/history_test.cpp b/tests/unit/history_test.cpp index 05e9fa71..0ba3bca4 100644 --- a/tests/unit/history_test.cpp +++ b/tests/unit/history_test.cpp @@ -4,7 +4,7 @@ // Created Date: 2026-05-05 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-13 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // //////////////////////////////////////////////////////////////////////////////// @@ -385,8 +385,7 @@ TEST_F(HistoryTest, OutOfOrderStateIsInsertedChronologically) { history.AddState(state_at_five, 5); history.AddState(state_at_two, 2); - ASSERT_EQ(history.GetRecordedTimesteps(), - (std::vector{2, 5})); + ASSERT_EQ(history.GetRecordedTimesteps(), (std::vector{2, 5})); ASSERT_EQ(history.GetLatestRecordedTimestep(), 5); const auto states = history.GetStateAsVector(); diff --git a/tests/unit/intervention_test.cpp b/tests/unit/intervention_test.cpp index 94744991..160a8804 100644 --- a/tests/unit/intervention_test.cpp +++ b/tests/unit/intervention_test.cpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-06 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-13 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // diff --git a/tests/unit/logging_test.cpp b/tests/unit/logging_test.cpp index df90d5df..154fe43c 100644 --- a/tests/unit/logging_test.cpp +++ b/tests/unit/logging_test.cpp @@ -4,7 +4,7 @@ // Created Date: 2025-03-18 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-14 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2025-2026 Syndemics Lab at Boston Medical Center // @@ -159,14 +159,12 @@ TEST_F(LoggingTest, ConfigureLoggerCreatesSharedLogger) { TEST_F(LoggingTest, ConfigureLoggerRejectsSharedDestinationChange) { const std::string other_log_file = "/tmp/respond_other_shared.log"; std::remove(other_log_file.c_str()); - ASSERT_EQ(ConfigureLogger( - LoggingConfig{"configured_shared_logger", shared_log_file_, - true}), + ASSERT_EQ(ConfigureLogger(LoggingConfig{"configured_shared_logger", + shared_log_file_, true}), CreationStatus::kSuccess); - EXPECT_EQ(ConfigureLogger( - LoggingConfig{"configured_shared_logger", other_log_file, - true}), + EXPECT_EQ(ConfigureLogger(LoggingConfig{"configured_shared_logger", + other_log_file, true}), CreationStatus::kError); std::remove(other_log_file.c_str()); @@ -176,15 +174,13 @@ TEST_F(LoggingTest, ConcurrentSharedLoggerConfigurationUsesRequestedSinks) { constexpr size_t logger_count = 8; std::vector log_files; std::vector logger_names; - std::vector statuses(logger_count, - CreationStatus::kError); + std::vector statuses(logger_count, CreationStatus::kError); std::vector workers; for (size_t i = 0; i < logger_count; ++i) { log_files.push_back("/tmp/respond_concurrent_shared_" + std::to_string(i) + ".log"); - logger_names.push_back("concurrent_shared_logger_" + - std::to_string(i)); + logger_names.push_back("concurrent_shared_logger_" + std::to_string(i)); std::remove(log_files.back().c_str()); } @@ -408,8 +404,7 @@ TEST_F(LoggingTest, CheckLoggerExistsFalse) { TEST_F(LoggingTest, MissingLoggerDoesNotCreateFallbackLogger) { LogInfo("missing_logger", "message without a configured logger"); - EXPECT_EQ(CheckLoggerExists("missing_logger"), - CreationStatus::kNotCreated); + EXPECT_EQ(CheckLoggerExists("missing_logger"), CreationStatus::kNotCreated); } TEST_F(LoggingTest, GetLoggerInfo) { diff --git a/tests/unit/markov_test.cpp b/tests/unit/markov_test.cpp index fdfdd69b..8f2dd9cb 100644 --- a/tests/unit/markov_test.cpp +++ b/tests/unit/markov_test.cpp @@ -4,8 +4,8 @@ // Created Date: 2025-06-06 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-09-14 // -// Modified By: Dimitri Baptiste // +// Last Modified: 2026-09-24 // +// Modified By: Matthew Carroll // // ----- // // Copyright (c) 2025-2026 Syndemics Lab at Boston Medical Center // //////////////////////////////////////////////////////////////////////////////// diff --git a/tests/unit/migration_test.cpp b/tests/unit/migration_test.cpp index 4d4d0889..487c4c91 100644 --- a/tests/unit/migration_test.cpp +++ b/tests/unit/migration_test.cpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-06 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-07-13 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // diff --git a/tests/unit/simulation_test.cpp b/tests/unit/simulation_test.cpp index 1a63a0a9..ef2a5e4e 100644 --- a/tests/unit/simulation_test.cpp +++ b/tests/unit/simulation_test.cpp @@ -4,7 +4,7 @@ // Created Date: 2026-02-09 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-08-19 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // @@ -108,9 +108,9 @@ TEST_F(SimulationTest, ConstructorWithLogNameAndLogFile) { } TEST_F(SimulationTest, ConstructorRejectsLoggerInitializationFailure) { - ASSERT_EQ(CreateFileLogger("conflicting_simulation_logger", - default_log_file_), - CreationStatus::kSuccess); + ASSERT_EQ( + CreateFileLogger("conflicting_simulation_logger", default_log_file_), + CreationStatus::kSuccess); RuntimeConfig config; config.logging.logger_name = "conflicting_simulation_logger"; config.logging.file_path = test_log_file_; @@ -162,8 +162,7 @@ TEST_F(SimulationTest, StoresRuntimeConfig) { Simulation s(config); EXPECT_EQ(s.GetRuntimeConfig().execution.total_threads, 4); - EXPECT_EQ(s.GetRuntimeConfig().logging.logger_name, - "runtime_simulation"); + EXPECT_EQ(s.GetRuntimeConfig().logging.logger_name, "runtime_simulation"); EXPECT_EQ(s.GetRuntimeConfig().logging.file_path, test_log_file_); } @@ -335,9 +334,9 @@ TEST_F(SimulationTest, RunsModelsConcurrentlyWhenEnabled) { auto run_model = [&]() { const int active = entered.fetch_add(1) + 1; int observed_maximum = maximum_active.load(); - while (active > observed_maximum && - !maximum_active.compare_exchange_weak(observed_maximum, - active)) { + while ( + active > observed_maximum && + !maximum_active.compare_exchange_weak(observed_maximum, active)) { } while (entered.load() < 2) { std::this_thread::yield(); @@ -486,8 +485,9 @@ TEST_F(SimulationTest, RethrowsWorkerExceptionAfterJoining) { auto completing_source = std::make_unique>(); auto completing_model = std::make_unique>(); - EXPECT_CALL(*completing_model, RunTimesteps()) - .WillOnce([&]() { completed.fetch_add(1); }); + EXPECT_CALL(*completing_model, RunTimesteps()).WillOnce([&]() { + completed.fetch_add(1); + }); EXPECT_CALL(*completing_source, clone()) .WillOnce(Return(::testing::ByMove(std::move(completing_model)))); simulation.AddModel(std::move(completing_source)); diff --git a/tests/unit/timestep_test.cpp b/tests/unit/timestep_test.cpp index 503ae67b..d84ac702 100644 --- a/tests/unit/timestep_test.cpp +++ b/tests/unit/timestep_test.cpp @@ -4,7 +4,7 @@ // Created Date: 2026-07-06 // // Author: Matthew Carroll // // ----- // -// Last Modified: 2026-08-19 // +// Last Modified: 2026-09-24 // // Modified By: Matthew Carroll // // ----- // // Copyright (c) 2026 Syndemics Lab at Boston Medical Center // @@ -47,7 +47,6 @@ class TimestepTest : public ::testing::Test { std::string test_log_file_; std::string shared_log_file_; std::string default_log_file_; - }; TEST_F(TimestepTest, DefaultConstructor) { @@ -93,8 +92,8 @@ TEST_F(TimestepTest, CreateTransitionUsesLoggingConfig) { TEST_F(TimestepTest, ClonedTransitionPreservesLoggingConfig) { const LoggingConfig logging_config{"clone_transition", test_log_file_, false}; - auto transition = Transition::Create("migration", "migration", - logging_config); + auto transition = + Transition::Create("migration", "migration", logging_config); auto clone = transition->clone(); ASSERT_NE(clone, nullptr); @@ -102,10 +101,9 @@ TEST_F(TimestepTest, ClonedTransitionPreservesLoggingConfig) { } TEST_F(TimestepTest, CreateTransitionRejectsUnsupportedType) { - EXPECT_THROW( - Transition::Create("unsupported", "unsupported", "test_log", - test_log_file_), - std::invalid_argument); + EXPECT_THROW(Transition::Create("unsupported", "unsupported", "test_log", + test_log_file_), + std::invalid_argument); } TEST_F(TimestepTest, AddTransitionClonesInputTransition) { @@ -165,8 +163,7 @@ TEST_F(TimestepTest, AddMatrixToTransitionByMissingNameThrows) { Timestep ts("test_log", test_log_file_); Eigen::MatrixXd m = Eigen::MatrixXd::Identity(2, 2); - EXPECT_THROW(ts.AddMatrixToTransition("missing", m), - std::invalid_argument); + EXPECT_THROW(ts.AddMatrixToTransition("missing", m), std::invalid_argument); } TEST_F(TimestepTest, RemoveTransition) { @@ -243,8 +240,9 @@ TEST_F(TimestepTest, MovePreservesLoggerNameWithoutRecreatingLogger) { Timestep moved(std::move(original)); spdlog::drop("test_log"); - EXPECT_THROW(moved.AddMatrixToTransition(0, Eigen::MatrixXd::Identity(1, 1)), - std::out_of_range); + EXPECT_THROW( + moved.AddMatrixToTransition(0, Eigen::MatrixXd::Identity(1, 1)), + std::out_of_range); EXPECT_EQ(CheckLoggerExists("test_log"), CreationStatus::kNotCreated); }