diff --git a/VERSION.txt b/VERSION.txt index 5fe4d4334..0f76e0a2f 100644 --- a/VERSION.txt +++ b/VERSION.txt @@ -1 +1 @@ -1.20261008.1 +1.20261009.0 diff --git a/src/sed/sedinstance.cpp b/src/sed/sedinstance.cpp index 689ef5ff5..654d99573 100644 --- a/src/sed/sedinstance.cpp +++ b/src/sed/sedinstance.cpp @@ -21,6 +21,7 @@ limitations under the License. #include "libopencor/seddocument.h" #include +#include #include namespace libOpenCOR { @@ -140,22 +141,38 @@ double SedInstance::Impl::run() } // Run all the tasks associated with this instance unless they have some issues. + // Note: a task may throw an exception (e.g., std::bad_alloc if its results cannot be allocated), in which case we + // report it as an error rather than let it escape. Indeed, run() may be called from startRun(), i.e. on a + // separate thread, and an escaping exception would leave us in a state from which we cannot recover. Also, + // we have no way to trigger such an exception in our tests, hence we ignore our try...catch statement during + // code coverage. auto res {0.0}; - for (const auto &task : mTasks) { - if (!task->hasIssues()) { - res += task->pimpl()->run(); +#ifndef CODE_COVERAGE_ENABLED + try { +#endif + for (const auto &task : mTasks) { + if (!task->hasIssues()) { + res += task->pimpl()->run(); - if (task->hasIssues()) { - addIssues(task, "Task"); + if (task->hasIssues()) { + addIssues(task, "Task"); - // Reset the issues of the task so that they are not reported again should the instance be run again. + // Reset the issues of the task so that they are not reported again should the instance be run + // again. - task->pimpl()->removeAllIssues(); + task->pimpl()->removeAllIssues(); + } } } +#ifndef CODE_COVERAGE_ENABLED + } catch (const std::exception &exception) { + addError(std::string("The simulation failed: ") + exception.what() + "."); + } catch (...) { + addError("The simulation failed."); } +#endif // Reset and make sure that our control flags are no longer passed to each task. @@ -275,7 +292,20 @@ SedInstance::SedInstance(const SedDocumentPtr &pDocument) SedInstance::~SedInstance() { - pimpl()->waitForRun(); // To ensure that the instance is not running before we delete it. + // Make sure that the instance is not running before we delete it. + // Note: run() reports a failure as an issue rather than throw an exception, so waitForRun() should never throw, but + // an exception must never escape a destructor (it would result in std::terminate() being called), hence we + // make sure of it. + +#ifndef CODE_COVERAGE_ENABLED + try { +#endif + pimpl()->waitForRun(); +#ifndef CODE_COVERAGE_ENABLED + } catch (...) { // NOLINT(bugprone-empty-catch) + // There is nothing more that we can do since we are being deleted. + } +#endif } SedInstance::Impl *SedInstance::pimpl() diff --git a/src/sed/sedmodel.cpp b/src/sed/sedmodel.cpp index fd9c43ea2..78491862b 100644 --- a/src/sed/sedmodel.cpp +++ b/src/sed/sedmodel.cpp @@ -41,6 +41,12 @@ const FilePtr &SedModel::Impl::file() const bool SedModel::Impl::isValid() { + // Reset our issues so that they are not reported again should we be validated again. + + removeAllIssues(); + + // Make sure that the CellML file is valid. + switch (mFile->pimpl()->mCellmlFile->type()) { case libcellml::AnalyserModel::Type::INVALID: addError("The CellML file is invalid."); diff --git a/src/sed/sedsimulation.cpp b/src/sed/sedsimulation.cpp index 99b95a2d3..b2423297f 100644 --- a/src/sed/sedsimulation.cpp +++ b/src/sed/sedsimulation.cpp @@ -18,9 +18,12 @@ limitations under the License. #include "seddocument_p.h" #include "sedmodel_p.h" #include "sedsimulation_p.h" +#include "seduniformtimecourse_p.h" #include "solvernla_p.h" #include "solverode_p.h" +#include "utils.h" + namespace libOpenCOR { static constexpr auto ID_PREFIX {"simulation"}; @@ -30,7 +33,7 @@ SedSimulation::Impl::Impl(const SedDocumentPtr &pDocument) { } -bool SedSimulation::Impl::isValid(const SedModelPtr &pModel, bool pUniformTimeCourse) +bool SedSimulation::Impl::isValid(const SedModelPtr &pModel, const SedUniformTimeCoursePtr &pUniformTimeCourse) { auto modelType {pModel->pimpl()->mFile->pimpl()->mCellmlFile->type()}; const auto &modelId = pModel->pimpl()->mId; @@ -51,23 +54,44 @@ bool SedSimulation::Impl::isValid(const SedModelPtr &pModel, bool pUniformTimeCo addError(error); }; - // Make sure that we are a uniform time course simulation if our model is an ODE/DAE model. + // Reset our issues so that they are not reported again should we be validated again. + + 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. //---GRY--- WE DON'T CURRENTLY SUPPORT OTHER TYPES OF SIMULATION FOR ODE/DAE MODELS. - if (((modelType == libcellml::AnalyserModel::Type::ODE) - || (modelType == libcellml::AnalyserModel::Type::DAE)) - && !pUniformTimeCourse) { - std::string error; + if ((modelType == libcellml::AnalyserModel::Type::ODE) + || (modelType == libcellml::AnalyserModel::Type::DAE)) { + if (pUniformTimeCourse == nullptr) { + std::string error; - error.reserve(mId.size() + modelId.size() + 96); // NOLINT + error.reserve(mId.size() + modelId.size() + 96); // NOLINT - error += "Simulation '"; - error += mId; - error += "' is to be used with model '"; - error += modelId; - error += "' which (currently) requires a uniform time course simulation."; + error += "Simulation '"; + error += mId; + error += "' is to be used with model '"; + error += modelId; + error += "' which (currently) requires a uniform time course simulation."; - addError(error); + 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); + } } // Make sure that we have the solver(s) required by our model. diff --git a/src/sed/sedsimulation_p.h b/src/sed/sedsimulation_p.h index 9ed4786c5..afdfe34fc 100644 --- a/src/sed/sedsimulation_p.h +++ b/src/sed/sedsimulation_p.h @@ -30,7 +30,7 @@ class SedSimulation::Impl: public SedBase::Impl explicit Impl(const SedDocumentPtr &pDocument); - bool isValid(const SedModelPtr &pModel, bool pUniformTimeCourse); + bool isValid(const SedModelPtr &pModel, const SedUniformTimeCoursePtr &pUniformTimeCourse); const SolverOdePtr &odeSolver() const; void setOdeSolver(const SolverOdePtr &pOdeSolver); diff --git a/src/sed/sedtask.cpp b/src/sed/sedtask.cpp index d9ed47b66..76e05aaa1 100644 --- a/src/sed/sedtask.cpp +++ b/src/sed/sedtask.cpp @@ -79,7 +79,7 @@ bool SedTask::Impl::isValid() // Make sure that the simulation is valid for the model. - if (!mSimulation->pimpl()->isValid(mModel, std::dynamic_pointer_cast(mSimulation) != nullptr)) { + if (!mSimulation->pimpl()->isValid(mModel, std::dynamic_pointer_cast(mSimulation))) { addIssues(mSimulation, "Simulation"); } diff --git a/tests/api/sed/instancetests.cpp b/tests/api/sed/instancetests.cpp index 94cd95d6e..409d99fb4 100644 --- a/tests/api/sed/instancetests.cpp +++ b/tests/api/sed/instancetests.cpp @@ -634,6 +634,49 @@ TEST(InstanceSedTest, odeModelWithNonUniformTimeCourseSimulation) EXPECT_EQ_ISSUES(instance, STEADY_STATE_EXPECTED_ISSUES); } +TEST(InstanceSedTest, odeModelWithInvalidNumberOfSteps) +{ + static const libOpenCOR::ExpectedIssues ZERO_STEPS_EXPECTED_ISSUES {{ + {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 libOpenCOR::ExpectedIssues NEGATIVE_STEPS_EXPECTED_ISSUES {{ + {libOpenCOR::Issue::Type::ERROR, "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires a strictly positive number of steps but -100 is provided."}, + }}; + + 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])}; + + simulation->setNumberOfSteps(0); + + auto instance {document->instantiate()}; + + EXPECT_EQ_ISSUES(instance, ZERO_STEPS_EXPECTED_ISSUES); + + simulation->setNumberOfSteps(-100); // NOLINT + + instance = document->instantiate(); + + EXPECT_EQ_ISSUES(instance, NEGATIVE_STEPS_EXPECTED_ISSUES); + + // Running the instance should not do anything, but it should still leave it idle. + + 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); + + // Make sure that the number of steps is not reported as invalid anymore once it has been fixed. + + simulation->setNumberOfSteps(1000); // NOLINT + + instance = document->instantiate(); + + EXPECT_FALSE(instance->hasIssues()); + EXPECT_GT(instance->run(), 0.0); + EXPECT_FALSE(instance->hasIssues()); +} + 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 37e9c76a9..5a0040e8e 100644 --- a/tests/bindings/javascript/sed.instance.test.js +++ b/tests/bindings/javascript/sed.instance.test.js @@ -693,6 +693,56 @@ test.describe('Sed instance tests', () => { ]); }); + test('ODE model with invalid 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 simulation = document.simulations.get(0); + + simulation.numberOfSteps = 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 a strictly positive number of steps but 0 is provided." + ] + ]); + + simulation.numberOfSteps = -100; + + instance = document.instantiate(); + + const negativeStepsExpectedIssues = [ + [ + loc.Issue.Type.ERROR, + "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires a strictly positive number of steps but -100 is provided." + ] + ]; + + assertIssues(loc, instance, negativeStepsExpectedIssues); + + // Running the instance should not do anything, but it should still leave it idle. + + assert.strictEqual(instance.startRun(), true); + assert.strictEqual(instance.waitForRun(), 0.0); + assert.strictEqual(instance.status, loc.SedInstance.Status.IDLE); + assertIssues(loc, instance, negativeStepsExpectedIssues); + + // Make sure that the number of steps is not reported as invalid anymore once it has been fixed. + + simulation.numberOfSteps = 1000; + + instance = document.instantiate(); + + assert.strictEqual(instance.hasIssues, false); + assert.ok(instance.run() > 0.0); + assert.strictEqual(instance.hasIssues, false); + }); + 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 286cd5790..3feedcaea 100644 --- a/tests/bindings/python/test_sed_instance.py +++ b/tests/bindings/python/test_sed_instance.py @@ -620,6 +620,54 @@ def test_ode_model_with_non_uniform_time_course_simulation(): assert_issues(instance, steady_state_expected_issues) +def test_ode_model_with_invalid_number_of_steps(): + zero_steps_expected_issues = [ + [ + 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.", + ], + ] + negative_steps_expected_issues = [ + [ + loc.Issue.Type.Error, + "Task | Simulation: simulation 'simulation1' is to be used with model 'model1' which requires a strictly positive number of steps but -100 is provided.", + ], + ] + + file = loc.File(utils.resource_path("cellml_2.cellml")) + document = loc.SedDocument(file) + simulation = document.simulations[0] + + simulation.number_of_steps = 0 + + instance = document.instantiate() + + assert_issues(instance, zero_steps_expected_issues) + + simulation.number_of_steps = -100 + + instance = document.instantiate() + + assert_issues(instance, negative_steps_expected_issues) + + # Running the instance should not do anything, but it should still leave it idle. + + 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) + + # Make sure that the number of steps is not reported as invalid anymore once it has been fixed. + + simulation.number_of_steps = 1000 + + instance = document.instantiate() + + assert not instance.has_issues + assert instance.run() > 0.0 + assert not instance.has_issues + + def test_nla_model(): expected_issues = [ [