Skip to content
This repository has been archived by the owner on Sep 9, 2024. It is now read-only.

Add passing of parameters to ode23 #57

Closed
wants to merge 6 commits into from

Conversation

dpsanders
Copy link

This adds to ode23 the possibility to pass parameters through to the function to integrate, via parameter splatting. This is useful functionality; see, for example, this post on Julia-users.

I also updated (read: corrected) the documentation to use the Julia, not MATLAB, convention, and to not give InexactErrors.

The last commit updates the documentation to use the @doc style.

…the example, reduce the MATLAB-ness, add the new parameter functionality, comment that there are keyword arguments
…merical values so that the example code does not give InexactErrors. Removed spaces in docs of ode23
@coveralls
Copy link

Coverage Status

Coverage decreased (-97.09%) to 0.0% when pulling 0980c62 on dpsanders:ode23_with_params into 4a71fa0 on JuliaLang:master.

@acroy
Copy link
Contributor

acroy commented Mar 23, 2015

Thanks for the PR. Unfortunately, things are a bit complicated ... Firstly, ode23 will eventually be replaced by ode23_bs and we just kept it to keep track of regressions etc (see #16). Secondly, although we never discussed passing parameters to the rhs function (AFAIK, but maybe #30 contains something on this), the suggested solution is probably to use anonymous functions, i.e. in your new test

for α in 1.0:1.0:10.
    t,y=solver( (t,y)->-α*y, 1., [0:.001:1;])
end

should work already now for all solvers (maybe FastAnonymous.jl might be worth a look in this context). But maybe I missed what you were trying to achieve ...
Thirdly, if we would make this change we would have to do it for all solvers to keep the API more or less consistent.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.97%) to 96.12% when pulling d991e1d on dpsanders:ode23_with_params into 4a71fa0 on JuliaLang:master.

@dpsanders
Copy link
Author

Thanks for the comments (and apologies for the delay in replying).

I agree that anonymous functions provide a solution to the problem, but it seems strange if one already has a function, say SIR, to have to wrap it in another function and write
tout, yout = ode23( (t, x) -> SIR(t, x, a, b), x0, tspan)
instead of just
tout, yout = ode23(SIR, x0, tspan, a, b)

As you imply, this will also presumably involve a performance hit, though FastAnonymous.jl could indeed be a solution. I would like to do a performance comparison -- are there any particularly nasty ODEs I could test this on that you usually use?

For what it's worth, both MATLAB and scipy.integrate allow passing through parameters to the function (if I am not mistaken), and it seems a strange omission.

I am happy to go through and add parameter splatting to the other functions if this is the consensus -- I just thought that for my first PR I would be conservative!

Any thoughts on the documentation changes? Some of those are necessary (since the current documentation for ode23 seems to just be a copy of the MATLAB version and is actually wrong and misleading, as witnessed in the post in Julia-users referenced in the PR).

@mauro3
Copy link
Contributor

mauro3 commented Mar 25, 2015

Maybe you can use https://github.com/mauro3/IVPTestSuite.jl for performance testing. (But I guess you're aware of that package ;-)

@dpsanders
Copy link
Author

Great, thanks!

@pwl
Copy link

pwl commented Mar 25, 2015

For what my opinion is worth, I would prefer to keep using the anonymous functions to pass parameters. This way the implementation of ODE.jl stays free of clutter like params... and is more readable. It would be especially messy to add params to some more complex algorithms. That said, I have never investigated the performance hit.

@acroy
Copy link
Contributor

acroy commented Mar 25, 2015

Personally, I don't see a problem with anonymous functions (performance aside, but this will certainly improve). AFAIK MATLAB dropped passing parameters the way you suggest (at least there is noting in the current documentation) and Python might do it for performance reasons, but someone else has to comment on the reasons.

I would support a solution that is more general, i.e. in the direction of having a "problem" type like we discussed for instance in #30. Having this could obviously include support for passing extra parameters.

Any thoughts on the documentation changes?

I like the changes and in particular the move to Docile. As I said above, ode23 will likely be removed though.

@mauro3
Copy link
Contributor

mauro3 commented Mar 25, 2015

Regarding the issue. I prefer an anonymous function solution. This is also what matlab does. I'm not sure this pull request will have much impact on performance as passing functions to functions is already bad. Hopefully, this will be fixed in Julia soon! Until then FastAnonymous.jl probably is fastest, faster than passing in the function.

@tomasaschan
Copy link
Contributor

Yeah, I think anonymous functions is much cleaner too (I actually think the ODEProblem-type solution is kind-of clunky, but I guess that's a matter of taste.)

Performance-wise, this is not something we should worry about in ODE.jl - Julia as a whole will be crippled compared to its ambitions until this is resolved, so I would be very surprised if this isn't fixed way before 1.0.

@stevengj
Copy link
Contributor

The problem with anonymous functions is that you can only do one thing with them (call them). If the user defines an ODEProblem type, this is more flexible because you can define multiple methods on that type to provide function values, Jacobians, etcetera. That is why it is good to have type-based low-level interface. (You can still have a high-level interface based on passing simple functions for the simple cases.)

@acroy acroy mentioned this pull request Mar 29, 2015
@acroy
Copy link
Contributor

acroy commented Apr 8, 2015

@dpsanders : Now we have removed the old implementation of ode23. If you are still up for it, a PR for changing the documentation to Docile would be great.

@acroy acroy closed this Apr 8, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants