Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cached simplify in ODE export #1672

Merged
merged 10 commits into from
Feb 19, 2022
Merged

Cached simplify in ODE export #1672

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
eca64cc
cache simplifications
dilpath Feb 16, 2022
ee7cd95
Update python/amici/ode_export.py
dilpath Feb 16, 2022
768e26a
pysb error workaround: argument to enable caching; default sbml to ca…
dilpath Feb 16, 2022
bd77cc7
remove copy
dilpath Feb 17, 2022
1accaac
Merge branch 'develop' into cached_simplify
dilpath Feb 17, 2022
8c27c36
default no cache
dilpath Feb 17, 2022
96320ab
default off everywhere
dilpath Feb 17, 2022
8b574ca
enable pysb again
dilpath Feb 18, 2022
5c98eea
exclude from pysb
dilpath Feb 18, 2022
8232cf8
add usage warnings, add option to turn on for pysb
dilpath Feb 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 17 additions & 1 deletion python/amici/ode_export.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -959,7 +959,8 @@ class ODEModel:
"""

def __init__(self, verbose: Optional[Union[bool, int]] = False,
simplify: Optional[Callable] = sp.powsimp):
simplify: Optional[Callable] = sp.powsimp,
cache_simplify: bool = False):
"""
Create a new ODEModel instance.

Expand All @@ -969,6 +970,10 @@ def __init__(self, verbose: Optional[Union[bool, int]] = False,

:param simplify:
see :meth:`ODEModel._simplify`

:param cache_simplify:
Whether to cache calls to the simplify method. Can e.g. decrease
import times for models with events.
"""
self._states: List[State] = []
self._observables: List[Observable] = []
Expand Down Expand Up @@ -1037,6 +1042,17 @@ def __init__(self, verbose: Optional[Union[bool, int]] = False,

self._lock_total_derivative: List[str] = list()
self._simplify: Callable = simplify
if cache_simplify and simplify is not None:
def cached_simplify(
expr: sp.Expr,
_simplified: Dict[str, sp.Expr] = {},
_simplify: Callable = simplify,
) -> sp.Expr:
expr_str = repr(expr)
if expr_str not in _simplified:
_simplified[expr_str] = _simplify(expr)
dweindl marked this conversation as resolved.
Show resolved Hide resolved
return _simplified[expr_str]
self._simplify = cached_simplify
self._x0_fixedParameters_idx: Union[None, Sequence[int]]
self._w_recursion_depth: int = 0
self._has_quadratic_nllh: bool = True
Expand Down
15 changes: 14 additions & 1 deletion python/amici/pysb_import.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ def pysb2amici(
compute_conservation_laws: bool = True,
compile: bool = True,
simplify: Callable = lambda x: sp.powsimp(x, deep=True),
cache_simplify: bool = False,
generate_sensitivity_code: bool = True,
):
r"""
Expand Down Expand Up @@ -115,6 +116,9 @@ def pysb2amici(
:param simplify:
see :attr:`amici.ODEModel._simplify`

:param cache_simplify:
see :func:`amici.ODEModel.__init__`

:param generate_sensitivity_code:
if set to ``False``, code for sensitivity computation will not be
generated
Expand All @@ -134,6 +138,7 @@ def pysb2amici(
noise_distributions=noise_distributions,
compute_conservation_laws=compute_conservation_laws,
simplify=simplify,
cache_simplify=cache_simplify,
verbose=verbose,
)
exporter = ODEExporter(
Expand All @@ -160,6 +165,7 @@ def ode_model_from_pysb_importer(
noise_distributions: Optional[Dict[str, Union[str, Callable]]] = None,
compute_conservation_laws: bool = True,
simplify: Callable = sp.powsimp,
cache_simplify: bool = False,
verbose: Union[int, bool] = False,
) -> ODEModel:
"""
Expand Down Expand Up @@ -188,14 +194,21 @@ def ode_model_from_pysb_importer(
:param simplify:
see :attr:`amici.ODEModel._simplify`

:param cache_simplify:
see :func:`amici.ODEModel.__init__`

:param verbose: verbosity level for logging, True/False default to
:attr:`logging.DEBUG`/:attr:`logging.ERROR`

:return:
New ODEModel instance according to pysbModel
"""

ode = ODEModel(verbose=verbose, simplify=simplify)
ode = ODEModel(
verbose=verbose,
simplify=simplify,
cache_simplify=cache_simplify,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's known to fail for pysb models, I think we shouldn't offer it as an option, but rather set it to False and add a comment that it isn't compatible.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, now not sure. Just tried to debug the PySB issue by settings the default value in pysb2amici and ode_model_from_pysb_importer to True, but all tests in pytest test_pysb.py passed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also the PEtab pysb test suite? I am worried about the failure in e.g. https://github.com/AMICI-dev/AMICI/runs/5219969341?check_suite_focus=true. If caching silently leads to wrong model equations, this is a severe issue. Unless we have a good explanation of why that happened there and know that it won't happen again, I'd keep it out of there.

)

if constant_parameters is None:
constant_parameters = []
Expand Down
10 changes: 9 additions & 1 deletion python/amici/sbml_import.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,7 @@ def sbml2amici(self,
compile: bool = True,
compute_conservation_laws: bool = True,
simplify: Callable = lambda x: sp.powsimp(x, deep=True),
cache_simplify: bool = False,
log_as_log10: bool = True,
generate_sensitivity_code: bool = True,
**kwargs) -> None:
Expand Down Expand Up @@ -292,6 +293,9 @@ def sbml2amici(self,
:param simplify:
see :attr:`ODEModel._simplify`

:param cache_simplify:
see :func:`amici.ODEModel.__init__`

:param log_as_log10:
If ``True``, log in the SBML model will be parsed as ``log10``
(default), if ``False``, log will be parsed as natural logarithm
Expand Down Expand Up @@ -366,7 +370,11 @@ def sbml2amici(self,
self._clean_reserved_symbols()
self._process_time()

ode_model = ODEModel(verbose=verbose, simplify=simplify)
ode_model = ODEModel(
verbose=verbose,
simplify=simplify,
cache_simplify=cache_simplify,
)
ode_model.import_from_sbml_importer(
self, compute_cls=compute_conservation_laws)
exporter = ODEExporter(
Expand Down