-
Notifications
You must be signed in to change notification settings - Fork 30
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.
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
Handle events occuring at fixed timepoints without root-finding #2227
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isnt the value of t_
always overwritten by solver->writeSolution
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_solver
events that require root-finding by the solver come first and the otherne - ne_solver
events come after that. The solver only tracksne_solver
roots.To be extended to parameterized but state-independent trigger functions at some point.