search

AMICI-dev / AMICI

Advanced Multilanguage Interface to CVODES and IDAS
https://amici.readthedocs.io/
Other
109 stars 31 forks source link

Failing Performance Test #1062

Closed FFroehlich closed 4 years ago

FFroehlich commented 4 years ago

https://github.com/ICB-DCM/AMICI/runs/623848607?check_suite_focus=true is exceeding target time for model import by ~20% after an increase in import time by 20-40%. Not sure whether we are really happy with that. Looking at the logs it looks like dwdp and dwdx are the likely culprits, although I am also somewhat surprised by the time required to construct w.

Looks like the substitution in https://github.com/ICB-DCM/AMICI/blob/390305f98c96ea505cf52884991774e7dfefdeff/python/amici/ode_export.py#L1641 takes over two minutes.

Are we fine with this (i.e. we increase the time limit for that test) or do we want to bring that down?

paulstapor commented 4 years ago

Overall, I would be happy to bring that down more. Just to ask this one again: What argues against using ImmutableSparseMatrix instead of ImmutableDenseMatrix in sympy for those quantities? I would hope, that this reduces cpu time in some operations... Moreover, if no assignment rules or rate rules are used, dxdotdp_explicit should always be 0. Also this takes 10 minutes overall... However, I don't know how much we want to tailor general routines to special cases by using shortcut in the code...

dweindl commented 4 years ago

Are we fine with this (i.e. we increase the time limit for that test) or do we want to bring that down?

If there are good ideas to speed things up, that would be the way to go. If not, I guess we can live with it... (another point to look at in #851).

FFroehlich commented 4 years ago

Overall, I would be happy to bring that down more. Just to ask this one again: What argues against using ImmutableSparseMatrix instead of ImmutableDenseMatrix in sympy for those quantities? I would hope, that this reduces cpu time in some operations...

I think we just never explored using sparse matrices, not sure what the overhead for construction/conversion and impact on multiplication time is.

Moreover, if no assignment rules or rate rules are used, dxdotdp_explicit should always be 0. Also this takes 10 minutes overall... However, I don't know how much we want to tailor general routines to special cases by using shortcut in the code...

maybe add check in _derivative to check _free_vars for overlap with differentiation variables before computing jacobian and if None, just generate zero matrix? I agree, would avoid adding more special cases and rather find good/better general solutions.

FFroehlich commented 4 years ago

Are we fine with this (i.e. we increase the time limit for that test) or do we want to bring that down?

If there are good ideas to speed things up, that would be the way to go. If not, I guess we can live with it... (another point to look at in #851).

Not sure, specifically for the handling of constant species I am no longer convinced that implementing them as conservation laws is the right approach, as this comes with a lot of overhead I did not anticipate. Might be better to directly convert them into expressions, but have to think a bit more about correctness of this approach with reinitialization.