Fix sensitivities for models with events / extend and cleanup event tests #2084

dweindl · 2023-05-11T19:32:18Z

Fixes incorrect sensitivities for models with events with state-dependent trigger functions.

Add three new event test cases and do some cleanup.

Closes #2086

Three new event test cases and some cleanup. To reduce changes in #1539 Co-authored-by: Paul Stapor <paul.stapor@helmholtz-muenchen.de>

codecov · 2023-05-11T19:54:50Z

Codecov Report

Merging #2084 (ef6cf15) into develop (438812d) will not change coverage.
The diff coverage is 0.00%.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #2084   +/-   ##
========================================
  Coverage    49.44%   49.44%           
========================================
  Files           32       32           
  Lines         5497     5497           
========================================
  Hits          2718     2718           
  Misses        2779     2779

Flag	Coverage Δ
petab	`49.44% <0.00%> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
python/sdist/amici/de_export.py	`78.41% <0.00%> (ø)`

dweindl · 2023-05-12T06:55:02Z

There seems to be some issue with forward sensitivities for python/tests/test_events.py::test_models[piecewise_plus_event_simple_case].

Only for d_x1/d_alpha, rest is okay.

FFroehlich

🙇

FFroehlich · 2023-05-12T12:26:33Z

There seems to be some issue with forward sensitivities for python/tests/test_events.py::test_models[piecewise_plus_event_simple_case].

Only for d_x1/d_alpha, rest is okay.

Oi! This is bad, will try to have a look next week!

stephanmg · 2023-05-12T12:29:44Z

There seems to be some issue with forward sensitivities for python/tests/test_events.py::test_models[piecewise_plus_event_simple_case].
Only for d_x1/d_alpha, rest is okay.

Oi! This is bad, will try to have a look next week!

True. Will also have a look, we should then sync up perhaps.

This reverts commit 4381ac3.

dweindl · 2023-05-16T05:38:42Z

python/tests/test_events.py

+def model_definition_piecewise_plus_event_semi_complicated():
+    """Test model for boolean operations in a piecewise condition, discrete
+    events and a non-vanishing quadrature for the adjoint state.
+    """


Currently fails finite differences check.

Passes with

diff --git a/python/tests/util.py b/python/tests/util.py index d5907da29..e85160a98 100644 --- a/python/tests/util.py +++ b/python/tests/util.py @@ -154,4 +154,4 @@ def check_trajectories_with_forward_sensitivities( solver.setRelativeToleranceFSA(1e-13) rdata = runAmiciSimulation(amici_model, solver=solver) _check_close(rdata["x"], result_expected_x, field="x", rtol=1e-10, atol=1e-12) - _check_close(rdata["sx"], result_expected_sx, field="sx", rtol=1e-10, atol=1e-9) + _check_close(rdata["sx"], result_expected_sx, field="sx", rtol=1e-07, atol=1e-9)

Seems a bit loose, but then again, we are comparing to FD...

FFroehlich · 2023-05-16T09:46:55Z

python/sdist/amici/de_export.py

@@ -2024,7 +2024,7 @@ def _compute_equation(self, name: str) -> None:
                        # This part only works if we use self.eq('xdot')
                        # instead of self.sym('xdot'). Not immediately clear
                        # why that is.
-                        tmp_dxdp += smart_multiply(self.eq("xdot"), self.sym("stau").T)
+                        tmp_dxdp += smart_multiply(self.eq("xdot_old"), self.sym("stau").T)


This does seem plausible. Would argue though that self.sym("xdot_old") is more intuitive (should be the same, no?).

But the fact that this makes a difference does point to a more conceptual problem. If xdot needs to be evaluated before the event (which kinda makes sense), the same should also apply to ddeltaxdt, ddeltaxdx and ddeltaxdp. Probably best to add to derived state, add respective methods to model and compute these quantities pre-event. Probably worthwhile to come up with a testcase where this applies beforehand. Should be straightforward to do, use current testcase and change deltax such that ddeltaxdt, ddeltaxdx and ddeltaxdp have a dependency on x, i.e., are different pre- and post-event.

Would argue though that self.sym("xdot_old") is more intuitive (should be the same, no?).

Yes, I already changed that.

Thanks for looking into that. I will merge this PR, as it clearly improves things already, and keep your suggestions for another one.

PS: Some of these cases are already covered:

python/tests/test_events.py::test_models[events_plus_heavisides] ddeltaxdp [Matrix([ [0, 0, 0, 0, 0, 0, 0, 0, 0], [0, 0, 0, 0, 0, 0, 0, 0, 0], [0, 0, 0, 0, 0, 0, 0, 0, 0]]), Matrix([ [0, 0, 0, 0, 0, 0, 0, 0, 0], [0, 0, 0, 0, 0, 0, 0, 0, 0], [0, 0, 0, 0, 0, 0, 0, 0, 1/3]]), Matrix([ [0, 0, 0, 0, 0, 0, 0, 0, 0], [0, 0, 0, 0, 0, 0, 0, 0, 0], [0, 0, 0, 0, 0, 0, 0, 0, 0]]), Matrix([ [0, 0, 0, 0, 0, 0, 0, 0, 0], [0, 0, 0, 0, 0, 0, 0, 0, 0], [0, 0, 0, 0, 0, 0, 0, 0, 0]]), Matrix([ [0, 0, 0, 0, 0, 0, 0, 0, 0], [0, 0, 0, 0, 0, 0, 0, 0, 0], [0, 0, 0, 0, 0, 0, 0, 0, 0]])] ddeltaxdx [Matrix([ [0, 0, -1/2], [0, 0, 0], [0, 0, 0]]), Matrix([ [0, 0, 0], [0, 0, 0], [0, 0, 0]]), Matrix([ [0, 0, 0], [0, 0, 0], [0, 0, 0]]), Matrix([ [0, 0, 0], [0, 0, 0], [0, 0, 0]]), Matrix([ [0, 0, 0], [0, 0, 0], [0, 0, 0]])] python/tests/test_events.py::test_models[piecewise_plus_event_simple_case] ddeltaxdp [Matrix([[0, 0, 1, 0]]), Matrix([[0, 0, 0, 0]]), Matrix([[0, 0, 0, 0]]), Matrix([[0, 0, 0, 0]])] ddeltaxdx [Matrix([[-1]]), Matrix([[0]]), Matrix([[0]]), Matrix([[0]])] python/tests/test_events.py::test_models[piecewise_plus_event_semi_complicated] ddeltaxdp [Matrix([ [0, 0, 1, 0, 0, 0, 0], [0, 0, 0, 0, 0, 0, 0]]), Matrix([ [0, 0, 0, 0, 0, 0, 0], [0, 0, 0, 0, 0, 0, 0]]), Matrix([ [0, 0, 0, 0, 0, 0, 0], [0, 0, 0, 0, 0, 0, 0]]), Matrix([ [0, 0, 0, 0, 0, 0, 0], [0, 0, 0, 0, 0, 0, 0]]), Matrix([ [0, 0, 0, 0, 0, 0, 0], [0, 0, 0, 0, 0, 0, 0]])] ddeltaxdx [Matrix([ [-1, 0], [ 0, 0]]), Matrix([ [0, 1], [0, 0]]), Matrix([ [0, 0], [0, 0]]), Matrix([ [0, 0], [0, 0]]), Matrix([ [0, 0], [0, 0]])]

Only non-zero ddeltaxdt seems to be missing.

Ah, they are non-zero, but not dependent on x. Noted.

Extend / cleanup event tests

69027d3

Three new event test cases and some cleanup. To reduce changes in #1539 Co-authored-by: Paul Stapor <paul.stapor@helmholtz-muenchen.de>

dweindl requested a review from a team as a code owner May 11, 2023 19:32

dweindl self-assigned this May 11, 2023

typo

f08b3f1

dweindl marked this pull request as draft May 12, 2023 06:56

FFroehlich approved these changes May 12, 2023

View reviewed changes

dweindl added 2 commits May 12, 2023 11:21

Merge branch 'develop' into event_test

9b51ab5

windows

0589237

Merge remote-tracking branch 'origin/develop' into event_test

9424ef6

dweindl added this to the v0.18.0 milestone May 15, 2023

dweindl mentioned this pull request May 15, 2023

Incorrect forward sensitivities in model with events #2086

Closed

dweindl added 5 commits May 15, 2023 16:54

fix?

4381ac3

Revert "fix?"

d9d41bd

This reverts commit 4381ac3.

fix?

06b5d7b

sym vs eq?

97baddc

Why test twice?

4ab8813

dweindl commented May 16, 2023

View reviewed changes

dweindl assigned FFroehlich May 16, 2023

Merge branch 'develop' into event_test

e0e28fb

dweindl marked this pull request as ready for review May 16, 2023 06:44

FFroehlich reviewed May 16, 2023

View reviewed changes

dweindl changed the title ~~Extend / cleanup event tests~~ Fix sensitivities for models with events / extend and cleanup event tests May 16, 2023

increase tolerance

f9c197b

dweindl mentioned this pull request May 16, 2023

Add tests for models with events with state-dependent ddeltaxd{t,p,x} #2089

Closed

1 task

dweindl enabled auto-merge May 16, 2023 10:14

Merge branch 'develop' into event_test

ef6cf15

dweindl disabled auto-merge May 16, 2023 12:16

dweindl merged commit a5398dd into develop May 16, 2023

dweindl deleted the event_test branch May 16, 2023 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sensitivities for models with events / extend and cleanup event tests #2084

Fix sensitivities for models with events / extend and cleanup event tests #2084

dweindl commented May 11, 2023 •

edited

Loading

codecov bot commented May 11, 2023 •

edited

Loading

dweindl commented May 12, 2023 •

edited

Loading

FFroehlich left a comment

FFroehlich commented May 12, 2023 •

edited

Loading

stephanmg commented May 12, 2023

dweindl May 16, 2023

FFroehlich May 16, 2023

dweindl May 16, 2023

dweindl May 16, 2023

dweindl May 16, 2023

dweindl May 16, 2023

Fix sensitivities for models with events / extend and cleanup event tests #2084

Fix sensitivities for models with events / extend and cleanup event tests #2084

Conversation

dweindl commented May 11, 2023 • edited Loading

codecov bot commented May 11, 2023 • edited Loading

Codecov Report

dweindl commented May 12, 2023 • edited Loading

FFroehlich left a comment

Choose a reason for hiding this comment

FFroehlich commented May 12, 2023 • edited Loading

stephanmg commented May 12, 2023

dweindl May 16, 2023

Choose a reason for hiding this comment

FFroehlich May 16, 2023

Choose a reason for hiding this comment

dweindl May 16, 2023

Choose a reason for hiding this comment

dweindl May 16, 2023

Choose a reason for hiding this comment

dweindl May 16, 2023

Choose a reason for hiding this comment

dweindl May 16, 2023

Choose a reason for hiding this comment

dweindl commented May 11, 2023 •

edited

Loading

codecov bot commented May 11, 2023 •

edited

Loading

dweindl commented May 12, 2023 •

edited

Loading

FFroehlich commented May 12, 2023 •

edited

Loading