Merge pull request #2485 from dweindl/release_0.26.1

Release 0.26.1
AMICI-dev · Jul 11, 2024 · b0d79ff · b0d79ff
2 parents b958cce + 86b1c59
commit b0d79ff
Show file tree

Hide file tree

Showing 18 changed files with 186 additions and 73 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,7 +4,28 @@ See also our [versioning policy](https://amici.readthedocs.io/en/latest/versioni
 
 ## v0.X Series
 
-### v0.26.0 (2024-07-06)
+### v0.26.1 (2024-07-11)
+
+**Fixes**
+
+* Fixed some C++ exception handling that previously could crash Python under
+  certain conditions
+
+  by @dweindl in https://github.com/AMICI-dev/AMICI/pull/2484
+
+* Disabled turning warnings into errors when building amici on GitHub Actions
+  in downstream projects
+
+  by @dweindl in https://github.com/AMICI-dev/AMICI/pull/2481
+
+* Fixed CMP0167 warning with CMake 3.30
+
+  by @dweindl in https://github.com/AMICI-dev/AMICI/pull/2480
+
+
+**Full Changelog**: https://github.com/AMICI-dev/AMICI/compare/v0.26.0...v0.26.1
+
+### v0.26.0 (2024-07-02)
 
 AMICI v0.26.0 requires sympy>=1.12.1 and petab>=0.4.0.
 

diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -8,6 +8,10 @@ cmake_policy(VERSION 3.15...3.27)
 if(POLICY CMP0144)
   cmake_policy(SET CMP0144 NEW)
 endif(POLICY CMP0144)
+# cmake >= 3.30
+if(POLICY CMP0167)
+  cmake_policy(SET CMP0167 NEW)
+endif(POLICY CMP0167)
 
 project(amici)
 
@@ -32,10 +36,11 @@ message(STATUS "CMAKE_INSTALL_PREFIX: ${CMAKE_INSTALL_PREFIX}")
 
 list(APPEND CMAKE_MODULE_PATH ${CMAKE_CURRENT_SOURCE_DIR}/cmake)
 
-if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
-  # require at least gcc 4.9, otherwise regex wont work properly
-  if(CMAKE_CXX_COMPILER_VERSION VERSION_LESS 4.9)
-    message(FATAL_ERROR "GCC version must be at least 4.9!")
+if("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU" OR "${CMAKE_CXX_COMPILER_ID}"
+                                                STREQUAL "MINGW")
+  # require at least gcc 9.1 for proper C+17 support, e.g. std::reduce
+  if(CMAKE_CXX_COMPILER_VERSION VERSION_LESS 9.1)
+    message(FATAL_ERROR "GCC version must be at least 9.1!")
   endif()
 endif()
 
@@ -143,8 +148,7 @@ else()
   set(VENDORED_SUNDIALS_INSTALL_DIR ${VENDORED_SUNDIALS_BUILD_DIR})
 endif()
 set(SUNDIALS_ROOT "${VENDORED_SUNDIALS_INSTALL_DIR}/${CMAKE_INSTALL_LIBDIR}")
-find_package(
-  SUNDIALS REQUIRED CONFIG PATHS "${SUNDIALS_ROOT}/cmake/sundials/")
+find_package(SUNDIALS REQUIRED CONFIG PATHS "${SUNDIALS_ROOT}/cmake/sundials/")
 message(STATUS "Found SUNDIALS: ${SUNDIALS_DIR}")
 
 set(GSL_LITE_INCLUDE_DIR "${CMAKE_CURRENT_SOURCE_DIR}/ThirdParty/gsl")

diff --git a/include/amici/model_dae.h b/include/amici/model_dae.h
@@ -53,10 +53,10 @@ class Model_DAE : public Model {
         std::map<realtype, std::vector<int>> state_independent_events = {}
     )
         : Model(
-            model_dimensions, simulation_parameters, o2mode, idlist, z2event,
-            pythonGenerated, ndxdotdp_explicit, ndxdotdx_explicit,
-            w_recursion_depth, state_independent_events
-        ) {
+              model_dimensions, simulation_parameters, o2mode, idlist, z2event,
+              pythonGenerated, ndxdotdp_explicit, ndxdotdx_explicit,
+              w_recursion_depth, state_independent_events
+          ) {
         derived_state_.M_ = SUNMatrixWrapper(nx_solver, nx_solver);
         auto M_nnz = static_cast<sunindextype>(
             std::reduce(idlist.begin(), idlist.end())

diff --git a/include/amici/model_ode.h b/include/amici/model_ode.h
@@ -52,10 +52,10 @@ class Model_ODE : public Model {
         std::map<realtype, std::vector<int>> state_independent_events = {}
     )
         : Model(
-            model_dimensions, simulation_parameters, o2mode, idlist, z2event,
-            pythonGenerated, ndxdotdp_explicit, ndxdotdx_explicit,
-            w_recursion_depth, state_independent_events
-        ) {}
+              model_dimensions, simulation_parameters, o2mode, idlist, z2event,
+              pythonGenerated, ndxdotdp_explicit, ndxdotdx_explicit,
+              w_recursion_depth, state_independent_events
+          ) {}
 
     void
     fJ(realtype t, realtype cj, AmiVector const& x, AmiVector const& dx,

diff --git a/python/sdist/setup.py b/python/sdist/setup.py
@@ -152,7 +152,9 @@ def get_extensions():
         cmake_configure_options=[
             *global_cmake_configure_options,
             "-Werror=dev"
-            if "GITHUB_ACTIONS" in os.environ
+            # Turn warnings in to errors on GitHub Actions,
+            #  match original repo and forks with default name
+            if os.environ.get("GITHUB_REPOSITORY", "").endswith("/AMICI")
             else "-Wno-error=dev",
             "-DAMICI_PYTHON_BUILD_EXT_ONLY=ON",
             f"-DPython3_EXECUTABLE={Path(sys.executable).as_posix()}",

diff --git a/python/tests/test_swig_interface.py b/python/tests/test_swig_interface.py
@@ -5,7 +5,7 @@
 
 import copy
 import numbers
-
+from math import nan
 import pytest
 
 import amici
@@ -534,3 +534,55 @@ def test_rdataview(sbml_example_presimulation_module):
 
     # field names are included by dir()
     assert "x" in dir(rdata)
+
+
+def test_python_exceptions(sbml_example_presimulation_module):
+    """Test that C++ exceptions are correctly caught and re-raised in Python."""
+
+    # amici-base extension throws and its swig-wrapper catches
+    solver = amici.CVodeSolver()
+    with pytest.raises(
+        RuntimeError, match="maxsteps must be a positive number"
+    ):
+        solver.setMaxSteps(-1)
+
+    # model extension throws and its swig-wrapper catches
+    model = sbml_example_presimulation_module.get_model()
+    with pytest.raises(RuntimeError, match="Steadystate mask has wrong size"):
+        model.set_steadystate_mask([1] * model.nx_solver * 2)
+
+    # amici-base extension throws and its swig-wrapper catches
+    edata = amici.ExpData(1, 1, 1, [1])
+    # too short sx0
+    edata.sx0 = (1, 2)
+    with pytest.raises(
+        RuntimeError,
+        match=r"Number of initial conditions sensitivities \(36\) "
+        r"in model does not match ExpData \(2\).",
+    ):
+        amici.runAmiciSimulation(model, solver, edata)
+
+    amici.runAmiciSimulations(
+        model, solver, [edata, edata], failfast=True, num_threads=1
+    )
+
+    # model throws, base catches, swig-exception handling is not involved
+    model.setParameters([nan] * model.np())
+    model.setTimepoints([1])
+    rdata = amici.runAmiciSimulation(model, solver)
+    assert rdata.status == amici.AMICI_FIRST_RHSFUNC_ERR
+
+    edata = amici.ExpData(1, 1, 1, [1])
+    rdatas = amici.runAmiciSimulations(
+        model, solver, [edata, edata], failfast=True, num_threads=1
+    )
+    assert rdatas[0].status == amici.AMICI_FIRST_RHSFUNC_ERR
+
+    # model throws, base catches, swig-exception handling is involved
+    from amici._amici import runAmiciSimulation
+
+    with pytest.raises(
+        RuntimeError, match="AMICI failed to integrate the forward problem"
+    ):
+        # rethrow=True
+        runAmiciSimulation(solver, None, model.get(), True)
diff --git a/scripts/buildAmici.sh b/scripts/buildAmici.sh
@@ -13,7 +13,7 @@ amici_build_dir="${amici_path}/build"
 mkdir -p "${amici_build_dir}"
 cd "${amici_build_dir}"
 
-if [ "${GITHUB_ACTIONS:-}" = true ] ||
+if [[ "${GITHUB_REPOSITORY:-}" = *"/AMICI" ]] ||
   [ "${ENABLE_AMICI_DEBUGGING:-}" = TRUE ] ||
   [ "${ENABLE_GCOV_COVERAGE:-}" = TRUE ]; then
   # Running on CI server

diff --git a/src/amici.cpp b/src/amici.cpp
@@ -282,17 +282,28 @@ std::vector<std::unique_ptr<ReturnData>> runAmiciSimulations(
 #pragma omp parallel for num_threads(num_threads)
 #endif
     for (int i = 0; i < (int)edatas.size(); ++i) {
-        auto mySolver = std::unique_ptr<Solver>(solver.clone());
-        auto myModel = std::unique_ptr<Model>(model.clone());
-
-        /* if we fail we need to write empty return datas for the python
-         interface */
-        if (skipThrough) {
-            ConditionContext conditionContext(myModel.get(), edatas[i]);
+        // must catch exceptions in parallel section to avoid termination
+        try {
+            auto mySolver = std::unique_ptr<Solver>(solver.clone());
+            auto myModel = std::unique_ptr<Model>(model.clone());
+
+            /* if we fail we need to write empty return datas for the python
+             interface */
+            if (skipThrough) {
+                ConditionContext conditionContext(myModel.get(), edatas[i]);
+                results[i]
+                    = std::unique_ptr<ReturnData>(new ReturnData(solver, model)
+                    );
+            } else {
+                results[i] = runAmiciSimulation(*mySolver, edatas[i], *myModel);
+            }
+        } catch (std::exception const& ex) {
             results[i]
                 = std::unique_ptr<ReturnData>(new ReturnData(solver, model));
-        } else {
-            results[i] = runAmiciSimulation(*mySolver, edatas[i], *myModel);
+            results[i]->status = AMICI_ERROR;
+            results[i]->messages.push_back(
+                LogItem(LogSeverity::error, "OTHER", ex.what())
+            );
         }
 
         skipThrough |= failfast && results[i]->status < 0;

diff --git a/src/edata.cpp b/src/edata.cpp
@@ -59,9 +59,9 @@ ExpData::ExpData(
 
 ExpData::ExpData(Model const& model)
     : ExpData(
-        model.nytrue, model.nztrue, model.nMaxEvent(), model.getTimepoints(),
-        model.getFixedParameters()
-    ) {
+          model.nytrue, model.nztrue, model.nMaxEvent(), model.getTimepoints(),
+          model.getFixedParameters()
+      ) {
     reinitializeFixedParameterInitialStates
         = model.getReinitializeFixedParameterInitialStates()
           && model.getReinitializationStateIdxs().empty();
@@ -70,9 +70,9 @@ ExpData::ExpData(Model const& model)
 
 ExpData::ExpData(ReturnData const& rdata, realtype sigma_y, realtype sigma_z)
     : ExpData(
-        rdata, std::vector<realtype>(rdata.nytrue * rdata.nt, sigma_y),
-        std::vector<realtype>(rdata.nztrue * rdata.nmaxevent, sigma_z)
-    ) {}
+          rdata, std::vector<realtype>(rdata.nytrue * rdata.nt, sigma_y),
+          std::vector<realtype>(rdata.nztrue * rdata.nmaxevent, sigma_z)
+      ) {}
 
 ExpData::ExpData(
     ReturnData const& rdata, std::vector<realtype> sigma_y,

diff --git a/src/exception.cpp b/src/exception.cpp
@@ -37,17 +37,17 @@ CvodeException::CvodeException(
     int const error_code, char const* function, char const* extra
 )
     : AmiException(
-        "CVODE routine %s failed with error code %i. %s", function, error_code,
-        extra ? extra : ""
-    ) {}
+          "CVODE routine %s failed with error code %i. %s", function,
+          error_code, extra ? extra : ""
+      ) {}
 
 IDAException::IDAException(
     int const error_code, char const* function, char const* extra
 )
     : AmiException(
-        "IDA routine %s failed with error code %i. %s", function, error_code,
-        extra ? extra : ""
-    ) {}
+          "IDA routine %s failed with error code %i. %s", function, error_code,
+          extra ? extra : ""
+      ) {}
 
 IntegrationFailure::IntegrationFailure(int code, realtype t)
     : AmiException("AMICI failed to integrate the forward problem")
@@ -61,8 +61,8 @@ IntegrationFailureB::IntegrationFailureB(int code, realtype t)
 
 NewtonFailure::NewtonFailure(int code, char const* function)
     : AmiException(
-        "NewtonSolver routine %s failed with error code %i", function, code
-    ) {
+          "NewtonSolver routine %s failed with error code %i", function, code
+      ) {
     error_code = code;
 }
 

diff --git a/src/interface_matlab.cpp b/src/interface_matlab.cpp
@@ -423,8 +423,10 @@ void setModelData(mxArray const* prhs[], int nrhs, Model& model) {
                 model.setParameterScale(
                     static_cast<ParameterScaling>(dbl2int(mxGetScalar(a)))
                 );
-            } else if((mxGetM(a) == 1 && gsl::narrow<int>(mxGetN(a)) == model.np())
-                       || (mxGetN(a) == 1 && gsl::narrow<int>(mxGetM(a)) == model.np())) {
+            } else if ((mxGetM(a) == 1
+                        && gsl::narrow<int>(mxGetN(a)) == model.np())
+                       || (mxGetN(a) == 1
+                           && gsl::narrow<int>(mxGetM(a)) == model.np())) {
                 auto pscaleArray = static_cast<double*>(mxGetData(a));
                 std::vector<ParameterScaling> pscale(model.np());
                 for (int ip = 0; ip < model.np(); ++ip) {
@@ -597,7 +599,9 @@ void mexFunction(int nlhs, mxArray* plhs[], int nrhs, mxArray const* prhs[]) {
                 ex.what()
             );
         }
-    } else if (solver->getSensitivityOrder() >= amici::SensitivityOrder::first && solver->getSensitivityMethod() == amici::SensitivityMethod::adjoint) {
+    } else if (solver->getSensitivityOrder() >= amici::SensitivityOrder::first
+               && solver->getSensitivityMethod()
+                      == amici::SensitivityMethod::adjoint) {
         mexErrMsgIdAndTxt("AMICI:mex:setup", "No data provided!");
     }
 

diff --git a/src/newton_solver.cpp b/src/newton_solver.cpp
@@ -145,7 +145,7 @@ void NewtonSolverDense::solveLinearSystem(AmiVector& rhs) {
         throw NewtonFailure(status, "SUNLinSolSolve_Dense");
 }
 
-void NewtonSolverDense::reinitialize(){
+void NewtonSolverDense::reinitialize() {
     /* dense solver does not need reinitialization */
 };
 

diff --git a/src/rdata.cpp b/src/rdata.cpp
@@ -15,14 +15,14 @@ namespace amici {
 
 ReturnData::ReturnData(Solver const& solver, Model const& model)
     : ReturnData(
-        model.getTimepoints(),
-        ModelDimensions(static_cast<ModelDimensions const&>(model)),
-        model.nplist(), model.nMaxEvent(), model.nt(),
-        solver.getNewtonMaxSteps(), model.getParameterScale(), model.o2mode,
-        solver.getSensitivityOrder(), solver.getSensitivityMethod(),
-        solver.getReturnDataReportingMode(), model.hasQuadraticLLH(),
-        model.getAddSigmaResiduals(), model.getMinimumSigmaResiduals()
-    ) {}
+          model.getTimepoints(),
+          ModelDimensions(static_cast<ModelDimensions const&>(model)),
+          model.nplist(), model.nMaxEvent(), model.nt(),
+          solver.getNewtonMaxSteps(), model.getParameterScale(), model.o2mode,
+          solver.getSensitivityOrder(), solver.getSensitivityMethod(),
+          solver.getReturnDataReportingMode(), model.hasQuadraticLLH(),
+          model.getAddSigmaResiduals(), model.getMinimumSigmaResiduals()
+      ) {}
 
 ReturnData::ReturnData(
     std::vector<realtype> ts, ModelDimensions const& model_dimensions,

diff --git a/src/spline.cpp b/src/spline.cpp
@@ -217,7 +217,7 @@ double seval(
                     j = k; /* move the upper bound */
                 if (u >= x[k])
                     i = k; /* move the lower bound */
-            }              /* there are no more segments to search */
+            } /* there are no more segments to search */
             while (j > i + 1);
         }
     }
@@ -273,7 +273,7 @@ double sinteg(
                 j = k; /* move the upper bound */
             if (u >= x[k])
                 i = k; /* move the lower bound */
-        }              /* there are no more segments to search */
+        } /* there are no more segments to search */
         while (j > i + 1);
     }