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.

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

NaNs in model expressions with DiracDelta due to unfortunate term ordering #2231

Closed
dweindl opened this issue Dec 12, 2023 · 2 comments · Fixed by #2234
Closed

NaNs in model expressions with DiracDelta due to unfortunate term ordering #2231

dweindl opened this issue Dec 12, 2023 · 2 comments · Fixed by #2234

Comments

@dweindl
Copy link
Member

dweindl commented Dec 12, 2023

Encountered the following problem with https://github.com/Benchmarking-Initiative/Benchmark-Models-PEtab/tree/d7c52f90b15ca83a3d3f832f08989f1448fbd01e/Benchmark-Models/Bertozzi_PNAS2020:

Some expressions in dwdp that involve DiracDeltas may become NaN when evaluated for DiracDelta(0). DiracDelta(0) is currently implemented as DBL_MAX, not infinity. Therefore, a * DiracDelta(0) is finite for a in [-1, 1].
For the Bertozzi_PNAS2020 model, we have something like a * DiracDelta(0) * b, which is non-finite if a is outside [-1, 1], even if a * b would be in [-1, 1] (in this specific case, a*b == 0). Reordering the multiplicands fixes that problem.

Not sure how to handle that robustly. In this case, just moving the DiracDelta as far to the right as possible fixes the issue. However, in other situations it might be beneficial if a DiracDelta(x != 0) occurs further left.

Any thoughts?

@FFroehlich
Copy link
Member

How do we end up with Dirac deltas in dwdp in the first place? Sounds like something with event parsing went awry. Having briefly looked at the model it looks like it's coming from those piecewise functions, which should be processed by

def _process_heavisides(

@dweindl
Copy link
Member Author

dweindl commented Dec 12, 2023

Thanks for the pointer. I think I found the issue.

dweindl added a commit to dweindl/AMICI that referenced this issue Dec 12, 2023
* Substitute non-time-dependent Heavisides
* Substitute inside the expanded expression, otherwise not all substitution targets are found

Closes AMICI-dev#2231
@dweindl dweindl linked a pull request Dec 12, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants