cvodes reloaded

[Sebastian Weber]

Hi there!

As discussed I removed old issues and old branches. The fresh and new ones are

https://github.com/stan-dev/math/issues/293

and this goes with the feature/issue-293-refactor-integrate_ode_bdf branch. I haven't created a pull yet, could be early.

So I also went ahead and effectively pulled out of the cvodes_integrator the constructor to setup CVODES and the integrate_times functionality. What was left I named cvodes_data. I hope this was in line with the discussion we had.

What bothers me a little bit is that I have to pass y0 and theta into the cvodes_data object merely for the purpose of doing the function overloading to get to the right rhs_sens implementation, but I am not sure if there is a better way given the design principle to avoid if/then/else over using the types here.

Tests do break at the moment as the cvodes_integrator is still being tested, but not anymore existing.

Best,
Sebastian

[Sebastian Weber]

I just noticed that I forgot to also pull the destruction of CVODES memory into the function.

Moreover, we now need to wrap the integration for loop into a try statement as exceptions may be thrown during integration and deallocation of CVODES is not anymore conveniently handled by a destructor.

I hope that additional try-block wont impact performance.

Sebastian

[Bob Carpenter]

It's not that expensive to have try/catch.

Having said that, where it makes sense, I prefer
the constructor/destructor-based memory management
of the C++ RAII pattern. I haven't been following this
discussion closely, so excuse me if this is well trodden
ground---there are always competing concerns to balance.

- Bob

> --
> You received this message because you are subscribed to the Google Groups "stan development mailing list" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to stan-dev+u...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[Sebastian Weber]

Hi!

I have now added the respective try/catch scheme and I make sure that CVODES is taken down appropriately. Tests break as the cvodes_integrator tests are still around which we may use to test the cvodes_data object (or can we declare that one which is in cvodes_utils.hpp as not test-worthy?).

As I understood, the reasoning from Michael and Daniel was that it would be easier to manage the integrator if most of the work were done inside the integrate_ode_bdf function as opposed to create an object. The argument for it was that this would be easier to test and moreover, the cvodes_integrator could almost do its work inside the constructor as the integrate function actually did not any longer take any arguments at all. I am not sure if RAII was equated into it; it wasn't on my radar at that point.

Nonetheless, for the requirement of cvodes for the static callbacks we still need to have a cvodes_data object around which is the point of interaction of CVODES code with the ODE itself.

Should we start the pull now? Or what would be good to move forward now?

Sebastian

[Bob Carpenter]

We're trying to restrict pull requests to branches which
are code complete (no remaining to-do items).

- Bob

[Sebastian Weber]

Well, that's why I am asking... so I wait for now as the tests are not yet in place and we should settle on design and tests first then.

I have to say, the current design decisions are above my head. There are some things in the pull which I do care about, but these appear to be accepted. So to proceed I rely on you guys who have a more holistic view of the Stan design principles.

Sebastian

On Thursday, May 12, 2016 at 5:34:10 PM UTC+2, Bob Carpenter wrote:
> We're trying to restrict pull requests to branches which
> are code complete (no remaining to-do items).
>
> - Bob
>
> > On May 11, 2016, at 4:18 PM,
> >