Handle events occuring at fixed timepoints without root-finding #2227
Conversation
A first attempt towards AMICI-dev#2185 For events that occur at known timepoints, we don't need sundials' root-finding. We can just stop the solver at the respective timepoints and handle the events. To be extended to parameterized but state-independent trigger functions.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #2227 +/- ##
===========================================
+ Coverage 76.50% 76.66% +0.15%
===========================================
Files 91 91
Lines 15041 15109 +68
===========================================
+ Hits 11507 11583 +76
+ Misses 3534 3526 -8
Flags with carried forward coverage won't be shown. Click here to find out more.
|
python/sdist/amici/de_model.py
Outdated
|
|
||
| def triggers_at_fixed_timepoint(self) -> bool: | ||
| """Check whether the event triggers at a (single) fixed time-point.""" | ||
| t_root = sp.solve(self.get_val(), amici_time_symbol) |
There was a problem hiding this comment.
Does it make sense to cache the value of t_root at initialization? get_trigger_time alone computes this twice. Would also reduce some redundancy.
There was a problem hiding this comment.
Can be done. I don't think computing it twice matters performance-wise, but the redundancy is not so nice, agreed.
src/forwardproblem.cpp
Outdated
| if (is_next_t_too_close(t_, next_t_out)) { | ||
| // next timepoint is too close to current timepoint | ||
| // update `t_`, required by `handleDataPoint` | ||
| t_ = next_t_out; |
There was a problem hiding this comment.
isnt the value of t_ always overwritten by solver->writeSolution?
There was a problem hiding this comment.
Correct. But solver->writeSolution is only called for the next timepoint once handleDataPoint finishes. By then it's fine that it gets overwritten.
There was a problem hiding this comment.
That‘s not evident from the code here though
| discs_.push_back(t_); | ||
|
|
||
| /* extract and store which events occurred */ |
There was a problem hiding this comment.
I lifted that up into ForwardProblem::workForwardProblem. This is the only place where handleEvent is called with !seflag && !initial_event. We need it out there, because we need to modify roots_found_ after calling getRootInfo.
There was a problem hiding this comment.
I might change that to just directly passing the timepoint to handleDataPoint, I think that's clearer. (EDIT: oops, that belongs to #2227 (comment))
A first attempt towards #2185
For events that occur at known timepoints, we don't need sundials' root-finding. We can just stop the solver at the respective timepoints and handle the events.
Here, events are sorted such that the
ne_solverevents that require root-finding by the solver come first and the otherne - ne_solverevents come after that. The solver only tracksne_solverroots.To be extended to parameterized but state-independent trigger functions at some point.