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

Added test for using eltype(tspan)==Int and typeof(y0)=Int#96

Closed
mauro3 wants to merge 2 commits intoSciML:masterfrom
mauro3:m3/int-tspan
Closed

Added test for using eltype(tspan)==Int and typeof(y0)=Int#96
mauro3 wants to merge 2 commits intoSciML:masterfrom
mauro3:m3/int-tspan

Conversation

@mauro3
Copy link
Copy Markdown
Contributor

@mauro3 mauro3 commented May 25, 2016

We previously had problems with calls of the sort ode((t,y)->2t, 0., [0,1]) where tspan is a Vector{Int}. This adds a test to check that the solvers support this call. It showed that ode23s didn't and also fixes this.

Also fixed ode23s to allow for it.
@codecov-io
Copy link
Copy Markdown

codecov-io commented May 25, 2016

Current coverage is 93.96%

No coverage report found for master at a0ef97d.

Powered by Codecov. Last updated by a0ef97d...5999a8c

@coveralls
Copy link
Copy Markdown

coveralls commented May 25, 2016

Coverage Status

Coverage increased (+0.3%) to 93.968% when pulling d278c38 on mauro3:m3/int-tspan into a0ef97d on JuliaLang:master.

@coveralls
Copy link
Copy Markdown

coveralls commented May 25, 2016

Coverage Status

Coverage increased (+0.3%) to 93.968% when pulling d278c38 on mauro3:m3/int-tspan into a0ef97d on JuliaLang:master.

and fixed runge-kutta solvers to cope with it.
@coveralls
Copy link
Copy Markdown

coveralls commented Jun 20, 2016

Coverage Status

Coverage increased (+0.3%) to 93.968% when pulling 5999a8c on mauro3:m3/int-tspan into a0ef97d on JuliaLang:master.

@mauro3
Copy link
Copy Markdown
Contributor Author

mauro3 commented Jun 20, 2016

Added test and fix for using an integer as y0. See:
https://groups.google.com/d/msg/julia-users/Mn0OrZLGNkg/cCQE7UH3BwAJ

@mauro3 mauro3 changed the title Added test for using eltype(tspan)==Int Added test for using eltype(tspan)==Int and typeof(y0)=Int Jun 20, 2016
@pwl
Copy link
Copy Markdown

pwl commented Jun 21, 2016

Is Vector{Int} the only ugly type that we want to support? What about Vector{BigInt}, or a crazy mix like Number[1,1.1,BigInt(1)]? How far do we go with this functionality? Also I kind of agree that it would be nice to accept Vector{Int} along with other variations but the approach with make_consistent_types is never going to be type stable. Is there no other way to implement this? How about something along the lines of

ode{T<:AbstractFloat}(t0::AbstractVector{T})=print(t0)   # the actual implementation
ode(t0::AbstractVector)=f(float(t0))

so that instead of supporting t0::AbstractVector{Number} we would restrict the implementation to t0::AbstractVector{AbstractFloat}. I don't know if this makes any sense but I like it slightly better then make_consistent_types approach.

@mauro3
Copy link
Copy Markdown
Contributor Author

mauro3 commented Jun 21, 2016

I don't think it needs to be type stable if we use barrier-functions (which is not the case yet...). If the type on which the iterator is based on is constructed using leaftypes only then it should be fine and it doesn't matter that figuring out the leaftypes is done by an type-unstable function. We may want to draw the line at things like Number[1,1.1,BigInt(1)].

I think one of the points of having pure Julia implementations of ode-solvers is to be able to use them with any Julia type (as long as that type supports certain operations).

@ChrisRackauckas
Copy link
Copy Markdown
Member

As mauro3 noted, you don't need type-stability if you shield the main loop inside another function which is type-stable. However, since that's not currently done, there is a chance that this will cause everything to "Box" and maybe cause issues with type-inference (maybe, because sometimes the boxing isn't an issue). Did you benchmark and check the native code for Int and Float64 inputs at all?

However, I don't think hard-coding Float64 is ever a good idea. For example, I'm pretty sure this will throw an InexactError() if someone is trying to use BigFloats, ArbFloats, etc. That's not a problem that should addressed here since this is making ode23s consistent with the other functions, but it's something to think about as a future improvement.

@pwl
Copy link
Copy Markdown

pwl commented Sep 2, 2016

We decided not to go this way and we are now throwing an error if there are any conversions to be made by the solver: https://github.com/JuliaODE/ODE.jl/blob/dev/test/top-interface.jl#L78-L84. Any conversions that we come up with were simply too ambiguous and arbitrary, it's better if the user does them explicitly.

It's still a little early to do any benchmarks, but I have them in mind and we will do the optimization once we are happy with the current implementation of #49.

@ChrisRackauckas
Copy link
Copy Markdown
Member

Should this be closed then?

@pwl
Copy link
Copy Markdown

pwl commented Sep 2, 2016

I would say so.

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.

5 participants