diff --git a/CHANGELOG.md b/CHANGELOG.md index e60a72eb4d..a9f1cde585 100644 --- 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 index 9b0374b3a9..2dc0ada467 100644 --- 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 index 232bfc867d..03ffbf8fb9 100644 --- a/include/amici/model_dae.h +++ b/include/amici/model_dae.h @@ -53,10 +53,10 @@ class Model_DAE : public Model { std::map> 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( std::reduce(idlist.begin(), idlist.end()) diff --git a/include/amici/model_ode.h b/include/amici/model_ode.h index 3632c2198f..24d410a33c 100644 --- a/include/amici/model_ode.h +++ b/include/amici/model_ode.h @@ -52,10 +52,10 @@ class Model_ODE : public Model { std::map> 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 index 9dd268953a..d01a0ec75c 100755 --- 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 index 2dfb46e0a7..f61b28ea4d 100644 --- 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 index cb2428579c..22faece983 100755 --- 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 index c74c88e3a5..9fbc472774 100644 --- a/src/amici.cpp +++ b/src/amici.cpp @@ -282,17 +282,28 @@ std::vector> 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.clone()); - auto myModel = std::unique_ptr(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.clone()); + auto myModel = std::unique_ptr(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(new ReturnData(solver, model) + ); + } else { + results[i] = runAmiciSimulation(*mySolver, edatas[i], *myModel); + } + } catch (std::exception const& ex) { results[i] = std::unique_ptr(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 index 2408e4becd..a4c0868ad8 100644 --- 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(rdata.nytrue * rdata.nt, sigma_y), - std::vector(rdata.nztrue * rdata.nmaxevent, sigma_z) - ) {} + rdata, std::vector(rdata.nytrue * rdata.nt, sigma_y), + std::vector(rdata.nztrue * rdata.nmaxevent, sigma_z) + ) {} ExpData::ExpData( ReturnData const& rdata, std::vector sigma_y, diff --git a/src/exception.cpp b/src/exception.cpp index 81ec938d29..984be52821 100644 --- 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 index e0f4702f34..93ffb24fdd 100644 --- 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(dbl2int(mxGetScalar(a))) ); - } else if((mxGetM(a) == 1 && gsl::narrow(mxGetN(a)) == model.np()) - || (mxGetN(a) == 1 && gsl::narrow(mxGetM(a)) == model.np())) { + } else if ((mxGetM(a) == 1 + && gsl::narrow(mxGetN(a)) == model.np()) + || (mxGetN(a) == 1 + && gsl::narrow(mxGetM(a)) == model.np())) { auto pscaleArray = static_cast(mxGetData(a)); std::vector 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 index c14b05c7f2..72bf0a3d64 100644 --- 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 index d7b99f3c8a..649b420ccb 100644 --- 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(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(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 ts, ModelDimensions const& model_dimensions, diff --git a/src/spline.cpp b/src/spline.cpp index 4e7d742520..719af0a86e 100644 --- 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); } diff --git a/src/splinefunctions.cpp b/src/splinefunctions.cpp index f5d2599ea7..0b07a75aa9 100644 --- a/src/splinefunctions.cpp +++ b/src/splinefunctions.cpp @@ -145,9 +145,9 @@ HermiteSpline::HermiteSpline( bool logarithmic_parametrization ) : AbstractSpline( - std::move(nodes), std::move(node_values), equidistant_spacing, - logarithmic_parametrization - ) + std::move(nodes), std::move(node_values), equidistant_spacing, + logarithmic_parametrization + ) , node_values_derivative_(std::move(node_values_derivative)) , first_node_bc_(firstNodeBC) , last_node_bc_(lastNodeBC) @@ -602,21 +602,25 @@ void HermiteSpline::compute_coefficients_extrapolation_sensi( case SplineExtrapolation::linear: if (first_node_bc_ == SplineBoundaryCondition::zeroDerivative) { sm0 = 0; - } else if (get_node_derivative_by_fd() && first_node_bc_ == SplineBoundaryCondition::given) { + } else if (get_node_derivative_by_fd() + && first_node_bc_ == SplineBoundaryCondition::given) { sm0 = (dvaluesdp[spline_offset + ip + nplist] - sp0) / (nodes_[1] - nodes_[0]); - } else if (get_node_derivative_by_fd() && first_node_bc_ == SplineBoundaryCondition::natural) { + } else if (get_node_derivative_by_fd() + && first_node_bc_ == SplineBoundaryCondition::natural) { throw AmiException( "Natural boundary condition for " "Hermite splines with linear extrapolation is " "not yet implemented." ); - } else if (!get_node_derivative_by_fd() && first_node_bc_ == SplineBoundaryCondition::given) { + } else if (!get_node_derivative_by_fd() + && first_node_bc_ == SplineBoundaryCondition::given) { sm0 = dslopesdp[spline_offset + ip]; - } else if (!get_node_derivative_by_fd() && first_node_bc_ == SplineBoundaryCondition::natural) { + } else if (!get_node_derivative_by_fd() + && first_node_bc_ == SplineBoundaryCondition::natural) { throw AmiException( "Natural boundary condition for " "Hermite splines with linear extrapolation is " @@ -662,24 +666,28 @@ void HermiteSpline::compute_coefficients_extrapolation_sensi( case SplineExtrapolation::linear: if (last_node_bc_ == SplineBoundaryCondition::zeroDerivative) { sm_end = 0; - } else if (get_node_derivative_by_fd() && last_node_bc_ == SplineBoundaryCondition::given) { + } else if (get_node_derivative_by_fd() + && last_node_bc_ == SplineBoundaryCondition::given) { sm_end = (sp_end - dvaluesdp [spline_offset + ip + (n_nodes() - 2) * nplist]) / (nodes_[n_nodes() - 1] - nodes_[n_nodes() - 2]); - } else if (get_node_derivative_by_fd() && last_node_bc_ == SplineBoundaryCondition::natural) { + } else if (get_node_derivative_by_fd() + && last_node_bc_ == SplineBoundaryCondition::natural) { throw AmiException( "Natural boundary condition for " "Hermite splines with linear extrapolation is " "not yet implemented." ); - } else if (!get_node_derivative_by_fd() && last_node_bc_ == SplineBoundaryCondition::given) { + } else if (!get_node_derivative_by_fd() + && last_node_bc_ == SplineBoundaryCondition::given) { sm_end = dslopesdp[spline_offset + ip + (n_nodes() - 1) * nplist]; - } else if (!get_node_derivative_by_fd() && last_node_bc_ == SplineBoundaryCondition::natural) { + } else if (!get_node_derivative_by_fd() + && last_node_bc_ == SplineBoundaryCondition::natural) { throw AmiException( "Natural boundary condition for " "Hermite splines with linear extrapolation is " diff --git a/src/sundials_linsol_wrapper.cpp b/src/sundials_linsol_wrapper.cpp index 5752ea03c3..6836949cb8 100644 --- a/src/sundials_linsol_wrapper.cpp +++ b/src/sundials_linsol_wrapper.cpp @@ -277,8 +277,8 @@ N_Vector SUNLinSolSPBCGS::getResid() const { SUNLinSolSPFGMR::SUNLinSolSPFGMR(AmiVector const& x, int pretype, int maxl) : SUNLinSolWrapper( - SUNLinSol_SPFGMR(const_cast(x.getNVector()), pretype, maxl) - ) { + SUNLinSol_SPFGMR(const_cast(x.getNVector()), pretype, maxl) + ) { if (!solver_) throw AmiException("Failed to create solver."); } @@ -311,8 +311,8 @@ N_Vector SUNLinSolSPFGMR::getResid() const { SUNLinSolSPGMR::SUNLinSolSPGMR(AmiVector const& x, int pretype, int maxl) : SUNLinSolWrapper( - SUNLinSol_SPGMR(const_cast(x.getNVector()), pretype, maxl) - ) { + SUNLinSol_SPGMR(const_cast(x.getNVector()), pretype, maxl) + ) { if (!solver_) throw AmiException("Failed to create solver."); } @@ -404,8 +404,8 @@ SUNNonLinSolFixedPoint::SUNNonLinSolFixedPoint( int count, const_N_Vector x, int m ) : SUNNonLinSolWrapper( - SUNNonlinSol_FixedPointSens(count, const_cast(x), m) - ) {} + SUNNonlinSol_FixedPointSens(count, const_cast(x), m) + ) {} int SUNNonLinSolFixedPoint::getSysFn(SUNNonlinSolSysFn* SysFn) const { return SUNNonlinSolGetSysFn_FixedPoint(solver, SysFn); diff --git a/swig/CMakeLists_model.cmake b/swig/CMakeLists_model.cmake index eb9fe681c9..523571c52e 100644 --- a/swig/CMakeLists_model.cmake +++ b/swig/CMakeLists_model.cmake @@ -1,4 +1,15 @@ cmake_minimum_required(VERSION 3.15) +cmake_policy(VERSION 3.15...3.27) + +# cmake >=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) + if(DEFINED ENV{SWIG}) set(SWIG_EXECUTABLE $ENV{SWIG}) diff --git a/version.txt b/version.txt index 4e8f395fa5..30f6cf8d98 100644 --- a/version.txt +++ b/version.txt @@ -1 +1 @@ -0.26.0 +0.26.1