From a253b17f8156d81a7ee4ba7de495dac68ab01ea1 Mon Sep 17 00:00:00 2001 From: Daniel Weindl Date: Fri, 5 Jan 2024 09:18:23 +0100 Subject: [PATCH 1/6] pre-commit ruff (#2257) Replace black&isort by [ruff](https://docs.astral.sh/ruff/) (faster, additional functionality). * Enable (99%) black-compatible formatting * Enable linting with autofixing * Fix some linting issues * `pre-commit run --all-files` --- .pre-commit-config.yaml | 30 +++++++++---------- documentation/conf.py | 2 +- python/sdist/amici/__init__.py | 11 ++++--- python/sdist/amici/__init__.template.py | 4 +-- python/sdist/amici/de_export.py | 2 +- python/sdist/amici/gradient_check.py | 4 +-- python/sdist/amici/import_utils.py | 6 ++-- python/sdist/amici/petab/util.py | 7 +++-- python/sdist/amici/plotting.py | 3 +- python/sdist/amici/pysb_import.py | 4 +-- python/sdist/amici/sbml_import.py | 9 +++--- python/sdist/amici/sbml_utils.py | 2 +- python/sdist/amici/splines.py | 5 +++- python/sdist/amici/swig_wrappers.py | 2 +- python/sdist/pyproject.toml | 5 ++++ python/tests/conftest.py | 1 - .../lotka_volterra/model/writer.py | 1 - python/tests/test_edata.py | 4 +-- python/tests/test_pysb.py | 1 + tests/conftest.py | 5 +++- .../generateTestConfig/example_steadystate.py | 2 +- 21 files changed, 62 insertions(+), 48 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 521ff54f85..84438209b1 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,12 +1,6 @@ # See https://pre-commit.com for more information # See https://pre-commit.com/hooks.html for more hooks repos: -- repo: https://github.com/pycqa/isort - rev: 5.12.0 - hooks: - - id: isort - name: isort (python) - args: ["--profile", "black", "--filter-files", "--line-length", "79"] - repo: https://github.com/pre-commit/pre-commit-hooks rev: v4.4.0 hooks: @@ -16,15 +10,21 @@ repos: args: [--allow-multiple-documents] - id: end-of-file-fixer - id: trailing-whitespace -- repo: https://github.com/psf/black - rev: 23.7.0 +- repo: https://github.com/astral-sh/ruff-pre-commit + # Ruff version. + rev: v0.1.11 hooks: - - id: black-jupyter - # It is recommended to specify the latest version of Python - # supported by your project here, or alternatively use - # pre-commit's default_language_version, see - # https://pre-commit.com/#top_level-default_language_version - language_version: python3.11 - args: ["--line-length", "79"] + # Run the linter. + - id: ruff + args: + - --fix + - --config + - python/sdist/pyproject.toml + + # Run the formatter. + - id: ruff-format + args: + - --config + - python/sdist/pyproject.toml exclude: '^(ThirdParty|models)/' diff --git a/documentation/conf.py b/documentation/conf.py index 6576601208..c880530e00 100644 --- a/documentation/conf.py +++ b/documentation/conf.py @@ -5,7 +5,6 @@ # This file does only contain a selection of the most common options. For a # full list see the documentation: # http://www.sphinx-doc.org/en/stable/config - import os import re import subprocess @@ -18,6 +17,7 @@ import exhale_multiproject_monkeypatch import mock import pandas as pd +import sphinx import sympy as sp from exhale import configs as exhale_configs from sphinx.transforms.post_transforms import ReferencesResolver diff --git a/python/sdist/amici/__init__.py b/python/sdist/amici/__init__.py index 4ee6f05637..b06ea6d0f3 100644 --- a/python/sdist/amici/__init__.py +++ b/python/sdist/amici/__init__.py @@ -107,18 +107,21 @@ def _imported_from_setup() -> bool: # from .swig_wrappers hdf5_enabled = "readSolverSettingsFromHDF5" in dir() # These modules require the swig interface and other dependencies - from .numpy import ExpDataView, ReturnDataView + from .numpy import ExpDataView, ReturnDataView # noqa: F401 from .pandas import * from .swig_wrappers import * # These modules don't require the swig interface from typing import Protocol, runtime_checkable - from .de_export import DEExporter, DEModel - from .sbml_import import SbmlImporter, assignmentRules2observables + from .de_export import DEExporter, DEModel # noqa: F401 + from .sbml_import import ( # noqa: F401 + SbmlImporter, + assignmentRules2observables, + ) @runtime_checkable - class ModelModule(Protocol): + class ModelModule(Protocol): # noqa: F811 """Type of AMICI-generated model modules. To enable static type checking.""" diff --git a/python/sdist/amici/__init__.template.py b/python/sdist/amici/__init__.template.py index c37ac0f962..f5e49b03dd 100644 --- a/python/sdist/amici/__init__.template.py +++ b/python/sdist/amici/__init__.template.py @@ -15,7 +15,7 @@ "version currently installed." ) -from .TPL_MODELNAME import * -from .TPL_MODELNAME import getModel as get_model +from .TPL_MODELNAME import * # noqa: F403, F401 +from .TPL_MODELNAME import getModel as get_model # noqa: F401 __version__ = "TPL_PACKAGE_VERSION" diff --git a/python/sdist/amici/de_export.py b/python/sdist/amici/de_export.py index 63c9c05072..dddb748cab 100644 --- a/python/sdist/amici/de_export.py +++ b/python/sdist/amici/de_export.py @@ -587,7 +587,7 @@ def smart_multiply( def smart_is_zero_matrix( - x: Union[sp.MutableDenseMatrix, sp.MutableSparseMatrix] + x: Union[sp.MutableDenseMatrix, sp.MutableSparseMatrix], ) -> bool: """A faster implementation of sympy's is_zero_matrix diff --git a/python/sdist/amici/gradient_check.py b/python/sdist/amici/gradient_check.py index 27e2d671d3..5372fcc853 100644 --- a/python/sdist/amici/gradient_check.py +++ b/python/sdist/amici/gradient_check.py @@ -6,7 +6,7 @@ """ import copy -from typing import Callable, List, Optional, Sequence +from typing import List, Optional, Sequence import numpy as np @@ -331,7 +331,7 @@ def _check_results( """ result = rdata[field] - if type(result) is float: + if type(result) is float: # noqa E721 result = np.array(result) _check_close( diff --git a/python/sdist/amici/import_utils.py b/python/sdist/amici/import_utils.py index 2dfc60d0c5..9f75fcff75 100644 --- a/python/sdist/amici/import_utils.py +++ b/python/sdist/amici/import_utils.py @@ -72,7 +72,7 @@ class ObservableTransformation(str, enum.Enum): def noise_distribution_to_observable_transformation( - noise_distribution: Union[str, Callable] + noise_distribution: Union[str, Callable], ) -> ObservableTransformation: """ Parse noise distribution string and extract observable transformation @@ -93,7 +93,7 @@ def noise_distribution_to_observable_transformation( def noise_distribution_to_cost_function( - noise_distribution: Union[str, Callable] + noise_distribution: Union[str, Callable], ) -> Callable[[str], str]: """ Parse noise distribution string to a cost function definition amici can @@ -423,7 +423,7 @@ def _parse_special_functions(sym: sp.Expr, toplevel: bool = True) -> sp.Expr: def _denest_piecewise( - args: Sequence[Union[sp.Expr, sp.logic.boolalg.Boolean, bool]] + args: Sequence[Union[sp.Expr, sp.logic.boolalg.Boolean, bool]], ) -> Tuple[Union[sp.Expr, sp.logic.boolalg.Boolean, bool]]: """ Denest piecewise functions that contain piecewise as condition diff --git a/python/sdist/amici/petab/util.py b/python/sdist/amici/petab/util.py index d30c1a6e9b..8f309e4071 100644 --- a/python/sdist/amici/petab/util.py +++ b/python/sdist/amici/petab/util.py @@ -1,6 +1,6 @@ """Various helper functions for working with PEtab problems.""" import re -from typing import Dict, Tuple, Union +from typing import TYPE_CHECKING, Dict, Tuple, Union import libsbml import pandas as pd @@ -9,6 +9,9 @@ from petab.mapping import resolve_mapping from petab.models import MODEL_TYPE_PYSB, MODEL_TYPE_SBML +if TYPE_CHECKING: + pysb = None + def get_states_in_condition_table( petab_problem: petab.Problem, @@ -63,7 +66,7 @@ def get_states_in_condition_table( spm = pysb.pattern.SpeciesPatternMatcher( model=petab_problem.model.model ) - except NotImplementedError as e: + except NotImplementedError: raise NotImplementedError( "Requires https://github.com/pysb/pysb/pull/570. " "To use this functionality, update pysb via " diff --git a/python/sdist/amici/plotting.py b/python/sdist/amici/plotting.py index edf4a33156..895f83814a 100644 --- a/python/sdist/amici/plotting.py +++ b/python/sdist/amici/plotting.py @@ -56,8 +56,7 @@ def plot_state_trajectories( elif model is not None and prefer_names: labels = np.asarray(model.getStateNames())[list(state_indices)] labels = [ - l if l else model.getStateIds()[ix] - for ix, l in enumerate(labels) + l if l else model.getStateIds()[ix] for ix, l in enumerate(labels) ] elif model is not None: labels = np.asarray(model.getStateIds())[list(state_indices)] diff --git a/python/sdist/amici/pysb_import.py b/python/sdist/amici/pysb_import.py index aa1dc7cd9b..5862d515a1 100644 --- a/python/sdist/amici/pysb_import.py +++ b/python/sdist/amici/pysb_import.py @@ -410,7 +410,7 @@ def _process_pysb_species(pysb_model: pysb.Model, ode_model: DEModel) -> None: sp.Symbol(f"__s{ix}"), f"{specie}", init, xdot[ix] ) ) - logger.debug(f"Finished Processing PySB species ") + logger.debug("Finished Processing PySB species ") @log_execution_time("processing PySB parameters", logger) @@ -1353,7 +1353,7 @@ def has_fixed_parameter_ic( def extract_monomers( - complex_patterns: Union[pysb.ComplexPattern, List[pysb.ComplexPattern]] + complex_patterns: Union[pysb.ComplexPattern, List[pysb.ComplexPattern]], ) -> List[str]: """ Constructs a list of monomer names contained in complex patterns. diff --git a/python/sdist/amici/sbml_import.py b/python/sdist/amici/sbml_import.py index 124dfc7a63..d8361c04ab 100644 --- a/python/sdist/amici/sbml_import.py +++ b/python/sdist/amici/sbml_import.py @@ -21,7 +21,6 @@ List, Optional, Sequence, - Set, Tuple, Union, ) @@ -1165,7 +1164,7 @@ def _process_reactions(self): # accounts for possibly variable compartments. self.stoichiometric_matrix[ species["index"], reaction_index - ] += (sign * stoichiometry * species["conversion_factor"]) + ] += sign * stoichiometry * species["conversion_factor"] if reaction.isSetId(): sym_math = self._local_symbols[reaction.getId()] else: @@ -2360,9 +2359,9 @@ def _replace_in_all_expressions( # rule (at the end of the _process_species method), hence needs to be # processed here too. self.compartments = { - smart_subs(c, old, new) - if replace_identifiers - else c: smart_subs(v, old, self._make_initial(new)) + smart_subs(c, old, new) if replace_identifiers else c: smart_subs( + v, old, self._make_initial(new) + ) for c, v in self.compartments.items() } diff --git a/python/sdist/amici/sbml_utils.py b/python/sdist/amici/sbml_utils.py index 66c9d01bbc..75bcb82a18 100644 --- a/python/sdist/amici/sbml_utils.py +++ b/python/sdist/amici/sbml_utils.py @@ -519,7 +519,7 @@ def mathml2sympy( def _parse_logical_operators( - math_str: Union[str, float, None] + math_str: Union[str, float, None], ) -> Union[str, float, None]: """ Parses a math string in order to replace logical operators by a form diff --git a/python/sdist/amici/splines.py b/python/sdist/amici/splines.py index fdb0912045..657170c2f5 100644 --- a/python/sdist/amici/splines.py +++ b/python/sdist/amici/splines.py @@ -1510,7 +1510,10 @@ def spline_user_functions( "AmiciSplineSensitivity": [ ( lambda *args: True, - lambda spline_id, x, param_id, *p: f"sspl_{spline_ids.index(spline_id)}_{p_index[param_id]}", + lambda spline_id, + x, + param_id, + *p: f"sspl_{spline_ids.index(spline_id)}_{p_index[param_id]}", ) ], } diff --git a/python/sdist/amici/swig_wrappers.py b/python/sdist/amici/swig_wrappers.py index f56f3bd5d2..4983a28f23 100644 --- a/python/sdist/amici/swig_wrappers.py +++ b/python/sdist/amici/swig_wrappers.py @@ -54,7 +54,7 @@ def _capture_cstdout(): def _get_ptr( - obj: Union[AmiciModel, AmiciExpData, AmiciSolver, AmiciReturnData] + obj: Union[AmiciModel, AmiciExpData, AmiciSolver, AmiciReturnData], ) -> Union[ "amici_swig.Model", "amici_swig.ExpData", diff --git a/python/sdist/pyproject.toml b/python/sdist/pyproject.toml index 011064fbdb..91b8484af6 100644 --- a/python/sdist/pyproject.toml +++ b/python/sdist/pyproject.toml @@ -15,3 +15,8 @@ build-backend = "setuptools.build_meta" [tool.black] line-length = 79 + +[tool.ruff] +line-length = 79 +ignore = ["E402", "F403", "F405", "E741"] +extend-include = ["*.ipynb"] diff --git a/python/tests/conftest.py b/python/tests/conftest.py index 9ab64b91d7..1da7cb31b3 100644 --- a/python/tests/conftest.py +++ b/python/tests/conftest.py @@ -2,7 +2,6 @@ import copy import importlib import os -import shutil import sys import amici diff --git a/python/tests/petab_test_problems/lotka_volterra/model/writer.py b/python/tests/petab_test_problems/lotka_volterra/model/writer.py index 76b98c3deb..c6abebaebe 100644 --- a/python/tests/petab_test_problems/lotka_volterra/model/writer.py +++ b/python/tests/petab_test_problems/lotka_volterra/model/writer.py @@ -1,6 +1,5 @@ from pathlib import Path -import petab import yaml2sbml yaml2sbml_yaml = "lotka_volterra.yaml" diff --git a/python/tests/test_edata.py b/python/tests/test_edata.py index 9c4d9b9edc..27c67de61e 100644 --- a/python/tests/test_edata.py +++ b/python/tests/test_edata.py @@ -2,11 +2,11 @@ import amici import numpy as np from amici.testing import skip_on_valgrind -from test_sbml_import import model_units_module +from test_sbml_import import model_units_module # noqa: F401 @skip_on_valgrind -def test_edata_sensi_unscaling(model_units_module): +def test_edata_sensi_unscaling(model_units_module): # noqa: F811 """ ExpData parameters should be used for unscaling initial state sensitivities. diff --git a/python/tests/test_pysb.py b/python/tests/test_pysb.py index 52ca3a320f..2911b05fc9 100644 --- a/python/tests/test_pysb.py +++ b/python/tests/test_pysb.py @@ -1,4 +1,5 @@ """PYSB model tests""" +# flake8: noqa: F821 import importlib import logging diff --git a/tests/conftest.py b/tests/conftest.py index 9e90400518..7d98a09abb 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -3,10 +3,13 @@ import re import sys from pathlib import Path -from typing import List, Set, Tuple +from typing import TYPE_CHECKING, List, Set, Tuple import pytest +if TYPE_CHECKING: + from _pytest.reports import TestReport + # stores passed SBML semantic test suite IDs passed_ids = [] diff --git a/tests/generateTestConfig/example_steadystate.py b/tests/generateTestConfig/example_steadystate.py index 07fab49dfe..80e8a776d2 100755 --- a/tests/generateTestConfig/example_steadystate.py +++ b/tests/generateTestConfig/example_steadystate.py @@ -2,7 +2,7 @@ import sys import numpy as np -from example import AmiciExample, dict2attrs +from example import AmiciExample class ExampleSteadystate(AmiciExample): From 68a6625eadf54a214bb0eda333139e84a3b7a46a Mon Sep 17 00:00:00 2001 From: Daniel Weindl Date: Fri, 5 Jan 2024 09:32:50 +0100 Subject: [PATCH 2/6] Doc: Update edata.h (#2254) --- include/amici/edata.h | 61 ++++++++++++++++++++++--------------------- 1 file changed, 31 insertions(+), 30 deletions(-) diff --git a/include/amici/edata.h b/include/amici/edata.h index f8639ca2eb..78bcba0fc7 100644 --- a/include/amici/edata.h +++ b/include/amici/edata.h @@ -14,24 +14,24 @@ class ReturnData; /** * @brief ExpData carries all information about experimental or - * condition-specific data + * condition-specific data. */ class ExpData : public SimulationParameters { public: /** - * @brief default constructor + * @brief Default constructor. */ ExpData() = default; /** - * @brief Copy constructor, needs to be declared to be generated in - * swig + * @brief Copy constructor. */ + // needs to be declared to be wrapped by SWIG ExpData(ExpData const&) = default; /** - * @brief constructor that only initializes dimensions + * @brief Constructor that only initializes dimensions. * * @param nytrue Number of observables * @param nztrue Number of event outputs @@ -154,6 +154,16 @@ class ExpData : public SimulationParameters { /** * @brief Set output timepoints. * + * If the number of timepoint increases, this will grow the + * observation/sigma matrices and fill new entries with NaN. + * If the number of timepoints decreases, this will shrink the + * observation/sigma matrices. + * + * Note that the mapping from timepoints to measurements will not be + * preserved. E.g., say there are measurements at t = 2, and this + * function is called with [1, 2], then the old measurements will belong to + * t = 1. + * * @param ts timepoints */ void setTimepoints(std::vector const& ts); @@ -225,7 +235,7 @@ class ExpData : public SimulationParameters { void setObservedDataStdDev(std::vector const& observedDataStdDev); /** - * @brief Set indentical standard deviation for all measurements. + * @brief Set identical standard deviation for all measurements. * * @param stdDev standard deviation (dimension: scalar) */ @@ -278,8 +288,7 @@ class ExpData : public SimulationParameters { realtype const* getObservedDataStdDevPtr(int it) const; /** - * @brief set function that copies observed event data from input to - * ExpData::observedEvents + * @brief Set observed event data. * * @param observedEvents observed data (dimension: nmaxevent x nztrue, * row-major) @@ -287,8 +296,7 @@ class ExpData : public SimulationParameters { void setObservedEvents(std::vector const& observedEvents); /** - * @brief set function that copies observed event data for specific event - * observable + * @brief Set observed event data for specific event observable. * * @param observedEvents observed data (dimension: nmaxevent) * @param iz observed event data index @@ -296,8 +304,7 @@ class ExpData : public SimulationParameters { void setObservedEvents(std::vector const& observedEvents, int iz); /** - * @brief get function that checks whether event data at specified indices - * has been set + * @brief Check whether event data at specified indices has been set. * * @param ie event index * @param iz event observable index @@ -306,25 +313,23 @@ class ExpData : public SimulationParameters { bool isSetObservedEvents(int ie, int iz) const; /** - * @brief get function that copies data from ExpData::mz to output + * @brief Get observed event data. * * @return observed event data */ std::vector const& getObservedEvents() const; /** - * @brief get function that returns a pointer to observed data at ieth - * occurrence + * @brief Get pointer to observed data at ie-th occurrence. * * @param ie event occurrence * - * @return pointer to observed event data at ieth occurrence + * @return pointer to observed event data at ie-th occurrence */ realtype const* getObservedEventsPtr(int ie) const; /** - * @brief set function that copies data from input to - * ExpData::observedEventsStdDev + * @brief Set standard deviation of observed event data. * * @param observedEventsStdDev standard deviation of observed event data */ @@ -332,16 +337,14 @@ class ExpData : public SimulationParameters { setObservedEventsStdDev(std::vector const& observedEventsStdDev); /** - * @brief set function that sets all ExpData::observedDataStdDev to the - * input value + * @brief Set standard deviation of observed event data. * * @param stdDev standard deviation (dimension: scalar) */ void setObservedEventsStdDev(realtype stdDev); /** - * @brief set function that copies standard deviation of observed data for - * specific observable + * @brief Set standard deviation of observed data for a specific observable. * * @param observedEventsStdDev standard deviation of observed data * (dimension: nmaxevent) @@ -352,8 +355,7 @@ class ExpData : public SimulationParameters { ); /** - * @brief set function that sets all standard deviation of a specific - * observable to the input value + * @brief Set all standard deviations of a specific event-observable. * * @param stdDev standard deviation (dimension: scalar) * @param iz observed data index @@ -361,8 +363,8 @@ class ExpData : public SimulationParameters { void setObservedEventsStdDev(realtype stdDev, int iz); /** - * @brief get function that checks whether standard deviation of even data - * at specified indices has been set + * @brief Check whether standard deviation of event data + * at specified indices has been set. * * @param ie event index * @param iz event observable index @@ -371,16 +373,15 @@ class ExpData : public SimulationParameters { bool isSetObservedEventsStdDev(int ie, int iz) const; /** - * @brief get function that copies data from ExpData::observedEventsStdDev - * to output + * @brief Get standard deviation of observed event data. * * @return standard deviation of observed event data */ std::vector const& getObservedEventsStdDev() const; /** - * @brief get function that returns a pointer to standard deviation of - * observed event data at ie-th occurrence + * @brief Get pointer to standard deviation of + * observed event data at ie-th occurrence. * * @param ie event occurrence * From 9886c690f3c5619148a6fd9541e7d3741bbf108a Mon Sep 17 00:00:00 2001 From: Daniel Weindl Date: Sat, 6 Jan 2024 10:40:32 +0100 Subject: [PATCH 3/6] Fix swig shadow warning + other linting issues (#2261) Fixes swig warning ``` include/amici/simulation_parameters.h:71: Warning 509: Overloaded method amici::SimulationParameters::SimulationParameters(std::vector< amici::realtype,std::allocator< amici::realtype > >,std::vector< amici::realtype,std::allocator< amici::realtype > >,std::vector< amici::realtype,std::allocator< amici::realtype > >) effectively ignored, include/amici/simulation_parameters.h:54: Warning 509: as it is shadowed by amici::SimulationParameters::SimulationParameters(std::vector< amici::realtype,std::allocator< amici::realtype > >,std::vector< amici::realtype,std::allocator< amici::realtype > >,std::vector< int,std::allocator< int > >). ``` and some other linting warnings. --- include/amici/logging.h | 2 +- include/amici/model.h | 2 +- include/amici/simulation_parameters.h | 6 ++++++ src/steadystateproblem.cpp | 6 +++--- 4 files changed, 11 insertions(+), 5 deletions(-) diff --git a/include/amici/logging.h b/include/amici/logging.h index 0118bedd28..6af039d4c4 100644 --- a/include/amici/logging.h +++ b/include/amici/logging.h @@ -83,7 +83,7 @@ struct LogItem { , message(message){}; /** Severity level */ - LogSeverity severity; + LogSeverity severity = LogSeverity::error; /** Short identifier for the logged event */ std::string identifier; diff --git a/include/amici/model.h b/include/amici/model.h index f5421bed3d..3d2645028a 100644 --- a/include/amici/model.h +++ b/include/amici/model.h @@ -1437,7 +1437,7 @@ class Model : public AbstractModel, public ModelDimensions { std::vector const& getReinitializationStateIdxs() const; /** Flag indicating Matlab- or Python-based model generation */ - bool pythonGenerated; + bool pythonGenerated = false; /** * @brief getter for dxdotdp (matlab generated) diff --git a/include/amici/simulation_parameters.h b/include/amici/simulation_parameters.h index ca0e127c5c..55db090184 100644 --- a/include/amici/simulation_parameters.h +++ b/include/amici/simulation_parameters.h @@ -35,6 +35,11 @@ class SimulationParameters { this->parameters.size(), ParameterScaling::none )) {} +#ifndef SWIGPYTHON + /* + * include/amici/simulation_parameters.h:71: Warning 509: Overloaded method amici::SimulationParameters::SimulationParameters(std::vector< amici::realtype,std::allocator< amici::realtype > >,std::vector< amici::realtype,std::allocator< amici::realtype > >,std::vector< amici::realtype,std::allocator< amici::realtype > >) effectively ignored, + * include/amici/simulation_parameters.h:54: Warning 509: as it is shadowed by amici::SimulationParameters::SimulationParameters(std::vector< amici::realtype,std::allocator< amici::realtype > >,std::vector< amici::realtype,std::allocator< amici::realtype > >,std::vector< int,std::allocator< int > >). + */ /** * @brief Constructor * @param fixedParameters Model constants @@ -69,6 +74,7 @@ class SimulationParameters { this->parameters.size(), ParameterScaling::none )) , ts_(std::move(timepoints)) {} +#endif /** * @brief Set reinitialization of all states based on model constants for diff --git a/src/steadystateproblem.cpp b/src/steadystateproblem.cpp index 4576b43363..98c36589f7 100644 --- a/src/steadystateproblem.cpp +++ b/src/steadystateproblem.cpp @@ -601,8 +601,6 @@ void SteadystateProblem::applyNewtonsMethod(Model& model, bool newton_retry) { int& i_newtonstep = numsteps_.at(newton_retry ? 2 : 0); i_newtonstep = 0; gamma_ = 1.0; - bool update_direction = true; - bool step_successful = false; if (model.nx_solver == 0) return; @@ -613,6 +611,8 @@ void SteadystateProblem::applyNewtonsMethod(Model& model, bool newton_retry) { bool converged = false; wrms_ = getWrms(model, SensitivityMethod::none); converged = newton_retry ? false : wrms_ < conv_thresh; + bool update_direction = true; + while (!converged && i_newtonstep < max_steps_) { /* If Newton steps are necessary, compute the initial search @@ -634,7 +634,7 @@ void SteadystateProblem::applyNewtonsMethod(Model& model, bool newton_retry) { /* Compute new xdot and residuals */ realtype wrms_tmp = getWrms(model, SensitivityMethod::none); - step_successful = wrms_tmp < wrms_; + bool step_successful = wrms_tmp < wrms_; if (step_successful) { /* If new residuals are smaller than old ones, update state */ wrms_ = wrms_tmp; From 30ed2f0feb338cb30207f1ff7784280e26ab17de Mon Sep 17 00:00:00 2001 From: Daniel Weindl Date: Sat, 6 Jan 2024 10:46:19 +0100 Subject: [PATCH 4/6] Fix `SwigPtrView.__getattr__` (#2259) `hasattr` didn't work with SwigPtrView, because calling `__getattr__` for a missing attribute resulted in a KeyError via `__missing__`. However, hasattr expects an attribute error in that case. Fixed. Added tests. --- python/sdist/amici/numpy.py | 5 ++++- python/tests/test_swig_interface.py | 25 +++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/python/sdist/amici/numpy.py b/python/sdist/amici/numpy.py index 93b04603be..b259aca2a0 100644 --- a/python/sdist/amici/numpy.py +++ b/python/sdist/amici/numpy.py @@ -80,7 +80,10 @@ def __getattr__(self, item) -> Union[np.ndarray, float]: :returns: value """ - return self.__getitem__(item) + try: + return self.__getitem__(item) + except KeyError as e: + raise AttributeError(item) from e def __init__(self, swigptr): """ diff --git a/python/tests/test_swig_interface.py b/python/tests/test_swig_interface.py index 3eabafd49b..ffec95b77b 100644 --- a/python/tests/test_swig_interface.py +++ b/python/tests/test_swig_interface.py @@ -6,6 +6,8 @@ import copy import numbers +import pytest + import amici import numpy as np @@ -500,3 +502,26 @@ def test_model_is_deepcopyable(pysb_example_presimulation_module): assert model1.t0() == model2.t0() model2.setT0(100 + model2.t0()) assert model1.t0() != model2.t0() + + +def test_rdataview(sbml_example_presimulation_module): + """Test some SwigPtrView functionality via ReturnDataView.""" + model_module = sbml_example_presimulation_module + model = model_module.getModel() + rdata = amici.runAmiciSimulation(model, model.getSolver()) + assert isinstance(rdata, amici.ReturnDataView) + + # fields are accessible via dot notation and [] operator, + # __contains__ and __getattr__ are implemented correctly + with pytest.raises(AttributeError): + _ = rdata.nonexisting_attribute + + with pytest.raises(KeyError): + _ = rdata["nonexisting_attribute"] + + assert not hasattr(rdata, "nonexisting_attribute") + assert "x" in rdata + assert rdata.x == rdata["x"] + + # field names are included by dir() + assert "x" in dir(rdata) From 66f4ba540cba7fa562baa0f5b93b670526416b32 Mon Sep 17 00:00:00 2001 From: Daniel Weindl Date: Sat, 6 Jan 2024 11:43:57 +0100 Subject: [PATCH 5/6] GHA: Split macOS C++ / Python test jobs (#2260) Currently, macOS jobs run for >1h, this can be rather annoying. Splitting current tests into two separate jobs. Now we have 33' + 23' instead of 1h20'. Also: * cleanup some bash scripts * manage dependencies via pyproject.toml * move h5py dependency to extras * parallelize model compilation on macos --- .../install-macos-dependencies/action.yml | 31 ++++++++++ .github/workflows/test_python_cplusplus.yml | 60 +++++++++++++++---- python/sdist/setup.cfg | 6 +- scripts/installAmiciSource.sh | 32 +++++----- scripts/run-python-tests.sh | 12 ++-- scripts/run-valgrind-py.sh | 3 +- 6 files changed, 109 insertions(+), 35 deletions(-) create mode 100644 .github/actions/install-macos-dependencies/action.yml diff --git a/.github/actions/install-macos-dependencies/action.yml b/.github/actions/install-macos-dependencies/action.yml new file mode 100644 index 0000000000..b19cac1052 --- /dev/null +++ b/.github/actions/install-macos-dependencies/action.yml @@ -0,0 +1,31 @@ +name: Install AMICI dependencies for MacOS +description: Install AMICI dependencies for MacOS + +runs: + using: "composite" + steps: + # use all available cores + - run: echo "AMICI_PARALLEL_COMPILE=" >> $GITHUB_ENV + shell: bash + + # AMICI repository root + - run: echo "AMICI_DIR=$(pwd)" >> $GITHUB_ENV + shell: bash + + # BioNetGen path + - run: echo "BNGPATH=${AMICI_DIR}/ThirdParty/BioNetGen-2.7.0" >> $GITHUB_ENV + shell: bash + + # CMake hints + # Ensure CMake is using the python version that we will use for the python tests later on + - run: echo "PYTHON_EXECUTABLE=${Python3_ROOT_DIR}/bin/python3" >> $GITHUB_ENV + shell: bash + - run: echo "OpenMP_ROOT=$(brew --prefix)/opt/libomp" >> $GITHUB_ENV + shell: bash + - run: echo "BOOST_ROOT=$(brew --prefix)/opt/boost" >> $GITHUB_ENV + shell: bash + + # install amici dependencies + - name: homebrew + run: brew install hdf5 swig gcc libomp boost + shell: bash diff --git a/.github/workflows/test_python_cplusplus.yml b/.github/workflows/test_python_cplusplus.yml index 496779c6ec..5aa47173b8 100644 --- a/.github/workflows/test_python_cplusplus.yml +++ b/.github/workflows/test_python_cplusplus.yml @@ -218,8 +218,8 @@ jobs: # TODO: Include notebooks in coverage report - osx: - name: Tests OSX + macos_cpp_py: + name: Tests MacOS C++/Python runs-on: macos-latest steps: @@ -231,16 +231,11 @@ jobs: - uses: actions/checkout@v3 - run: git fetch --prune --unshallow - - run: echo "AMICI_DIR=$(pwd)" >> $GITHUB_ENV - - run: echo "BNGPATH=${AMICI_DIR}/ThirdParty/BioNetGen-2.7.0" >> $GITHUB_ENV - # Ensure CMake is using the python version that we will use for the python tests later on - - run: echo "PYTHON_EXECUTABLE=${Python3_ROOT_DIR}/bin/python3" >> $GITHUB_ENV - - run: echo "OpenMP_ROOT=$(brew --prefix)/opt/libomp" >> $GITHUB_ENV - - run: echo "BOOST_ROOT=$(brew --prefix)/opt/boost" >> $GITHUB_ENV + - name: Install dependencies + uses: ./.github/actions/install-macos-dependencies - # install amici dependencies - name: homebrew - run: brew install hdf5 swig gcc cppcheck libomp boost + run: brew install cppcheck - name: Build AMICI run: scripts/buildAll.sh @@ -254,8 +249,47 @@ jobs: - name: cppcheck run: scripts/run-cppcheck.sh - - name: Python tests - run: scripts/run-python-tests.sh - - name: C++ tests run: scripts/run-cpp-tests.sh + + - name: Python tests + run: | + scripts/run-python-tests.sh \ + test_pregenerated_models.py \ + test_splines_short.py \ + test_misc.py + + + macos_python: + name: Tests MacOS Python + runs-on: macos-latest + + steps: + - name: Set up Python + uses: actions/setup-python@v4 + with: + python-version: 3.9 + + - uses: actions/checkout@v3 + - run: git fetch --prune --unshallow + + - name: Install dependencies + uses: ./.github/actions/install-macos-dependencies + + - name: Install python package + run: | + pip show numpy > /dev/null || python3 -m pip install numpy + scripts/installAmiciSource.sh + + - name: Check OpenMP support + run: source build/venv/bin/activate && python -c "import amici; import sys; sys.exit(not amici.compiledWithOpenMP())" + + - name: Get BioNetGen + run: scripts/buildBNGL.sh + + - name: Python tests + run: | + scripts/run-python-tests.sh \ + --ignore=test_pregenerated_models.py \ + --ignore=test_splines_short.py \ + --ignore=test_misc.py diff --git a/python/sdist/setup.cfg b/python/sdist/setup.cfg index 8e8797fda7..5ae8e1e742 100644 --- a/python/sdist/setup.cfg +++ b/python/sdist/setup.cfg @@ -34,7 +34,6 @@ install_requires = numpy>=1.23.2; python_version=='3.11' numpy; python_version>='3.12' python-libsbml - h5py pandas>=2.0.2 wurlitzer toposort @@ -47,6 +46,7 @@ zip_safe = False petab = petab>=0.2.1 pysb = pysb>=1.13.1 test = + h5py pytest pytest-cov pytest-rerunfailures @@ -56,9 +56,13 @@ test = # see https://github.com/sys-bio/antimony/issues/92 # unsupported x86_64 / x86_64h antimony!=2.14; platform_system=='Darwin' and platform_machine in 'x86_64h' + scipy vis = matplotlib seaborn +examples = + jupyter + scipy [options.package_data] amici = diff --git a/scripts/installAmiciSource.sh b/scripts/installAmiciSource.sh index 4e693468b7..bbb4bf4a83 100755 --- a/scripts/installAmiciSource.sh +++ b/scripts/installAmiciSource.sh @@ -1,35 +1,37 @@ #!/bin/bash -# -# Build libamici -# +# Create a virtual environment and perform an editable amici installation set -e SCRIPT_PATH=$(dirname $BASH_SOURCE) -AMICI_PATH=$(cd $SCRIPT_PATH/.. && pwd) +AMICI_PATH=$(cd "$SCRIPT_PATH/.." && pwd) +venv_dir="${AMICI_PATH}/build/venv" # Disabled until cmake package is made compatible with updated setup.py #make python-wheel #pip3 install --user --prefix= `ls -t ${AMICI_PATH}/build/python/amici-*.whl | head -1` # test install from setup.py set +e -python3 -m venv ${AMICI_PATH}/build/venv --clear +mkdir -p "${venv_dir}" +python3 -m venv "${venv_dir}" --clear # in case this fails (usually due to missing ensurepip, try getting pip # manually if [[ $? ]]; then set -e - python3 -m venv ${AMICI_PATH}/build/venv --clear --without-pip - source ${AMICI_PATH}/build/venv/bin/activate - curl https://bootstrap.pypa.io/get-pip.py -o ${AMICI_PATH}/build/get-pip.py - python3 ${AMICI_PATH}/build/get-pip.py + python3 -m venv "${venv_dir}" --clear --without-pip + source "${venv_dir}/bin/activate" + get_pip=${AMICI_PATH}/build/get-pip.py + curl "https://bootstrap.pypa.io/get-pip.py" -o "${get_pip}" + python3 "${get_pip}" + rm "${get_pip}" else set -e - source ${AMICI_PATH}/build/venv/bin/activate + source "${venv_dir}/bin/activate" fi -pip install --upgrade pip wheel -pip install --upgrade pip scipy matplotlib coverage pytest \ - pytest-cov cmake_build_extension numpy -pip install git+https://github.com/FFroehlich/pysb@fix_pattern_matching # pin to PR for SPM with compartments -AMICI_BUILD_TEMP="${AMICI_PATH}/python/sdist/build/temp" pip install --verbose -e ${AMICI_PATH}/python/sdist[petab,test,vis] --no-build-isolation +python -m pip install --upgrade pip wheel +python -m pip install --upgrade pip setuptools cmake_build_extension numpy +python -m pip install git+https://github.com/FFroehlich/pysb@fix_pattern_matching # pin to PR for SPM with compartments +AMICI_BUILD_TEMP="${AMICI_PATH}/python/sdist/build/temp" \ + python -m pip install --verbose -e "${AMICI_PATH}/python/sdist[petab,test,vis]" --no-build-isolation deactivate diff --git a/scripts/run-python-tests.sh b/scripts/run-python-tests.sh index 982aa02f0f..d58a1c8dec 100755 --- a/scripts/run-python-tests.sh +++ b/scripts/run-python-tests.sh @@ -1,7 +1,8 @@ #!/bin/bash -# Test python model wrapping inside virtual environment +# Run Python test suite inside virtual environment +# Usage: ./run-python-tests.sh [additional pytest arguments] -script_path=$(dirname $BASH_SOURCE) +script_path=$(dirname "${BASH_SOURCE[0]}") amici_path=$(cd "$script_path"/.. && pwd) set -e @@ -12,7 +13,10 @@ fi cd "${amici_path}"/python/tests source "${amici_path}"/build/venv/bin/activate -pip install scipy h5py pytest pytest-cov # PEtab tests are run separately -pytest --ignore-glob=*petab* --ignore-glob=*test_splines.py +pytest \ + --ignore-glob=*petab* \ + --ignore-glob=*test_splines.py \ + --durations=10 \ + $@ diff --git a/scripts/run-valgrind-py.sh b/scripts/run-valgrind-py.sh index d9e6158d51..c2a6239ad4 100755 --- a/scripts/run-valgrind-py.sh +++ b/scripts/run-valgrind-py.sh @@ -2,7 +2,7 @@ # Without arguments: run Python test suite under valgrind # With arguments: run whatever was passed as arguments under valgrind -script_path=$(dirname $BASH_SOURCE) +script_path=$(dirname "${BASH_SOURCE[0]}") amici_path=$(cd "$script_path"/.. && pwd) set -e @@ -16,7 +16,6 @@ if [ $# -eq 0 ] # No arguments supplied, run all tests cd "${amici_path}"/python/tests source "${amici_path}"/build/venv/bin/activate - pip install scipy h5py pytest pytest-rerunfailures command=(python -m pytest -vv --ignore-glob=*petab* -W 'ignore:Signature ') # ^ ignores the following warning that occurs only under valgrind, # e.g. `valgrind python -c "import h5py"`: From 3d01ebbdd5d243b715a845f1ce4bd6cca10a08aa Mon Sep 17 00:00:00 2001 From: Daniel Weindl Date: Sun, 7 Jan 2024 14:10:21 +0100 Subject: [PATCH 6/6] Use Model.setAlwaysCheckFinite(True) in debug builds (#2262) * Use Model.setAlwaysCheckFinite(True) in debug builds So that we automatically use the additional checks in our tests. Closes #2235 * Fix sparse-`unravel_index` for the case where no indices/ptrs have been set (this is the case for matlab-generated models). --- include/amici/model.h | 4 ++++ python/tests/test_swig_interface.py | 12 +++++++---- src/sundials_matrix_wrapper.cpp | 33 +++++++++++++++++------------ tests/cpp/unittests/testMisc.cpp | 12 ++++++++++- 4 files changed, 42 insertions(+), 19 deletions(-) diff --git a/include/amici/model.h b/include/amici/model.h index 3d2645028a..d355ea7fee 100644 --- a/include/amici/model.h +++ b/include/amici/model.h @@ -2020,7 +2020,11 @@ class Model : public AbstractModel, public ModelDimensions { * Indicates whether the result of every call to `Model::f*` should be * checked for finiteness */ +#ifdef NDEBUG bool always_check_finite_{false}; +#else + bool always_check_finite_{true}; +#endif /** indicates whether sigma residuals are to be added for every datapoint */ bool sigma_res_{false}; diff --git a/python/tests/test_swig_interface.py b/python/tests/test_swig_interface.py index ffec95b77b..536c107458 100644 --- a/python/tests/test_swig_interface.py +++ b/python/tests/test_swig_interface.py @@ -68,10 +68,7 @@ def test_copy_constructors(pysb_example_presimulation_module): model_instance_settings0 = { # setting name: [default value, custom value] "AddSigmaResiduals": [False, True], - "AlwaysCheckFinite": [ - False, - True, - ], + "AlwaysCheckFinite": [False, True], # Skipped due to model dependency in `'InitialStates'`. "FixedParameters": None, "InitialStates": [ @@ -132,6 +129,13 @@ def test_model_instance_settings(pysb_example_presimulation_module): i_getter = 0 i_setter = 1 + # the default setting for AlwaysCheckFinite depends on whether the amici + # extension has been built in debug mode + model_instance_settings0["AlwaysCheckFinite"] = [ + model0.getAlwaysCheckFinite(), + not model0.getAlwaysCheckFinite(), + ] + # All settings are tested. assert set(model_instance_settings0) == set( amici.swig_wrappers.model_instance_settings diff --git a/src/sundials_matrix_wrapper.cpp b/src/sundials_matrix_wrapper.cpp index b5c7300628..a86574cd82 100644 --- a/src/sundials_matrix_wrapper.cpp +++ b/src/sundials_matrix_wrapper.cpp @@ -793,20 +793,25 @@ unravel_index(sunindextype i, SUNMatrix m) { } if (mat_id == SUNMATRIX_SPARSE) { - gsl_ExpectsDebug(i < SM_NNZ_S(m)); - sunindextype row = SM_INDEXVALS_S(m)[i]; - sunindextype i_colptr = 0; - while (SM_INDEXPTRS_S(m)[i_colptr] < SM_NNZ_S(m)) { - if (SM_INDEXPTRS_S(m)[i_colptr + 1] > i) { - sunindextype col = i_colptr; - gsl_EnsuresDebug(row >= 0); - gsl_EnsuresDebug(row < SM_ROWS_S(m)); - gsl_EnsuresDebug(col >= 0); - gsl_EnsuresDebug(col < SM_COLUMNS_S(m)); - return {row, col}; - } - ++i_colptr; - } + auto nnz = SM_NNZ_S(m); + auto ncols = SM_COLUMNS_S(m); + auto index_vals = SM_INDEXVALS_S(m); + auto index_ptrs = SM_INDEXPTRS_S(m); + gsl_ExpectsDebug(i < nnz); + sunindextype row = index_vals[i]; + sunindextype col = 0; + while (col < ncols && index_ptrs[col + 1] <= i) + ++col; + + // This can happen if indexvals / indexptrs haven't been set. + if(col == ncols) + return {-1, -1}; + + gsl_EnsuresDebug(row >= 0); + gsl_EnsuresDebug(row < SM_ROWS_S(m)); + gsl_EnsuresDebug(col >= 0); + gsl_EnsuresDebug(col < ncols); + return {row, col}; } throw amici::AmiException("Unimplemented SUNMatrix type for unravel_index"); diff --git a/tests/cpp/unittests/testMisc.cpp b/tests/cpp/unittests/testMisc.cpp index 14af3a7a82..f18de96b79 100644 --- a/tests/cpp/unittests/testMisc.cpp +++ b/tests/cpp/unittests/testMisc.cpp @@ -689,7 +689,7 @@ TEST(UnravelIndex, UnravelIndexSunMatSparse) // [2, 0] // data [1, 2, 3] // colptrs [0, 2, 3] - // rowidxs [2, 3, 1] + // rowidxs [2, 3, 0] D.set_data(0, 0, 0); D.set_data(1, 0, 0); D.set_data(2, 0, 1); @@ -708,6 +708,16 @@ TEST(UnravelIndex, UnravelIndexSunMatSparse) SUNMatDestroy(S); } + +TEST(UnravelIndex, UnravelIndexSunMatSparseMissingIndices) +{ + // Sparse matrix without any indices set + SUNMatrixWrapper mat = SUNMatrixWrapper(2, 3, 2, CSC_MAT); + EXPECT_EQ(unravel_index(0, mat.get()), std::make_pair((sunindextype) -1, (sunindextype) -1)); + EXPECT_EQ(unravel_index(1, mat.get()), std::make_pair((sunindextype) -1, (sunindextype) -1)); +} + + TEST(ReturnCodeToStr, ReturnCodeToStr) { EXPECT_EQ("AMICI_SUCCESS", simulation_status_to_str(AMICI_SUCCESS));