Add keywords minstep, maxstep and initstep to ode23s and oderkf

[acroy]

As the title says this PR adds the keywords minstep, maxstep and initstep to ode23s and oderkf as suggested in the API docs. The initial step estimator is actually due to @berceanu (I hope you are Ok with me taking it from #43; otherwise just let me know). The other commits are just minor cosmetic changes.

[coveralls]

Coverage decreased (-1.03%) to 96.05% when pulling b2cc50e on acroy:steps into 4a71fa0 on JuliaLang:master.

[acroy]

Anyone?

[tomasaschan]

The indentation in this if statement and the next makes it more difficult to understand the control flow here, IMO.

[tomasaschan]

Sorry. I took a look at the code and commented on a few nitpicks, but I haven't tried to run anything (I don't work with related stuff anymore, so I can do very little except look at the test results).

The CI build seems fine, as far as ODE.jl code is related - Coveralls doesn't seem to be working on 0.4, which means the build script fails there, but all the tests pass on both 0.3 and 0.4. No idea why Github didn't get a notification that the build was finished, but it's probably not your fault :)

[tomasaschan]

And as far as the intent of the PR is concerned, I'm all for it :)

[pwl]

Regarding the initial step choice, I also think you should replace the arbitrary constants (1e-5, 1e-15 etc.) with eps(...). For example, you could replace 1e-5 with eps(...)^(1/3). Although, I have no idea how the algorithm works and which constants should depend on the available precision.

By the way, if we want to have generic solvers, shouldn't we replace the numbers in Butcher tables with rationals?

[tomasaschan]

@pwl regarding rationals: yes, we should. I might do a quick PR if you don't want to :)

[acroy]

I think the constants in hinit are just empirical values, which have proven to work well (probably for double precision computations). We can try to replace them, but it might be problematic.

[tomasaschan]

Ah. Well, that does make it a little less obvious what to do, but I still think there's merit in trying. For most cases, it's quite obvious if the constant could chosen due to its relation to eps(Float64) or not (e.g. always splitting the time domain into 100 pieces is probably not related to floating point precision, while 1e-5 ~ cbrt(eps(Float64)) might be). I think we could try to use eps() whenever there's a straightforward way to get to the same value from it, and use convert() on the literal in all other cases to avoid promotion.

This might be better handled under #60 though, so I don't think it's necessary to change that before this PR can be merged. (Please do fix the indentation, though :) )

[dpsanders]

Just a quick comment -- does anyone solve ODEs with rationals? (I guess not...) eps doesn't make sense in that context. If we're only talking about floating point, then putting the constants in terms of eps sounds like a good solution.

[tomasaschan]

@acroy Are you using some IDE to do the indentation for you? There seems to be whitespace changes in a whole bunch of irrelevant places now, and the indentation is still off in many places (e.g. in hinit it seems that some rows have a tab size of 8 spaces and some (the if statements) have a tab size of 4)...

[acroy]

@tlycken I am using Atom as an editor. I thought that I might have (accidentally) converted the tabs to spaces at some point. Strange.

[coveralls]

Coverage decreased (-2.0%) to 95.09% when pulling 0ae6410 on acroy:steps into 4a71fa0 on JuliaLang:master.

[coveralls]

Coverage decreased (-2.0%) to 95.09% when pulling 146fc3d on acroy:steps into 4a71fa0 on JuliaLang:master.

[coveralls]

Coverage decreased (-2.0%) to 95.09% when pulling 146fc3d on acroy:steps into 4a71fa0 on JuliaLang:master.

[coveralls]

Coverage decreased (-2.0%) to 95.09% when pulling 31b1568 on acroy:steps into 4a71fa0 on JuliaLang:master.

[acroy]

Now it should be better. I also changed hinit to return the value of F at the initial time, which saves us one evaluation.

[coveralls]

Coverage decreased (-2.0%) to 95.09% when pulling c1641b8 on acroy:steps into 4a71fa0 on JuliaLang:master.

[tomasaschan]

Yeah, this LGTM.

[acroy]

Should we use the opportunity to replace ode23 by ode23_bs? I haven't updated the former since we wanted to replace it anyways.

[ViralBShah]

Perhaps pick a better name, if possible?

[acroy]

You mean _bs doesn't sound serious? :-) However, we would only use it internally and still export the name ode23 (which calls ode23_bs) like we do it for ode45 (which calls ode45_dp).

[ViralBShah]

I think it sounds too serious, but is difficult for a newcomer to decipher, if it was a public api. :-) Internal API, of course, is ok.

[tomasaschan]

I'd say it's time, but merge them separately to make it easier to revert one but not the other, should we need to.

[coveralls]

Coverage decreased (-2.0%) to 95.09% when pulling a1e3d17 on acroy:steps into 4a71fa0 on JuliaLang:master.

[acroy]

Alright, let's do it like that.