Variadic ODE design doc (design-doc #19) #20

bbbales2 · 2020-04-20T17:01:23Z

Rendered version here

There's a bit of code here that has implemented most of this design: stan-dev/math#1641

bbbales2 · 2020-04-20T17:04:00Z

@wds15, @yizhang-yiz, @charlesm93, @bob-carpenter -- whoever wants to comment or review, have at it. I'd like to get something like this through in the release 3 months from now.

wds15 · 2020-04-20T17:45:25Z

Really cool to see that much action from you on this! I will take a deep dive on this soon.

bbbales2 · 2020-04-29T19:39:23Z

@wds15 I'm bored and haven't coded stuff in a long time. Gimme something to do.

wds15 · 2020-04-29T20:03:48Z

Sorry for the long delay...

Do we have benchmarks new vs old stuff?

wds15

This is great to see this move forward.

A few points for consideration. Let me know what you think.

designs/0019-variadic_odes.md

wds15 · 2020-04-30T07:41:47Z

designs/0019-variadic_odes.md

+[guide-level-explanation]: #guide-level-explanation
+
+For the bdf solver, I am proposing introducing two new functions: `ode_bdf` and
+`ode_tol_bdf` (there will be two functions similarly introduced for the rk45 and


I am not too happy with the name ode_bdf. I mean, you get the solution of the ODE. The only thing which comes to my mind right now is solve_ode_bdf, but I am not entirely happy with that either. If you like that - great - if not, maybe someone else has a better idea or we live with it.

Another solution is integrate_ode_bdf_2, for example? Maybe this is the best thing to do? This is kind of consistent with what we plan to do for other functions as well if I am not wrong here.

Doesn't bother me. We're just getting into crazy long name territory. MATLAB has super short names like ode45 etc.

I think integrate/solve are a bit extraneous. @bob-carpenter I keep calling you in on naming things. What do you think here?

ode_bdf, solve_ode_bdf, integrate_ode_bdf_2? They're all fine to me.

Having @bob-carpenter‘s view on this would be nice, indeed

designs/0019-variadic_odes.md

wds15 · 2020-04-30T07:52:43Z

designs/0019-variadic_odes.md

+  side (`f`). Can be any non-function type.
+
+There is no way to optionally only provide some of the `rel_tol`, `abs_tol`, or
+`max_num_steps` arguments. We can keep the current defaults (`1e-6`, `1e-6`,


I thought our current defaults are much tighter. While I think that we could relax them to 1E-6 as you suggest, we should probably stay consistent with the integrate* functions, no? Or is this a good way to get to more sane defaults? Not sure on this.

Looks like they are inconsistent:

https://github.com/stan-dev/math/blob/develop/stan/math/rev/functor/integrate_ode_bdf.hpp

https://github.com/stan-dev/math/blob/develop/stan/math/prim/functor/integrate_ode_rk45.hpp

https://mc-stan.org/docs/2_23/stan-users-guide/control-parameters-for-ode-solving.html

wds15 · 2020-04-30T07:57:14Z

designs/0019-variadic_odes.md

+(with no rules on naming).
+2. The state is written in terms of `vector`s instead of real arrays. ODE states
+are mathematically written and expressed as vectors, so given the interface is
+changing we might as well change this to be consistent.


Hmm... non-linear ODEs are never written as matrix-vector products. Not sure if this a good reason. As I said, the output is like a multi-variate container (array of vectors) which is the better reason to me to switch to arrays of vectors.s

Good point ODEs are usually defined element by element. But they are still written as elements of vectors.

wds15 · 2020-04-30T07:59:31Z

designs/0019-variadic_odes.md

+we'd make it a vector argument (so it wasn't like one argument of `ode_bdf`
+needs to be a vector and one doesn't). However, then it seems like the output
+of `ode_bdf` would be a matrix. I think the output makes more sense as an array
+of vectors.


Fine for me, but a different reason though (maybe add my aspect to this?).

Which aspect?

My thought was that in Stan the array of vector thing is the natural container for multi-variate stuff... and the algebra solver uses the same conventions.

wds15 · 2020-04-30T08:01:51Z

designs/0019-variadic_odes.md

+that could otherwise be passed to through as a right hand side argument here).
+
+I'm not opposed to this either, and this also isn't incompatible with the
+current proposal, though it would probably require some extra work.


Na, we should not wait here. This is great stuff and will make things easier for users. It may actually make things faster as fewer copies will be needed due to avoiding the unpacking step in the RHS. Moreover, we also avoid that unpacked data will be cast to a var within the ODE RHS, which is probably what happens at the moment for more complicated models.

wds15 · 2020-04-30T08:04:56Z

designs/0019-variadic_odes.md

+2. ```y0``` - Initial state of the ode solve (`y0 = y(t0)`)
+3. ```t0``` - Initial time of the ode solve
+4. ```times``` - Sequences of times to which the ode will be solved (each
+  element must be greater than t0).


In the current implementation, the given times must be a strictly increasing vector. It would be far more convenient if the times vector must only be an increasing vector, but adjacent times are allowed to be repeated, so this should be legal for times:

1, 2, 3.2, 3.2, 4

This has caused quite some trouble for users - including myself. Can we allow that?

Good suggestion. Yes we can change this. Should we allow t = 0.0 to be in the times as well? I don't see why not.

Sure, lets allow that as well.

I ended up not allowing for t = t0 because I got confused thinking about how it would interact with t0/y0.

bbbales2 · 2020-04-30T18:07:39Z

Do we have benchmarks new vs old stuff?

I do not. I'm just assuming this interface can be fast enough, and then we can check it before it merges that it indeed is.

bbbales2 · 2020-05-14T18:23:40Z

Eek, was there something that you wanted me to write up here (said something about it in the meeting)?

I just started writing code. I guess things I've done are:

Converted to doing state as an Eigen::VectorXd
Written a bunch of docs and added more tests
Fixed the t0 issue again (hopefully for good this time)
And now I've started adding in the adjoint stuff in, which isn't really part of this design doc, but I'm doing it in the ODE thread. We might break that up into separate things -- I kinda just saw the opportunity and started.

wds15 · 2020-05-14T20:00:44Z

I was expecting to hear back so that we start exploring those additional tolerance control things for example. We still need to decide on what options to control in addition of any.

Btw, as we seem to switch to general template arguments you can do that here now as well which will probably make the code more memory efficient since some copies can be avoided, I think.

bbbales2 · 2020-05-14T22:18:40Z

I was expecting to hear back so that we start exploring those additional tolerance control things for example

Oh yeah, working through the interface now. We'll go back and forth on that once I get a baseline up.

wds15 · 2020-05-27T07:19:54Z

Just as a note: The just released sundial 5.3 includes in the release notes: "Our latest release, v5.3.0, adds support to CVODE for integrating IVPs with contstraints and projecting the solution onto the constraint manifold"

Sounds cool.

https://computing.llnl.gov/projects/sundials/sundials-software

totally not for this design per se, but could be a consideration should we want to move into this direction in the future.

bbbales2 · 2020-05-27T10:40:51Z

Oh interesting. That could be an easier way to handle positivity constraints.

But then we couldn't easily expose that functionality to the other solvers.

bbbales2 · 2020-06-01T20:16:35Z

I think the basic code and such is in place to evaluate this and start wrapping everything up.

There are three outstanding questions I know of (it's possible more get added in the course of review), but let me tag some people and see if we can get these out of the way.

The writeup is here.

We should decide on default tolerances. The docs are inconsistent now anyway. Currently CVODES solvers run at rtol = 1e-10, atol = 1e-10 and the RK45 solver runs at rtol = 1e-6, atol = 1e-6.
We need to decide on the error control for CVODES (whether or not to include sensitivity variables in error control, see here)
Naming. I originally did short names like ode_bdf for convenience. @wds15 likes integrate_ode_bdf_2 (details). Either one will do. What seems better from a user perspective?

@wds15 @charlesm93 @yizhang-yiz @bob-carpenter

wds15 · 2020-06-05T14:45:04Z

Is it an option to drop for the new interfaces the functions which set default tolerances? I do think that people should by all means set the tolerances.
I hope my benchmarks will be done next week and give some help to decide.
I am fine with short names. So ode_bdf is great for me...just like matlab

bbbales2 · 2020-06-22T16:12:12Z

@wds15 let's keep default tolerances so-as to not rock the boat. I'll copy them over and fix the doc to reflect the difference if nobody has any other suggestions.

I also like the different names since the interface is changing slightly (vector initial conditions + tolerances in different places). The new solvers aren't just a drop in substitute for the old solvers (though that is handled via the backwards compatibility integrate_ode_bdf etc. interfaces here).

bbbales2 · 2020-07-06T18:41:52Z

I've done BDF/Adams defaults as rtol = 1e-10, atol = 1e-10 and RK45 solver defaults at rtol = 1e-6, atol = 1e-6.
I've configured CVODES (BDF/Adams) to adjust timestep based on tolerance requirements of the sensitivity equations.
Short names are good.

wds15

Just some minor cleanup and then we are good.

designs/0019-variadic_odes.md

wds15 · 2020-07-07T08:41:54Z

designs/0019-variadic_odes.md

+depend on.
+
+Argument packing is inconvenient, makes code hard to read, and introduces more
+chances for making errors.


Maybe add as advantage that the requirement for packing leads easily to over-propagation of packed data to properly named variables which are cast to vars which is avoided with the new variadic interface.

Can't conceptually we put all data in the x_r arguments so there's no promotion?

Edit: I say conceptually just cause maybe it's really annoying but at least it's a possibility?

designs/0019-variadic_odes.md

…ity problem (design-doc #19)

bbbales2 · 2020-07-08T00:17:59Z

@wds15 did everything but the promotion. We have answers for all the unanswered questions, so I just cleaned those up.

wds15

Looks great.

…gn-doc #19)

rok-cesnovar · 2020-07-27T11:27:01Z

Should we now approve/merge this? Everything proposed here now lives in Stan, as of 2.24

wds15 · 2020-07-27T13:20:20Z

I would suggest to merge, yes.

rok-cesnovar · 2020-07-27T13:24:05Z

@wds15 want to approve? You have done most of the reviewing.

wds15

LGTM

wds15 · 2020-07-27T13:25:10Z

I just approved once more. Not sure why this is still roadblocked.

rok-cesnovar · 2020-07-27T13:26:08Z

You seem to not have approving rights (gray tick on approval), let me try.

rok-cesnovar

LGTM

rok-cesnovar · 2020-07-27T13:27:05Z

Someone with proper permissions will have to do it.

mitzimorris · 2020-07-27T14:39:04Z

maintainers only have read access. people with write access are BB, Carp, Steve Bronder, and D Lee. an odd set of folks. Steve might have been given write access in order to manage the CoC issue which isn't really a design doc.
choices: grant write access to all maintainers; get approval from BB, Steve, Daniel, or Bob.

Added initial variadic ODE design doc draft (design-doc #19)

0dff058

bbbales2 requested review from wds15, yizhang-yiz and charlesm93 April 20, 2020 17:02

wds15 requested changes Apr 30, 2020

View reviewed changes

bbbales2 mentioned this pull request May 7, 2020

Variadic argument lists for ODEs stan-dev/math#1641

Merged

5 tasks

bbbales2 added 2 commits May 29, 2020 14:05

Cleaning up ODE design doc (design-doc #19)

db634e1

Updated todo section for ODE design doc (#19)

25fa255

bbbales2 changed the title ~~Added initial variadic ODE design doc draft (design-doc #19)~~ ODE design doc draft (design-doc #19) Jul 6, 2020

bbbales2 changed the title ~~ODE design doc draft (design-doc #19)~~ Variadic ODE design doc (design-doc #19) Jul 6, 2020

wds15 requested changes Jul 7, 2020

View reviewed changes

bbbales2 added 2 commits July 7, 2020 19:57

Responding to review comments (design-doc #19)

760bd0f

Added comment on why timesteps adjust based on tolerances of sensitiv…

3695251

…ity problem (design-doc #19)

wds15 previously approved these changes Jul 10, 2020

View reviewed changes

Removed statement that said defaults are not decided (they are) (desi…

2a52530

…gn-doc #19)

bbbales2 dismissed wds15’s stale review via 2a52530 July 13, 2020 21:29

bbbales2 added 2 commits July 13, 2020 17:33

Updated defaults for BDF/Adams solvers (design-doc #19)

e8de2bf

A few cleanups (design-doc #19)

87f276c

wds15 approved these changes Jul 27, 2020

View reviewed changes

rok-cesnovar approved these changes Jul 27, 2020

View reviewed changes

rok-cesnovar merged commit e76066f into master Jul 28, 2020

Variadic ODE design doc (design-doc #19) #20

Variadic ODE design doc (design-doc #19) #20

Conversation

bbbales2 commented Apr 20, 2020 • edited Loading

bbbales2 commented Apr 20, 2020

wds15 commented Apr 20, 2020

bbbales2 commented Apr 29, 2020

wds15 commented Apr 29, 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

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

bbbales2 commented Apr 30, 2020

bbbales2 commented May 14, 2020

wds15 commented May 14, 2020

bbbales2 commented May 14, 2020

wds15 commented May 27, 2020

bbbales2 commented May 27, 2020

bbbales2 commented Jun 1, 2020

wds15 commented Jun 5, 2020

bbbales2 commented Jun 22, 2020

bbbales2 commented Jul 6, 2020

wds15 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbbales2 Jul 7, 2020 • edited Loading

Choose a reason for hiding this comment

bbbales2 commented Jul 8, 2020

wds15 left a comment

Choose a reason for hiding this comment

rok-cesnovar commented Jul 27, 2020

wds15 commented Jul 27, 2020

rok-cesnovar commented Jul 27, 2020

wds15 left a comment

Choose a reason for hiding this comment

wds15 commented Jul 27, 2020

rok-cesnovar commented Jul 27, 2020

rok-cesnovar left a comment

Choose a reason for hiding this comment

rok-cesnovar commented Jul 27, 2020

mitzimorris commented Jul 27, 2020

bbbales2 commented Apr 20, 2020 •

edited

Loading

bbbales2 Jul 7, 2020 •

edited

Loading