From abf68bba72b9de68189548f690f74f9e1b582dfe Mon Sep 17 00:00:00 2001 From: Alan Garny Date: Fri, 9 Oct 2026 02:30:13 +1300 Subject: [PATCH 1/3] New version. --- VERSION.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/VERSION.txt b/VERSION.txt index 9eab32c20..c88490b8f 100644 --- a/VERSION.txt +++ b/VERSION.txt @@ -1 +1 @@ -1.20261009.1 +1.20261009.2 From 4653616993fc44f6e12a2b5e6780efea275b1317 Mon Sep 17 00:00:00 2001 From: Alan Garny Date: Fri, 9 Oct 2026 02:53:44 +1300 Subject: [PATCH 2/3] Hardened SED-ML uniform time-course execution. --- src/api/libopencor/seduniformtimecourse.h | 1 + src/sed/sedinstance.cpp | 28 ++- src/sed/sedinstancetask.cpp | 107 ++++++++---- src/sed/sedsimulation.cpp | 30 +--- src/sed/sedsimulation_p.h | 2 - src/sed/seduniformtimecourse.cpp | 58 +++++++ src/sed/seduniformtimecourse_p.h | 2 + tests/api/sed/instancetests.cpp | 154 +++++++++++++++++ .../bindings/javascript/sed.instance.test.js | 163 ++++++++++++++++++ tests/bindings/python/test_sed_instance.py | 162 +++++++++++++++++ 10 files changed, 646 insertions(+), 61 deletions(-) diff --git a/src/api/libopencor/seduniformtimecourse.h b/src/api/libopencor/seduniformtimecourse.h index 823516a22..be9c9579c 100644 --- a/src/api/libopencor/seduniformtimecourse.h +++ b/src/api/libopencor/seduniformtimecourse.h @@ -30,6 +30,7 @@ namespace libOpenCOR { class LIBOPENCOR_EXPORT SedUniformTimeCourse: public SedSimulation { friend class SedInstanceTask; + friend class SedSimulation; public: /** diff --git a/src/sed/sedinstance.cpp b/src/sed/sedinstance.cpp index 654d99573..6ea4cdae2 100644 --- a/src/sed/sedinstance.cpp +++ b/src/sed/sedinstance.cpp @@ -20,6 +20,7 @@ limitations under the License. #include "libopencor/seddocument.h" +#include #include #include #include @@ -200,13 +201,32 @@ bool SedInstance::Impl::startRun() mRunning.store(true, std::memory_order_release); - mRunFuture = std::async(std::launch::async, [this]() { - const auto result = run(); + // Start our run in a separate thread. + // Note #1: we must be flagged as not running anymore once our run is done, even if run() throws an exception (it + // reports a failure as an issue, but it might still throw, e.g., std::bad_alloc when restoring our + // issues), hence we use a guard to do so. Otherwise, we would be stuck in RUNNING. + // Note #2: std::async() may throw an exception (e.g., std::system_error if no thread could be created), in which + // case we must also be flagged as not running anymore. We have no way to trigger such an exception in our + // tests, hence we ignore our try...catch statement during code coverage. +#ifndef CODE_COVERAGE_ENABLED + try { +#endif + mRunFuture = std::async(std::launch::async, [this]() { + auto resetRunning = [](std::atomic *pRunning) { + pRunning->store(false, std::memory_order_release); + }; + const std::unique_ptr, decltype(resetRunning)> runningGuard {&mRunning, resetRunning}; + + return run(); + }); +#ifndef CODE_COVERAGE_ENABLED + } catch (...) { mRunning.store(false, std::memory_order_release); - return result; - }); + throw; + } +#endif return true; } diff --git a/src/sed/sedinstancetask.cpp b/src/sed/sedinstancetask.cpp index 434cd3ee0..35606d999 100644 --- a/src/sed/sedinstancetask.cpp +++ b/src/sed/sedinstancetask.cpp @@ -27,8 +27,10 @@ limitations under the License. #include #include #include +#include #include #include +#include #include namespace libOpenCOR { @@ -434,14 +436,17 @@ void SedInstanceTask::Impl::run(double pVoiStart, double pVoiEnd, double pVoiInt }; // Compute the differential model. + // Note: when tracking our results, we take exactly as many steps as our results can hold (see run() below). auto *odeSolverPimpl {mOdeSolver->pimpl()}; + const auto lastVoiCounter {pTrackResults ? mResults.resultsSize - 1 : 0}; size_t voiCounter {0}; + auto voiEndReached {pTrackResults ? (lastVoiCounter == 0) : fuzzyCompare(mVoi, pVoiEnd)}; const auto computeRates = mRuntime->computeRates(); const auto computeVariablesForDifferentialModel = mRuntime->computeVariablesForDifferentialModel(); - while (!fuzzyCompare(mVoi, pVoiEnd)) { + while (!voiEndReached) { // Check whether a pause or stop has been requested. const auto runControl = mRunControl->load(std::memory_order_relaxed); @@ -468,9 +473,21 @@ void SedInstanceTask::Impl::run(double pVoiStart, double pVoiEnd, double pVoiInt return; } + // Determine the point that we want to reach. + // Note: our last point is always exactly pVoiEnd. Indeed, pVoiStart + n * pVoiInterval may differ slightly from + // pVoiEnd due to rounding errors, which would otherwise result in an extra step being taken and, when + // tracking our results, its results being written past the end of our results arrays. + + auto voiTarget {pVoiStart + static_cast(++voiCounter) * pVoiInterval}; + + voiEndReached = pTrackResults ? + (voiCounter == lastVoiCounter) : + ((voiTarget >= pVoiEnd) || fuzzyCompare(voiTarget, pVoiEnd)); + voiTarget = voiEndReached ? pVoiEnd : std::min(voiTarget, pVoiEnd); + // Update our model's state. - if (!odeSolverPimpl->solve(mVoi, std::min(pVoiStart + static_cast(++voiCounter) * pVoiInterval, pVoiEnd))) { + if (!odeSolverPimpl->solve(mVoi, voiTarget)) { // Note: an NLA system that could not be solved is a likely reason for our ODE solver to have failed (see // rhsFunction() in solvercvode.cpp). @@ -526,25 +543,36 @@ double SedInstanceTask::Impl::run() auto startTime {std::chrono::high_resolution_clock::now()}; // Reset our progress counters. - // Note: we retrieve our number of steps only once since it is used in several places below. - - const auto *sedUniformTimeCoursePimpl {mDifferentialModel ? mSedUniformTimeCourse->pimpl() : nullptr}; - const auto numberOfSteps {mDifferentialModel ? sedUniformTimeCoursePimpl->mNumberOfSteps : 1}; - const auto totalSteps {(numberOfSteps > 0) ? static_cast(numberOfSteps) : 0}; mCompletedSteps.store(0, std::memory_order_relaxed); - mTotalSteps.store(totalSteps, std::memory_order_relaxed); + mTotalSteps.store(0, std::memory_order_relaxed); - // Make sure that our number of steps is valid. - // Note: our number of steps was validated when we were instantiated (see SedSimulation::Impl::isValid()), but it - // may have been changed since then. + // Make sure that our simulation is valid. + // Note: our simulation was validated when we were instantiated (see SedSimulation::Impl::isValid()), but its times + // and/or number of steps may have been changed since then. - if (numberOfSteps <= 0) { - addIssue(Issue::Type::ERROR, sedUniformTimeCoursePimpl->invalidNumberOfStepsError(mModel, numberOfSteps), "Simulation"); + const auto *sedUniformTimeCoursePimpl {mDifferentialModel ? mSedUniformTimeCourse->pimpl() : nullptr}; - return 0.0; + if (mDifferentialModel) { + const auto errors {sedUniformTimeCoursePimpl->validationErrors(mModel)}; + + if (!errors.empty()) { + for (const auto &error : errors) { + addIssue(Issue::Type::ERROR, error, "Simulation"); + } + + return 0.0; + } } + // Set our total number of steps. + // Note: we retrieve our number of steps only once since it is used in several places below. + + const auto numberOfSteps {mDifferentialModel ? sedUniformTimeCoursePimpl->mNumberOfSteps : 1}; + const auto totalSteps {static_cast(numberOfSteps)}; + + mTotalSteps.store(totalSteps, std::memory_order_relaxed); + // (Re)initialise our model. // Note: reinitialise our model because we initialised it when we created the instance task. @@ -566,29 +594,48 @@ double SedInstanceTask::Impl::run() } } - // Initialise our results structure. - // Note: we release our previous results first (to limit our peak memory usage) and only then allocate our new - // results, which we do into a local structure that we move into ours once all of its arrays have been - // allocated. This means that should an allocation fail (e.g., std::bad_alloc), our results structure - // would be left empty rather than with an inconsistent results size and arrays of mismatched sizes (which - // would result in out-of-bounds accesses when retrieving our results). + // Initialise our results structure, if needed. + // Note #1: we only (re)allocate our results if their size has changed. This avoids needless allocations, but + // it also means that our results remain where they are in memory, which matters since our Python + // bindings return zero-copy NumPy arrays. + // Note #2: when (re)allocating our results, we release our previous results first (to limit our peak memory + // usage) and only then allocate our new results, which we do into a local structure that we move into + // ours once all of its arrays have been allocated. This means that should an allocation fail (e.g., + // std::bad_alloc), our results structure would be left empty rather than with an inconsistent results + // size and arrays of mismatched sizes (which would result in out-of-bounds accesses when retrieving + // our results). + // Note #3: on a 32-bit platform (e.g., WebAssembly), the size of an array may overflow (e.g., with many + // constants and a large number of steps), in which case we would allocate an array that is too small + // for our results, hence we check for it. We cannot trigger such an overflow on a 64-bit platform, so + // we ignore our check during code coverage. const auto resultsSize {totalSteps + 1}; - mResults = {}; + if (mResults.resultsSize != resultsSize) { + mResults = {}; + + auto arraySize = [resultsSize](size_t pCount) { +#ifndef CODE_COVERAGE_ENABLED + if ((pCount != 0) && (resultsSize > (std::numeric_limits::max() / pCount))) { + throw std::length_error("the results are too large to be allocated"); + } +#endif - SedInstanceTaskResults results; + return pCount * resultsSize; + }; + SedInstanceTaskResults results; - results.resultsSize = resultsSize; + results.resultsSize = resultsSize; - results.voi.resize(resultsSize); - results.states.resize(mStateCount * resultsSize); - results.rates.resize(mStateCount * resultsSize); - results.constants.resize(mConstantCount * resultsSize); - results.computedConstants.resize(mComputedConstantCount * resultsSize); - results.algebraicVariables.resize(mAlgebraicVariableCount * resultsSize); + results.voi.resize(resultsSize); + results.states.resize(arraySize(mStateCount)); + results.rates.resize(arraySize(mStateCount)); + results.constants.resize(arraySize(mConstantCount)); + results.computedConstants.resize(arraySize(mComputedConstantCount)); + results.algebraicVariables.resize(arraySize(mAlgebraicVariableCount)); - mResults = std::move(results); + mResults = std::move(results); + } // Run our simulation from the output start time to the output end time, tracking our results. diff --git a/src/sed/sedsimulation.cpp b/src/sed/sedsimulation.cpp index 96e87e4bb..a2d3d75fb 100644 --- a/src/sed/sedsimulation.cpp +++ b/src/sed/sedsimulation.cpp @@ -22,8 +22,6 @@ limitations under the License. #include "solvernla_p.h" #include "solverode_p.h" -#include "utils.h" - namespace libOpenCOR { static constexpr auto ID_PREFIX {"simulation"}; @@ -33,25 +31,6 @@ SedSimulation::Impl::Impl(const SedDocumentPtr &pDocument) { } -std::string SedSimulation::Impl::invalidNumberOfStepsError(const SedModelPtr &pModel, int pNumberOfSteps) const -{ - const auto &modelId = pModel->pimpl()->mId; - const auto numberOfSteps {toString(pNumberOfSteps)}; - std::string res; - - res.reserve(mId.size() + modelId.size() + numberOfSteps.size() + 112); // NOLINT - - res += "Simulation '"; - res += mId; - res += "' is to be used with model '"; - res += modelId; - res += "' which requires a strictly positive number of steps but "; - res += numberOfSteps; - res += " is provided."; - - return res; -} - bool SedSimulation::Impl::isValid(const SedModelPtr &pModel, const SedUniformTimeCoursePtr &pUniformTimeCourse) { auto modelType {pModel->pimpl()->mFile->pimpl()->mCellmlFile->type()}; @@ -77,8 +56,7 @@ bool SedSimulation::Impl::isValid(const SedModelPtr &pModel, const SedUniformTim removeAllIssues(); - // Make sure that we are a uniform time course simulation with a strictly positive number of steps if our model is - // an ODE/DAE model. + // Make sure that we are a valid uniform time course simulation if our model is an ODE/DAE model. //---GRY--- WE DON'T CURRENTLY SUPPORT OTHER TYPES OF SIMULATION FOR ODE/DAE MODELS. if ((modelType == libcellml::AnalyserModel::Type::ODE) @@ -95,8 +73,10 @@ bool SedSimulation::Impl::isValid(const SedModelPtr &pModel, const SedUniformTim error += "' which (currently) requires a uniform time course simulation."; addError(error); - } else if (pUniformTimeCourse->numberOfSteps() <= 0) { - addError(invalidNumberOfStepsError(pModel, pUniformTimeCourse->numberOfSteps())); + } else { + for (const auto &error : pUniformTimeCourse->pimpl()->validationErrors(pModel)) { + addError(error); + } } } diff --git a/src/sed/sedsimulation_p.h b/src/sed/sedsimulation_p.h index 7e7509be7..afdfe34fc 100644 --- a/src/sed/sedsimulation_p.h +++ b/src/sed/sedsimulation_p.h @@ -30,8 +30,6 @@ class SedSimulation::Impl: public SedBase::Impl explicit Impl(const SedDocumentPtr &pDocument); - std::string invalidNumberOfStepsError(const SedModelPtr &pModel, int pNumberOfSteps) const; - bool isValid(const SedModelPtr &pModel, const SedUniformTimeCoursePtr &pUniformTimeCourse); const SolverOdePtr &odeSolver() const; diff --git a/src/sed/seduniformtimecourse.cpp b/src/sed/seduniformtimecourse.cpp index ae8f623f2..2d3cc2d1a 100644 --- a/src/sed/seduniformtimecourse.cpp +++ b/src/sed/seduniformtimecourse.cpp @@ -18,6 +18,9 @@ limitations under the License. #include "utils.h" +#include "libopencor/sedmodel.h" + +#include #include namespace libOpenCOR { @@ -27,6 +30,61 @@ SedUniformTimeCourse::Impl::Impl(const SedDocumentPtr &pDocument) { } +Strings SedUniformTimeCourse::Impl::validationErrors(const SedModelPtr &pModel) const +{ + // Make sure that our times are finite and such that initialTime <= outputStartTime < outputEndTime, and that our + // number of steps is strictly positive. + // Note: we are validated both when instantiating a document (see SedSimulation::Impl::isValid()) and when running + // an instance task (see SedInstanceTask::Impl::run()) since our times and number of steps may have been + // changed in between. + + const auto &modelId = pModel->id(); + Strings res; + + if (!std::isfinite(mInitialTime) || !std::isfinite(mOutputStartTime) || !std::isfinite(mOutputEndTime) + || (mInitialTime > mOutputStartTime) || (mOutputStartTime >= mOutputEndTime)) { + const auto initialTime {toString(mInitialTime)}; + const auto outputStartTime {toString(mOutputStartTime)}; + const auto outputEndTime {toString(mOutputEndTime)}; + std::string error; + + error.reserve(mId.size() + modelId.size() + initialTime.size() + outputStartTime.size() + outputEndTime.size() + 152); // NOLINT + + error += "Simulation '"; + error += mId; + error += "' is to be used with model '"; + error += modelId; + error += "' which requires finite times such that initialTime <= outputStartTime < outputEndTime but "; + error += initialTime; + error += ", "; + error += outputStartTime; + error += ", and "; + error += outputEndTime; + error += " are provided."; + + res.push_back(error); + } + + if (mNumberOfSteps <= 0) { + const auto numberOfSteps {toString(mNumberOfSteps)}; + std::string error; + + error.reserve(mId.size() + modelId.size() + numberOfSteps.size() + 112); // NOLINT + + error += "Simulation '"; + error += mId; + error += "' is to be used with model '"; + error += modelId; + error += "' which requires a strictly positive number of steps but "; + error += numberOfSteps; + error += " is provided."; + + res.push_back(error); + } + + return res; +} + double SedUniformTimeCourse::Impl::initialTime() const { return mInitialTime; diff --git a/src/sed/seduniformtimecourse_p.h b/src/sed/seduniformtimecourse_p.h index baf77020c..159ac4c9b 100644 --- a/src/sed/seduniformtimecourse_p.h +++ b/src/sed/seduniformtimecourse_p.h @@ -32,6 +32,8 @@ class SedUniformTimeCourse::Impl: public SedSimulation::Impl explicit Impl(const SedDocumentPtr &pDocument); + Strings validationErrors(const SedModelPtr &pModel) const; + double initialTime() const; void setInitialTime(double pInitialTime); diff --git a/tests/api/sed/instancetests.cpp b/tests/api/sed/instancetests.cpp index 62a2b7928..93e061687 100644 --- a/tests/api/sed/instancetests.cpp +++ b/tests/api/sed/instancetests.cpp @@ -704,6 +704,160 @@ TEST(InstanceSedTest, odeModelWithInvalidNumberOfSteps) EXPECT_EQ(instanceTask->voi().size(), 501); } +TEST(InstanceSedTest, odeModelWithInvalidTimes) +{ + static const libOpenCOR::ExpectedIssues INITIAL_TIME_EXPECTED_ISSUES {{ + {libOpenCOR::Issue::Type::ERROR, "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires finite times such that initialTime <= outputStartTime < outputEndTime but 5, 0, and 50 are provided."}, + }}; + static const libOpenCOR::ExpectedIssues OUTPUT_END_TIME_EXPECTED_ISSUES {{ + {libOpenCOR::Issue::Type::ERROR, "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires finite times such that initialTime <= outputStartTime < outputEndTime but 0, 5, and 5 are provided."}, + }}; + static const libOpenCOR::ExpectedIssues TIMES_AND_STEPS_EXPECTED_ISSUES {{ + {libOpenCOR::Issue::Type::ERROR, "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires finite times such that initialTime <= outputStartTime < outputEndTime but 0, 5, and 5 are provided."}, + {libOpenCOR::Issue::Type::ERROR, "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires a strictly positive number of steps but 0 is provided."}, + }}; + + static const auto FIVE {5.0}; + static const auto FIFTY {50.0}; + + auto file {libOpenCOR::File::create(libOpenCOR::resourcePath("cellml_2.cellml"))}; + auto document {libOpenCOR::SedDocument::create(file)}; + const auto &simulation {std::dynamic_pointer_cast(document->simulations()[0])}; + + // Initial time after the output start time. + + simulation->setInitialTime(FIVE); + simulation->setOutputStartTime(0.0); + simulation->setOutputEndTime(FIFTY); + + auto instance {document->instantiate()}; + + EXPECT_EQ_ISSUES(instance, INITIAL_TIME_EXPECTED_ISSUES); + + // Output start time equal to the output end time. + + simulation->setInitialTime(0.0); + simulation->setOutputStartTime(FIVE); + simulation->setOutputEndTime(FIVE); + + instance = document->instantiate(); + + EXPECT_EQ_ISSUES(instance, OUTPUT_END_TIME_EXPECTED_ISSUES); + + // Both invalid times and an invalid number of steps. + + simulation->setNumberOfSteps(0); + + instance = document->instantiate(); + + EXPECT_EQ_ISSUES(instance, TIMES_AND_STEPS_EXPECTED_ISSUES); + + // Non-finite times. + + simulation->setOutputEndTime(std::numeric_limits::quiet_NaN()); + simulation->setNumberOfSteps(10); // NOLINT + + instance = document->instantiate(); + + EXPECT_EQ(instance->errorCount(), 1); + + simulation->setOutputEndTime(std::numeric_limits::infinity()); + + instance = document->instantiate(); + + EXPECT_EQ(instance->errorCount(), 1); + + // Valid times, but then made invalid after instantiation. + + simulation->setOutputStartTime(0.0); + simulation->setOutputEndTime(FIFTY); + simulation->setNumberOfSteps(static_cast(FIFTY)); + + instance = document->instantiate(); + + EXPECT_FALSE(instance->hasIssues()); + EXPECT_GT(instance->run(), 0.0); + EXPECT_FALSE(instance->hasIssues()); + EXPECT_DOUBLE_EQ(instance->progress(), 1.0); + + simulation->setOutputStartTime(FIVE); + simulation->setOutputEndTime(FIVE); + + EXPECT_EQ(instance->run(), 0.0); + EXPECT_EQ_ISSUES(instance, OUTPUT_END_TIME_EXPECTED_ISSUES); + EXPECT_EQ(instance->progress(), 0.0); + + // Valid times again. + + simulation->setOutputStartTime(0.0); + simulation->setOutputEndTime(FIFTY); + + EXPECT_GT(instance->run(), 0.0); + EXPECT_FALSE(instance->hasIssues()); + EXPECT_DOUBLE_EQ(instance->progress(), 1.0); +} + +TEST(InstanceSedTest, odeModelResultsReusedWhenRunAgain) +{ + // Note: our Python bindings return zero-copy NumPy arrays, so make sure that our results are not reallocated when + // running an instance again with the same number of steps. + + auto file {libOpenCOR::File::create(libOpenCOR::resourcePath("cellml_2.cellml"))}; + auto document {libOpenCOR::SedDocument::create(file)}; + auto instance {document->instantiate()}; + + instance->run(); + + EXPECT_FALSE(instance->hasIssues()); + + const auto &instanceTask {instance->tasks()[0]}; + const auto voi {instanceTask->voi()}; + const auto state {instanceTask->state(0)}; + + instance->run(); + + EXPECT_FALSE(instance->hasIssues()); + EXPECT_EQ(instanceTask->voi().data(), voi.data()); + EXPECT_EQ(instanceTask->state(0).data(), state.data()); + EXPECT_EQ(voi[voi.size() - 1], instanceTask->voi()[voi.size() - 1]); + EXPECT_EQ(state[state.size() - 1], instanceTask->state(0)[state.size() - 1]); +} + +TEST(InstanceSedTest, odeModelWithRoundingErrorOnOutputEndTime) +{ + // Note: with an output start time of -1, an output end time of 0, and 49 steps, the time of the last step is computed + // as -1 + 49 * (1 / 49), which is not exactly 0 due to rounding errors. This used to result in an extra step + // being taken and its results being written past the end of our results arrays. + + auto file {libOpenCOR::File::create(libOpenCOR::resourcePath("cellml_2.cellml"))}; + auto document {libOpenCOR::SedDocument::create(file)}; + const auto &simulation {std::dynamic_pointer_cast(document->simulations()[0])}; + + static const auto INITIAL_TIME {-1.0}; + static const auto OUTPUT_END_TIME {0.0}; + static const auto NUMBER_OF_STEPS {49}; + + simulation->setInitialTime(INITIAL_TIME); + simulation->setOutputStartTime(INITIAL_TIME); + simulation->setOutputEndTime(OUTPUT_END_TIME); + simulation->setNumberOfSteps(NUMBER_OF_STEPS); + + auto instance {document->instantiate()}; + + EXPECT_FALSE(instance->hasIssues()); + + instance->run(); + + EXPECT_FALSE(instance->hasIssues()); + EXPECT_DOUBLE_EQ(instance->progress(), 1.0); + + const auto &voi {instance->tasks()[0]->voi()}; + + EXPECT_EQ(voi.size(), NUMBER_OF_STEPS + 1); + EXPECT_EQ(voi[0], INITIAL_TIME); + EXPECT_EQ(voi[voi.size() - 1], OUTPUT_END_TIME); +} + TEST(InstanceSedTest, nlaModel) { static const libOpenCOR::ExpectedIssues EXPECTED_ISSUES {{ diff --git a/tests/bindings/javascript/sed.instance.test.js b/tests/bindings/javascript/sed.instance.test.js index 37772dba3..e15982b9b 100644 --- a/tests/bindings/javascript/sed.instance.test.js +++ b/tests/bindings/javascript/sed.instance.test.js @@ -772,6 +772,169 @@ test.describe('Sed instance tests', () => { assert.strictEqual(instanceTask.voi.length, 501); }); + test('ODE model with invalid times', () => { + const file = new loc.File(utils.resourcePath('cellml_2.cellml')); + + file.setContents(utils.fileContents(file.path)); + + const document = new loc.SedDocument(file); + const simulation = document.simulations.get(0); + + // Initial time after the output start time. + + simulation.initialTime = 5.0; + simulation.outputStartTime = 0.0; + simulation.outputEndTime = 50.0; + + let instance = document.instantiate(); + + assertIssues(loc, instance, [ + [ + loc.Issue.Type.ERROR, + "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires finite times such that initialTime <= outputStartTime < outputEndTime but 5, 0, and 50 are provided." + ] + ]); + + // Output start time equal to the output end time. + + simulation.initialTime = 0.0; + simulation.outputStartTime = 5.0; + simulation.outputEndTime = 5.0; + + instance = document.instantiate(); + + const outputEndTimeExpectedIssues = [ + [ + loc.Issue.Type.ERROR, + "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires finite times such that initialTime <= outputStartTime < outputEndTime but 0, 5, and 5 are provided." + ] + ]; + + assertIssues(loc, instance, outputEndTimeExpectedIssues); + + // Both invalid times and an invalid number of steps. + + simulation.numberOfSteps = 0; + + instance = document.instantiate(); + + assertIssues(loc, instance, [ + [ + loc.Issue.Type.ERROR, + "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires finite times such that initialTime <= outputStartTime < outputEndTime but 0, 5, and 5 are provided." + ], + [ + loc.Issue.Type.ERROR, + "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires a strictly positive number of steps but 0 is provided." + ] + ]); + + // Non-finite times. + + simulation.outputEndTime = Number.NaN; + simulation.numberOfSteps = 10; + + instance = document.instantiate(); + + assert.strictEqual(instance.errorCount, 1); + + simulation.outputEndTime = Number.POSITIVE_INFINITY; + + instance = document.instantiate(); + + assert.strictEqual(instance.errorCount, 1); + + // Valid times, but then made invalid after instantiation. + + simulation.outputStartTime = 0.0; + simulation.outputEndTime = 50.0; + simulation.numberOfSteps = 50; + + instance = document.instantiate(); + + assert.strictEqual(instance.hasIssues, false); + assert.ok(instance.run() > 0.0); + assert.strictEqual(instance.hasIssues, false); + assert.strictEqual(instance.progress, 1.0); + + simulation.outputStartTime = 5.0; + simulation.outputEndTime = 5.0; + + assert.strictEqual(instance.run(), 0.0); + assertIssues(loc, instance, outputEndTimeExpectedIssues); + assert.strictEqual(instance.progress, 0.0); + + // Valid times again. + + simulation.outputStartTime = 0.0; + simulation.outputEndTime = 50.0; + + assert.ok(instance.run() > 0.0); + assert.strictEqual(instance.hasIssues, false); + assert.strictEqual(instance.progress, 1.0); + }); + + test('ODE model results reused when run again', () => { + // Note: our results are returned as views on the WebAssembly heap, so make sure that they remain valid when running + // an instance again with the same number of steps. + + const file = new loc.File(utils.resourcePath('cellml_2.cellml')); + + file.setContents(utils.fileContents(file.path)); + + const document = new loc.SedDocument(file); + const instance = document.instantiate(); + + instance.run(); + + assert.strictEqual(instance.hasIssues, false); + + const instanceTask = instance.tasks[0]; + const voi = instanceTask.voi; + const state = instanceTask.state(0); + + instance.run(); + + assert.strictEqual(instance.hasIssues, false); + assert.strictEqual(voi.byteOffset, instanceTask.voi.byteOffset); + assert.strictEqual(state.byteOffset, instanceTask.state(0).byteOffset); + assert.strictEqual(voi[voi.length - 1], instanceTask.voi[voi.length - 1]); + assert.strictEqual(state[state.length - 1], instanceTask.state(0)[state.length - 1]); + }); + + test('ODE model with rounding error on output end time', () => { + // Note: with an output start time of -1, an output end time of 0, and 49 steps, the time of the last step is + // computed as -1 + 49 * (1 / 49), which is not exactly 0 due to rounding errors. This used to result in an + // extra step being taken and its results being written past the end of our results arrays. + + const file = new loc.File(utils.resourcePath('cellml_2.cellml')); + + file.setContents(utils.fileContents(file.path)); + + const document = new loc.SedDocument(file); + const simulation = document.simulations.get(0); + + simulation.initialTime = -1.0; + simulation.outputStartTime = -1.0; + simulation.outputEndTime = 0.0; + simulation.numberOfSteps = 49; + + const instance = document.instantiate(); + + assert.strictEqual(instance.hasIssues, false); + + instance.run(); + + assert.strictEqual(instance.hasIssues, false); + assert.strictEqual(instance.progress, 1.0); + + const voi = instance.tasks[0].voi; + + assert.strictEqual(voi.length, 50); + assert.strictEqual(voi[0], -1.0); + assert.strictEqual(voi[voi.length - 1], 0.0); + }); + test('NLA model', () => { const file = new loc.File(utils.resourcePath('api/sed/nla.cellml')); diff --git a/tests/bindings/python/test_sed_instance.py b/tests/bindings/python/test_sed_instance.py index b9af1dfd3..5db5e711f 100644 --- a/tests/bindings/python/test_sed_instance.py +++ b/tests/bindings/python/test_sed_instance.py @@ -695,6 +695,168 @@ def test_ode_model_with_invalid_number_of_steps(): assert len(instance_task.voi) == 501 +def test_ode_model_with_invalid_times(): + initial_time_expected_issues = [ + [ + loc.Issue.Type.Error, + "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires finite times such that initialTime <= outputStartTime < outputEndTime but 5, 0, and 50 are provided.", + ], + ] + output_end_time_expected_issues = [ + [ + loc.Issue.Type.Error, + "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires finite times such that initialTime <= outputStartTime < outputEndTime but 0, 5, and 5 are provided.", + ], + ] + times_and_steps_expected_issues = [ + [ + loc.Issue.Type.Error, + "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires finite times such that initialTime <= outputStartTime < outputEndTime but 0, 5, and 5 are provided.", + ], + [ + loc.Issue.Type.Error, + "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires a strictly positive number of steps but 0 is provided.", + ], + ] + + file = loc.File(utils.resource_path("cellml_2.cellml")) + document = loc.SedDocument(file) + simulation = document.simulations[0] + + # Initial time after the output start time. + + simulation.initial_time = 5.0 + simulation.output_start_time = 0.0 + simulation.output_end_time = 50.0 + + instance = document.instantiate() + + assert_issues(instance, initial_time_expected_issues) + + # Output start time equal to the output end time. + + simulation.initial_time = 0.0 + simulation.output_start_time = 5.0 + simulation.output_end_time = 5.0 + + instance = document.instantiate() + + assert_issues(instance, output_end_time_expected_issues) + + # Both invalid times and an invalid number of steps. + + simulation.number_of_steps = 0 + + instance = document.instantiate() + + assert_issues(instance, times_and_steps_expected_issues) + + # Non-finite times. + + simulation.output_end_time = math.nan + simulation.number_of_steps = 10 + + instance = document.instantiate() + + assert instance.error_count == 1 + + simulation.output_end_time = math.inf + + instance = document.instantiate() + + assert instance.error_count == 1 + + # Valid times, but then made invalid after instantiation. + + simulation.output_start_time = 0.0 + simulation.output_end_time = 50.0 + simulation.number_of_steps = 50 + + instance = document.instantiate() + + assert not instance.has_issues + assert instance.run() > 0.0 + assert not instance.has_issues + assert instance.progress == 1.0 + + simulation.output_start_time = 5.0 + simulation.output_end_time = 5.0 + + assert instance.run() == 0.0 + assert_issues(instance, output_end_time_expected_issues) + assert instance.progress == 0.0 + + # Valid times again. + + simulation.output_start_time = 0.0 + simulation.output_end_time = 50.0 + + assert instance.run() > 0.0 + assert not instance.has_issues + assert instance.progress == 1.0 + + +def test_ode_model_results_reused_when_run_again(): + # Note: our results are returned as zero-copy NumPy arrays, so make sure that they remain valid when running an + # instance again with the same number of steps. + + file = loc.File(utils.resource_path("cellml_2.cellml")) + document = loc.SedDocument(file) + instance = document.instantiate() + + instance.run() + + assert not instance.has_issues + + instance_task = instance.tasks[0] + voi = instance_task.voi + state = instance_task.state(0) + + instance.run() + + assert not instance.has_issues + assert ( + voi.__array_interface__["data"][0] + == instance_task.voi.__array_interface__["data"][0] + ) + assert ( + state.__array_interface__["data"][0] + == instance_task.state(0).__array_interface__["data"][0] + ) + assert voi[-1] == instance_task.voi[-1] + assert state[-1] == instance_task.state(0)[-1] + + +def test_ode_model_with_rounding_error_on_output_end_time(): + # Note: with an output start time of -1, an output end time of 0, and 49 steps, the time of the last step is computed + # as -1 + 49 * (1 / 49), which is not exactly 0 due to rounding errors. This used to result in an extra step + # being taken and its results being written past the end of our results arrays. + + file = loc.File(utils.resource_path("cellml_2.cellml")) + document = loc.SedDocument(file) + simulation = document.simulations[0] + + simulation.initial_time = -1.0 + simulation.output_start_time = -1.0 + simulation.output_end_time = 0.0 + simulation.number_of_steps = 49 + + instance = document.instantiate() + + assert not instance.has_issues + + instance.run() + + assert not instance.has_issues + assert instance.progress == 1.0 + + voi = instance.tasks[0].voi + + assert len(voi) == 50 + assert voi[0] == -1.0 + assert voi[-1] == 0.0 + + def test_nla_model(): expected_issues = [ [ From 3517c2733e4f229414827bb840f87b3afa2a4efd Mon Sep 17 00:00:00 2001 From: Alan Garny Date: Fri, 9 Oct 2026 03:03:56 +1300 Subject: [PATCH 3/3] Tightened the VOI end check and added invalid time tests. --- src/sed/sedinstancetask.cpp | 4 +--- tests/api/sed/instancetests.cpp | 15 +++++++++++++++ tests/bindings/javascript/sed.instance.test.js | 15 +++++++++++++++ tests/bindings/python/test_sed_instance.py | 15 +++++++++++++++ 4 files changed, 46 insertions(+), 3 deletions(-) diff --git a/src/sed/sedinstancetask.cpp b/src/sed/sedinstancetask.cpp index 35606d999..97f284661 100644 --- a/src/sed/sedinstancetask.cpp +++ b/src/sed/sedinstancetask.cpp @@ -480,9 +480,7 @@ void SedInstanceTask::Impl::run(double pVoiStart, double pVoiEnd, double pVoiInt auto voiTarget {pVoiStart + static_cast(++voiCounter) * pVoiInterval}; - voiEndReached = pTrackResults ? - (voiCounter == lastVoiCounter) : - ((voiTarget >= pVoiEnd) || fuzzyCompare(voiTarget, pVoiEnd)); + voiEndReached = pTrackResults ? (voiCounter == lastVoiCounter) : (voiTarget >= pVoiEnd); voiTarget = voiEndReached ? pVoiEnd : std::min(voiTarget, pVoiEnd); // Update our model's state. diff --git a/tests/api/sed/instancetests.cpp b/tests/api/sed/instancetests.cpp index 93e061687..1fa222d85 100644 --- a/tests/api/sed/instancetests.cpp +++ b/tests/api/sed/instancetests.cpp @@ -767,6 +767,21 @@ TEST(InstanceSedTest, odeModelWithInvalidTimes) EXPECT_EQ(instance->errorCount(), 1); + simulation->setInitialTime(-std::numeric_limits::infinity()); + simulation->setOutputStartTime(0.0); + simulation->setOutputEndTime(FIFTY); + + instance = document->instantiate(); + + EXPECT_EQ(instance->errorCount(), 1); + + simulation->setInitialTime(0.0); + simulation->setOutputStartTime(std::numeric_limits::quiet_NaN()); + + instance = document->instantiate(); + + EXPECT_EQ(instance->errorCount(), 1); + // Valid times, but then made invalid after instantiation. simulation->setOutputStartTime(0.0); diff --git a/tests/bindings/javascript/sed.instance.test.js b/tests/bindings/javascript/sed.instance.test.js index e15982b9b..943dab88d 100644 --- a/tests/bindings/javascript/sed.instance.test.js +++ b/tests/bindings/javascript/sed.instance.test.js @@ -844,6 +844,21 @@ test.describe('Sed instance tests', () => { assert.strictEqual(instance.errorCount, 1); + simulation.initialTime = Number.NEGATIVE_INFINITY; + simulation.outputStartTime = 0.0; + simulation.outputEndTime = 50.0; + + instance = document.instantiate(); + + assert.strictEqual(instance.errorCount, 1); + + simulation.initialTime = 0.0; + simulation.outputStartTime = Number.NaN; + + instance = document.instantiate(); + + assert.strictEqual(instance.errorCount, 1); + // Valid times, but then made invalid after instantiation. simulation.outputStartTime = 0.0; diff --git a/tests/bindings/python/test_sed_instance.py b/tests/bindings/python/test_sed_instance.py index 5db5e711f..b8f6cf840 100644 --- a/tests/bindings/python/test_sed_instance.py +++ b/tests/bindings/python/test_sed_instance.py @@ -766,6 +766,21 @@ def test_ode_model_with_invalid_times(): assert instance.error_count == 1 + simulation.initial_time = -math.inf + simulation.output_start_time = 0.0 + simulation.output_end_time = 50.0 + + instance = document.instantiate() + + assert instance.error_count == 1 + + simulation.initial_time = 0.0 + simulation.output_start_time = math.nan + + instance = document.instantiate() + + assert instance.error_count == 1 + # Valid times, but then made invalid after instantiation. simulation.output_start_time = 0.0