Skip to content

Adaptive timestepping in high-level interface? #75

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

Closed
ChrisRackauckas opened this issue Aug 20, 2016 · 12 comments
Closed

Adaptive timestepping in high-level interface? #75

ChrisRackauckas opened this issue Aug 20, 2016 · 12 comments

Comments

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Aug 20, 2016

Hey, I am in the process of wrapping some of Sundials into DifferentialEquations.jl and was wondering how you access CVode's adaptive timestepping with the high level interface here. Is it possible? Is there a work around that's needed?

Also, when doing adaptive, where do you find out what times it solved at?

@acroy
Copy link
Contributor

acroy commented Aug 22, 2016

What do you mean by "access CVode's adaptive timestepping"? You can set relative and absolute tolerances and Sundials is choosing the appropriate time-steps. AFAIK you can't access the internal times/results though ...

@ChrisRackauckas
Copy link
Member Author

I found out how to do it. I meant that I wanted to get all the internal steps and solutions. I will be putting the PR to access all this shortly (a few minutes?)

@acroy
Copy link
Contributor

acroy commented Aug 22, 2016

Great! This would be useful to have for the simplified interface (also in view of #10).

@alyst
Copy link
Contributor

alyst commented Aug 31, 2016

As was discussed a little bit in #67, it might be worth to improve API introduced by c4f46b2.

I would suggest to merge cvode_fulloutput() into cvode():

  • add collect_times keyword argument to cvode() that could be either :specified (the default behaviour AKA CV_NORMAL) or :all (the _fulloutput behaviour AKE CV_ONE_STEP)
  • add container keyword argument that could be either :matrix (the current behaviour, works well with collect_times=:specified) or :vectors (_fulloutput behaviour)
  • always return a tuple: timepoints vector plus ODE solution in a matrix or Vector{Vector{Float64}} form. This change would be backward-incompatible, but easy to fix in the user code and similar to how the other ODE APIs do that.

Since #67 would be merged rather soon and it would require the new minor tag, it would be nice to solve this API issue before tagging.

@ChrisRackauckas do you want to prepare the PR along these lines? (once #67 is merged)

@ChrisRackauckas
Copy link
Member Author

Sure, that sounds like a reasonable way to handle the issues.

@acroy
Copy link
Contributor

acroy commented Aug 31, 2016

I am not so fond of changing the output type with a keyword. It's not type stable and could lead to performance degradations (depending on what one does of course). I know that the Vector{Vector} output can be inconvenient or even slow ... We discussed this at length for ODE.jl and there are good arguments why we introduced it like this. So either we keep compatibility with ODE.jl and always return vectors of vectors or we always return a matrix. (Another option would be to use different types to control the output and make use of multiple dispatch).

@alyst
Copy link
Contributor

alyst commented Aug 31, 2016

Yes, that's my major concern as well. OTOH, type stability is not a big problem here, because normally cvode() would not be used as a kernel function (by design it would never be as efficient for being called multiple times as it is possible). And one can always use type assertions to fix the returned type.

The other alternative would be cvode!(output, ...) method that fills (and resizes when needed) the output var as so depends on its input type.

@ChrisRackauckas
Copy link
Member Author

Vector{Vector} never really turns out that slow. It can be slower to read, but in the tests I've done it's a good saving format. But type-stability would be something to worry about.

The in-place with dispatch method could be nice. It would allow for cvode to be in-place which would be a small improvement in the first place (small since I don't know how often people will re-call cvode with the same vector, but at least it's an option). Another way to do it with dispatch is to instead use a Symbol in the type signature, for example f{:Matrix}(...) and dispatch on the symbols. I would be against it since those less familiar with Julia might not find it as easy to understand, but it's an option.

But the kwarg for :specified and :all seems uncontroversial, and so does always returning the tuple. Is that correct?

@ChrisRackauckas
Copy link
Member Author

We should probably handle #40 at the same time too. Add a callback function as a kwarg which defaults to identity? I could put in an example showing how to make it plug into the Juno progressbar (which is what I plan to do with DifferentialEquations.jl anyways), but the same kind of thing would make it work with ProgressMeter.jl.

@acroy
Copy link
Contributor

acroy commented Sep 2, 2016

I agree, a kwarg for :specified and :all is uncontroversial. And I like the idea of having cvode!(output, ...). Given the comment of @ChrisRackauckas above I would vote for Vector{Vector} as the default for cvode (and please don't forget idasol). This would break old code, but it would be easy to fix.

The call-back issue #40 should IMO be handled separately (even if it is only a small PR).

@ChrisRackauckas
Copy link
Member Author

Can I get a review from someone? #87. I think that supporting Vector{Vector} and Matrix output like this (in a type-stable manner) bloats the code a bit too much. Matrix output also doesn't work as well with the in-place version. Lastly, I won't be using the Matrix output in DifferentialEquations.jl anyways, so unless you see an easier way to support it here, I vote for just doing Vector{Vector}.

I would also like to change the output to a specialized output type. I like this option more since it allows for more output while not bloating the user burden (most might just use t and y from the output, additionally dy from idasol output, but more could exist). Specifically, I would like to save mem for dense output interpolations among other things, and would like to handle the retcode better.

Then again, I am seeing that I might need a much more advanced interface anyways for DifferentialEquations.jl, so if you want something type-free I am good and I'll be forking it off to something else.

Also, I just found out about ONE_STEP_TSTOP so that will clean up some of the code, and I'll get to idasol soon.

@ChrisRackauckas
Copy link
Member Author

Satisfied by the common interface.

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

No branches or pull requests

3 participants