Fix ode gradients with respect to t0 (Issue #1833) #1834

bbbales2 · 2020-04-13T13:19:26Z

Tests

There were tests in place, but either:

They were testing against forced_harm_osc_ode_fun which actually had zero gradients
Or they were testing that the gradients were zero

So not setting the gradients was accidentally making these things pass. I changed the tests to use harm_osc_ode_fun where one of the gradients isn't zero, and I updated the checks to look for that.

Release notes

Fixed problem where ode solvers were not computing the gradients of the output with respect to the initial integration time. This affected the rk45, adams, and bdf solvers.

Checklist

Math issue ODE gradients with respect to t0 not working correctly #1833
Copyright holder: Columbia University

The copyright holder is typically you or your assignee, such as a university or company. By submitting this pull request, the copyright holder is agreeing to the license the submitted work under the following licenses:
- Code: BSD 3-clause (https://opensource.org/licenses/BSD-3-Clause)
- Documentation: CC-BY 4.0 (https://creativecommons.org/licenses/by/4.0/)
the basic tests are passing
- unit tests pass (to run, use: ./runTests.py test/unit)
- header checks pass, (make test-headers)
- dependencies checks pass, (make test-math-dependencies)
- docs build, (make doxygen)
- code passes the built in C++ standards checks (make cpplint)
the code is written in idiomatic C++ and changes are documented in the doxygen
the new changes are tested

wds15 · 2020-04-13T13:21:07Z

Thanks for catching this! I can review this later.

bbbales2 · 2020-04-13T13:22:07Z

@wds15, @yizhang-yiz, if the tests don't just pass on the first try, we've probably missed this release, but if you get a chance to review this today that'd be nice.

rok-cesnovar · 2020-04-13T13:23:07Z

This is a bugfix so it doesnt fall under the feature freeze. Still better to have it in the release candidate.

…erly (Issue #1833)

wds15

Looks like we need to sort out the sign thing. In any case, many thanks for catching this. My bad.

wds15 · 2020-04-13T16:43:15Z

stan/math/prim/functor/coupled_ode_observer.hpp

@@ -133,6 +141,10 @@ struct coupled_ode_observer {
        }
      }

+      if (!is_constant_all<T_t0>::value) {
+        ops_partials.edge3_.partials_[0] = -f_y0_t0_[j];


Why would this be equal to minus f_y0_t0_[j]? As I understand, the initial time-point is no different than the remaining time-points such that the derivative wrt to t0 is just f_y0_t0. I mean, in this version we would have at t0 the gradient equal to -f_y0_t0_[j] and just an epsilon later the sign would flip if the user would request at t0+epsilon a solution of the ODE.

So please change to f_y0_t0_[j] unless I am overlooking something here.

I am also not too happy with the variable name as this is called dy_dt usually. Maybe initial_dy_dt_ ?

Why would this be equal to minus f_y0_t0_[j]?

Imagine an ode solve with only an initial point and a final point. As an integral, if the final point moves right, the value of the integral gets larger. If the initial point moves right, that integral gets smaller (or the thing here: https://en.wikipedia.org/wiki/Leibniz_integral_rule).

as this is called dy_dt usually

Yeah I was working through making the odes variadic I never found naming that I was super happy with.

dy_dt is just f, by definition, so it's annoying to type the fully dy_dt. I kinda liked f_y0_t0 cause it's evaluated at the initial points (still really painful to type).

I'm happy to go initial_dy_dt. Doesn't matter that much to me.

but at t0+epsilon the gradient would flip sign. That is not right. Either we have a bigger problem (and the gradients are wrong for the other time-points as well) or this PR is wrong, but I don't think that both are right.

As you are suggesting that your version is the right one, I would need to to dive into it a bit.

Let's keep this as a bugfix which we can merge during this week.

but at t0+epsilon the gradient would flip sign.

The derivative with respect to the lower limit of the integral will still have that negative sign. Write out the integral for something like cos(t) from t = l to t = r and then differentiate that integral with respect to l and r and see the difference.

There's finite difference code here if you want to check with that: #1833 . I wrote this out to make sure I wasn't too off track.

Sure... I just need a calm moment tomorrow to write this down. It's against my intuition, so I won't approve for just now.

If someone else is confident this is right, then I withdraw my review.

(I guess you are right, I just want to follow along).

Yeah no it's fine it's confusing lol. Tomorrow is good for this.

Got it. Confusing, but you are right. The odd thing is that the initial is constant here while the initial time varies.

Can you add the test catching the size mis match thing below? Then this is fine to go in.

wds15 · 2020-04-13T16:44:00Z

test/unit/math/rev/functor/coupled_ode_observer_test.cpp

@@ -221,7 +221,7 @@ TEST_F(StanRevOde, observe_states_ddvd) {
      EXPECT_FLOAT_EQ(ys_coupled[t][n], y[t][n].val());
    for (size_t n = 0; n < 2; n++) {
      y[t][n].grad();
-      EXPECT_FLOAT_EQ(0.0, t0.adj());
+      EXPECT_FLOAT_EQ(-y0[n], t0.adj());


here as well, the test expectation is y0[n], I think.

wds15 · 2020-04-13T16:45:18Z

stan/math/prim/functor/coupled_ode_observer.hpp

+    if (!is_constant_all<T_t0>::value) {
+      f_y0_t0_
+          = f(value_of(t0), value_of(y0), value_of(theta_), x_, x_int_, msgs_);
+      check_size_match("coupled_ode_observer", "dy_dt", f_y0_t0_.size(),


Good catch - we need the check here. Is this check triggered somewhere in the tests?

No, but I can add that.

Checks added!

stan-buildbot · 2020-04-13T19:42:23Z

Name	Old Result	New Result	Ratio	Performance change( 1 - new / old )
gp_pois_regr/gp_pois_regr.stan	4.8	4.84	0.99	-0.78% slower
low_dim_corr_gauss/low_dim_corr_gauss.stan	0.02	0.02	0.98	-2.27% slower
eight_schools/eight_schools.stan	0.09	0.09	1.0	0.48% faster
gp_regr/gp_regr.stan	0.22	0.22	1.0	-0.18% slower
irt_2pl/irt_2pl.stan	6.47	6.48	1.0	-0.14% slower
performance.compilation	89.48	87.17	1.03	2.58% faster
low_dim_gauss_mix_collapse/low_dim_gauss_mix_collapse.stan	7.52	7.53	1.0	-0.13% slower
pkpd/one_comp_mm_elim_abs.stan	21.33	20.33	1.05	4.68% faster
sir/sir.stan	93.68	94.34	0.99	-0.7% slower
gp_regr/gen_gp_data.stan	0.05	0.05	0.99	-1.23% slower
low_dim_gauss_mix/low_dim_gauss_mix.stan	2.95	2.95	1.0	-0.03% slower
pkpd/sim_one_comp_mm_elim_abs.stan	0.31	0.3	1.01	0.93% faster
arK/arK.stan	1.74	1.74	1.0	0.11% faster
arma/arma.stan	0.66	0.65	1.01	1.39% faster
garch/garch.stan	0.51	0.51	1.01	0.57% faster
Mean result: 1.00378766533

Jenkins Console Log
Blue Ocean
Commit hash: e92cc41

Machine information

ProductName: Mac OS X ProductVersion: 10.11.6 BuildVersion: 15G22010

CPU:
Intel(R) Xeon(R) CPU E5-1680 v2 @ 3.00GHz

G++:
Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

Clang:
Apple LLVM version 7.0.2 (clang-700.1.81)
Target: x86_64-apple-darwin15.6.0
Thread model: posix

… size as input state (Issue #1833)

bbbales2 and others added 2 commits April 13, 2020 09:14

Fix ode gradients with respect to t0 (Issue #1833)

57d00b9

[Jenkins] auto-formatting by clang-format version 6.0.0

f9eed28

Updated coupled_ode_observer_test to test initial time gradients prop…

e92cc41

…erly (Issue #1833)

wds15 requested changes Apr 13, 2020

View reviewed changes

bbbales2 and others added 3 commits April 14, 2020 10:02

Added tests to make sure function returning state derivatives of same…

e14f70d

… size as input state (Issue #1833)

Updated naming in ode observer (Issue #1833)

4e50636

[Jenkins] auto-formatting by clang-format version 6.0.0

dcc6321

wds15 approved these changes Apr 15, 2020

View reviewed changes

wds15 merged commit cbcc81d into develop Apr 15, 2020

SteveBronder mentioned this pull request Apr 16, 2020

Stan Math 3.2 release #1826

Closed

bbbales2 mentioned this pull request Apr 20, 2020

Stanc3 release for Cmdstan 2.23 stan-dev/stanc3#498

Closed

bbbales2 mentioned this pull request May 8, 2021

Feature/adjoint odes #1905

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ode gradients with respect to t0 (Issue #1833) #1834

Fix ode gradients with respect to t0 (Issue #1833) #1834

bbbales2 commented Apr 13, 2020

wds15 commented Apr 13, 2020

bbbales2 commented Apr 13, 2020

rok-cesnovar commented Apr 13, 2020

wds15 left a comment

wds15 Apr 13, 2020

bbbales2 Apr 13, 2020

wds15 Apr 13, 2020

bbbales2 Apr 13, 2020

wds15 Apr 13, 2020

bbbales2 Apr 13, 2020

wds15 Apr 14, 2020

wds15 Apr 13, 2020

wds15 Apr 13, 2020

bbbales2 Apr 13, 2020

bbbales2 Apr 14, 2020

stan-buildbot commented Apr 13, 2020

Fix ode gradients with respect to t0 (Issue #1833) #1834

Fix ode gradients with respect to t0 (Issue #1833) #1834

Conversation

bbbales2 commented Apr 13, 2020

Tests

Release notes

Checklist

wds15 commented Apr 13, 2020

bbbales2 commented Apr 13, 2020

rok-cesnovar commented Apr 13, 2020

wds15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stan-buildbot commented Apr 13, 2020