-
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
Fix piecewise/Heaviside handling #2234
Conversation
* Substitute non-time-dependent Heavisides * Substitute inside the expanded expression, otherwise not all substitution targets are found Closes AMICI-dev#2231
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## develop #2234 +/- ##
============================================
+ Coverage 48.65% 76.64% +27.98%
============================================
Files 91 91
Lines 15109 15112 +3
============================================
+ Hits 7352 11583 +4231
+ Misses 7757 3529 -4228
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Okay, this might have been a more widespread problem - this also fixes frequent Brannmark_JBC2010 issues (i.e. breaks my example at https://amici.readthedocs.io/en/latest/example_errors.html#Steady-state-computation-failed :-|) |
Worth checking whether this change also affects the issues encountered in #1539 |
No. Brannmark_JBC2010 didn't have any stray Heavisides. Maybe just calling |
Unable to reproduce locally 🙄 |
python/sdist/amici/de_export.py
Outdated
@@ -2767,9 +2767,11 @@ def _process_heavisides( | |||
if heavisides: | |||
# only apply subs if necessary | |||
for heaviside_sympy, heaviside_amici in heavisides: | |||
dxdt = dxdt.subs(heaviside_sympy, heaviside_amici) | |||
dt_expanded = dt_expanded.subs( |
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.
was the issue that expand
led to manipulation of the trigger function which was then missed during this substitution?
Not super happy that we manipulate the rhs by applying expand here as I think we discussed at some point we want to keep simplification optional. I don't see why the expand is necessary here in the first place. Looks like this was introduced in #1352
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.
was the issue that
expand
led to manipulation of the trigger function which was then missed during this substitution?
Correct.
Not super happy that we manipulate the rhs by applying expand here as I think we discussed at some point we want to keep simplification optional. I don't see why the expand is necessary here in the first place. Looks like this was introduced in #1352
# expanding the rhs will in general help to collect the same
# heaviside function
So I guess, not expanding the expression may lead to more root functions to be tracked than strictly necessary in certain cases. I don't know how common this issue really is, and how much of a performance impact each trigger function has.
I am open to dropping expand
.
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.
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.
was the issue that
expand
led to manipulation of the trigger function which was then missed during this substitution?Correct.
Not super happy that we manipulate the rhs by applying expand here as I think we discussed at some point we want to keep simplification optional. I don't see why the expand is necessary here in the first place. Looks like this was introduced in #1352
# expanding the rhs will in general help to collect the same # heaviside function
So I guess, not expanding the expression may lead to more root functions to be tracked than strictly necessary in certain cases. I don't know how common this issue really is, and how much of a performance impact each trigger function has.
I am open to dropping
expand
.
But we are anyways calling sp.simplify
in _get_unique_root
. If expand really makes a difference, we could do sp.simplify(root_found.expand() - root.get_val())
, but I would be surprised if that's really the case. We can map multiple Heaviside functions to the same root finding function, so I would prefer keeping all symbolic manipulations in _get_unique_root
.
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.
If expand really makes a difference
I don't really have any evidence.
I find it hard to imagine though that we'd get a relevant performance hit from tracking e.g. 3 equivalent root functions instead of just one.
I'm fine with either. Just dropping the expand
requires fewest changes.
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.
Well, I don't think the expand
ever really improved anything in terms of reducing the number of root functions. Either it would simply have had no effect at all (i.e. the same roots would have been extracted with or without expand
), or it would have prevented replacing the Heavisides because the to-be-substituted subexpressions weren't found in the original expression. So I'd say it's safe to drop it completely.
This reverts commit fd3f076.
Fixes a bug where Heaviside functions weren't correctly substituted, potentially resulting in DiracDelta's in the model equations. The problem was, the substitution targets were collected from an expanded expression, but the actual substitution was attempted on the non-expanded expression, which did not always succeed.
Fixed by not expanding the expressions beforehand. Closes NaNs in model expressions with DiracDelta due to unfortunate term ordering #2231
Fixes redundant trigger-function processing