diff --git a/VERSION.txt b/VERSION.txt index 0f76e0a2f..9eab32c20 100644 --- a/VERSION.txt +++ b/VERSION.txt @@ -1 +1 @@ -1.20261009.0 +1.20261009.1 diff --git a/src/sed/sedinstancetask.cpp b/src/sed/sedinstancetask.cpp index 34c55800d..434cd3ee0 100644 --- a/src/sed/sedinstancetask.cpp +++ b/src/sed/sedinstancetask.cpp @@ -526,13 +526,25 @@ 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 totalSteps {mDifferentialModel ? static_cast(sedUniformTimeCoursePimpl->mNumberOfSteps) : 1}; + 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); + // 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. + + if (numberOfSteps <= 0) { + addIssue(Issue::Type::ERROR, sedUniformTimeCoursePimpl->invalidNumberOfStepsError(mModel, numberOfSteps), "Simulation"); + + return 0.0; + } + // (Re)initialise our model. // Note: reinitialise our model because we initialised it when we created the instance task. @@ -544,7 +556,7 @@ double SedInstanceTask::Impl::run() // Run our simulation from the initial time to the output start time, without tracking our results, but only if // the output start time is after the initial time. - const auto voiInterval {(sedUniformTimeCoursePimpl->mOutputEndTime - sedUniformTimeCoursePimpl->mOutputStartTime) / sedUniformTimeCoursePimpl->mNumberOfSteps}; + const auto voiInterval {(sedUniformTimeCoursePimpl->mOutputEndTime - sedUniformTimeCoursePimpl->mOutputStartTime) / numberOfSteps}; if (!fuzzyCompare(sedUniformTimeCoursePimpl->mInitialTime, sedUniformTimeCoursePimpl->mOutputStartTime)) { run(sedUniformTimeCoursePimpl->mInitialTime, sedUniformTimeCoursePimpl->mOutputStartTime, voiInterval, false); @@ -555,17 +567,28 @@ 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). const auto resultsSize {totalSteps + 1}; - mResults.resultsSize = resultsSize; + mResults = {}; + + SedInstanceTaskResults results; + + 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); - mResults.voi.resize(resultsSize); - mResults.states.resize(mStateCount * resultsSize); - mResults.rates.resize(mStateCount * resultsSize); - mResults.constants.resize(mConstantCount * resultsSize); - mResults.computedConstants.resize(mComputedConstantCount * resultsSize); - mResults.algebraicVariables.resize(mAlgebraicVariableCount * resultsSize); + 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 b2423297f..96e87e4bb 100644 --- a/src/sed/sedsimulation.cpp +++ b/src/sed/sedsimulation.cpp @@ -33,6 +33,25 @@ 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,20 +96,7 @@ bool SedSimulation::Impl::isValid(const SedModelPtr &pModel, const SedUniformTim addError(error); } else if (pUniformTimeCourse->numberOfSteps() <= 0) { - const auto numberOfSteps {toString(pUniformTimeCourse->numberOfSteps())}; - 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."; - - addError(error); + addError(invalidNumberOfStepsError(pModel, pUniformTimeCourse->numberOfSteps())); } } diff --git a/src/sed/sedsimulation_p.h b/src/sed/sedsimulation_p.h index afdfe34fc..7e7509be7 100644 --- a/src/sed/sedsimulation_p.h +++ b/src/sed/sedsimulation_p.h @@ -30,6 +30,8 @@ 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/tests/api/sed/instancetests.cpp b/tests/api/sed/instancetests.cpp index 409d99fb4..62a2b7928 100644 --- a/tests/api/sed/instancetests.cpp +++ b/tests/api/sed/instancetests.cpp @@ -675,6 +675,33 @@ TEST(InstanceSedTest, odeModelWithInvalidNumberOfSteps) EXPECT_FALSE(instance->hasIssues()); EXPECT_GT(instance->run(), 0.0); EXPECT_FALSE(instance->hasIssues()); + + // Make sure that an invalid number of steps is reported when running an instance even if it was valid when the + // instance was created. + + const auto &instanceTask {instance->tasks()[0]}; + + simulation->setNumberOfSteps(0); + + EXPECT_EQ(instance->run(), 0.0); + EXPECT_EQ_ISSUES(instance, ZERO_STEPS_EXPECTED_ISSUES); + EXPECT_EQ(instanceTask->voi().size(), 1001); + + simulation->setNumberOfSteps(-100); // NOLINT + + EXPECT_TRUE(instance->startRun()); + EXPECT_EQ(instance->waitForRun(), 0.0); + EXPECT_EQ(instance->status(), libOpenCOR::SedInstance::Status::IDLE); + EXPECT_EQ_ISSUES(instance, NEGATIVE_STEPS_EXPECTED_ISSUES); + EXPECT_EQ(instanceTask->voi().size(), 1001); + + // Make sure that the instance can be run again once the number of steps has been fixed. + + simulation->setNumberOfSteps(500); // NOLINT + + EXPECT_GT(instance->run(), 0.0); + EXPECT_FALSE(instance->hasIssues()); + EXPECT_EQ(instanceTask->voi().size(), 501); } TEST(InstanceSedTest, nlaModel) diff --git a/tests/bindings/javascript/sed.instance.test.js b/tests/bindings/javascript/sed.instance.test.js index 5a0040e8e..37772dba3 100644 --- a/tests/bindings/javascript/sed.instance.test.js +++ b/tests/bindings/javascript/sed.instance.test.js @@ -705,12 +705,14 @@ test.describe('Sed instance tests', () => { let instance = document.instantiate(); - assertIssues(loc, instance, [ + const zeroStepsExpectedIssues = [ [ 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." ] - ]); + ]; + + assertIssues(loc, instance, zeroStepsExpectedIssues); simulation.numberOfSteps = -100; @@ -741,6 +743,33 @@ test.describe('Sed instance tests', () => { assert.strictEqual(instance.hasIssues, false); assert.ok(instance.run() > 0.0); assert.strictEqual(instance.hasIssues, false); + + // Make sure that an invalid number of steps is reported when running an instance even if it was valid when the + // instance was created. + + const instanceTask = instance.tasks[0]; + + simulation.numberOfSteps = 0; + + assert.strictEqual(instance.run(), 0.0); + assertIssues(loc, instance, zeroStepsExpectedIssues); + assert.strictEqual(instanceTask.voi.length, 1001); + + simulation.numberOfSteps = -100; + + assert.strictEqual(instance.startRun(), true); + assert.strictEqual(instance.waitForRun(), 0.0); + assert.strictEqual(instance.status, loc.SedInstance.Status.IDLE); + assertIssues(loc, instance, negativeStepsExpectedIssues); + assert.strictEqual(instanceTask.voi.length, 1001); + + // Make sure that the instance can be run again once the number of steps has been fixed. + + simulation.numberOfSteps = 500; + + assert.ok(instance.run() > 0.0); + assert.strictEqual(instance.hasIssues, false); + assert.strictEqual(instanceTask.voi.length, 501); }); test('NLA model', () => { diff --git a/tests/bindings/python/test_sed_instance.py b/tests/bindings/python/test_sed_instance.py index 3feedcaea..b9af1dfd3 100644 --- a/tests/bindings/python/test_sed_instance.py +++ b/tests/bindings/python/test_sed_instance.py @@ -667,6 +667,33 @@ def test_ode_model_with_invalid_number_of_steps(): assert instance.run() > 0.0 assert not instance.has_issues + # Make sure that an invalid number of steps is reported when running an instance even if it was valid when the + # instance was created. + + instance_task = instance.tasks[0] + + simulation.number_of_steps = 0 + + assert instance.run() == 0.0 + assert_issues(instance, zero_steps_expected_issues) + assert len(instance_task.voi) == 1001 + + simulation.number_of_steps = -100 + + assert instance.start_run() is True + assert instance.wait_for_run() == 0.0 + assert instance.status == loc.SedInstance.Status.Idle + assert_issues(instance, negative_steps_expected_issues) + assert len(instance_task.voi) == 1001 + + # Make sure that the instance can be run again once the number of steps has been fixed. + + simulation.number_of_steps = 500 + + assert instance.run() > 0.0 + assert not instance.has_issues + assert len(instance_task.voi) == 501 + def test_nla_model(): expected_issues = [