Skip to content
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

convert with same type does not use the correct method #317

Closed
schillic opened this issue Aug 12, 2019 · 12 comments · Fixed by #567
Closed

convert with same type does not use the correct method #317

schillic opened this issue Aug 12, 2019 · 12 comments · Fixed by #567
Labels

Comments

@schillic
Copy link
Contributor

I noticed a recent problem when converting intervals.
The relevant definitions are here:

convert(::Type{Interval{T}}, x::Interval{T}) where {T} = x
convert(::Type{Interval{T}}, x::Interval) where {T} = atomic(Interval{T}, x)

To me they look correct, and I think until recently they worked as expected. In particular, the first method should fire if source and target type are identical. Below are the results in a fresh environment with Julia v1.2.0-rc2:

(v1.2) pkg> st
    Status `~/.julia/environments/v1.2/Project.toml`
  [d1acc4aa] IntervalArithmetic v0.16.0

julia> using IntervalArithmetic

julia> N = Float64; i = N(0) ± N(1)
[-1, 1]

julia> convert(typeof(i), i)
[-1, 1]

julia> @which convert(typeof(i), i)
convert(::Type{Interval{T}}, x::Interval) where T in IntervalArithmetic at .julia/packages/IntervalArithmetic/4e8KJ/src/intervals/conversion.jl:20

So the wrong method is used (the more general one). With Rationals this even crashes.

julia> N = Rational{Int}; i = N(0) ± N(1)
[-1//1, 1//1]

julia> convert(typeof(i), i)
ERROR: MethodError: no method matching atomic(::Type{Interval{Rational{Int64}}}, ::Interval{Rational{Int64}})

I realized this in a recent rebase of a pull request in Julia v1.0 and v1.1. The build was green before, and since this was after your latest release, the problem is probably not on your side. But I am out of ideas.
Note that this works:

julia> @which convert(Rational{Int}, 1//1)
convert(::Type{T}, x::T) where T<:Number in Base at number.jl:6
@lbenet
Copy link
Member

lbenet commented Aug 13, 2019

I reproduce both problems on Julia 1.1.1. The second problem is understandable (lack of a method)...

@schillic
Copy link
Contributor Author

The second problem is understandable (lack of a method)...

True, but it only goes there because it does not pick the correct method in line 19...

@dpsanders
Copy link
Member

This is a weird bug. I don't know why it's happening, but I have a fix in #320.

This should probably be reported as a Julia bug.

@schillic
Copy link
Contributor Author

Thanks a lot!
Since it happened all of a sudden, I suspect that some of your dependencies committed type piracy. I did not update my Julia version.

@dpsanders
Copy link
Member

Reported on JuliaLang here:
JuliaLang/julia#32884

@dpsanders
Copy link
Member

This happens with no other packages loaded, so I don't think any dependencies are involved?

@schillic
Copy link
Contributor Author

Right, it's just a mystery.

@dpsanders
Copy link
Member

Jeff answered in the above issue.

Did this actually cause any problems for you?

@schillic
Copy link
Contributor Author

schillic commented Aug 13, 2019

Yes, because of the Rationals. And it's also less efficient of course.

EDIT: Here is a simple example to show my problem:

julia> using IntervalArithmetic

julia> i = Interval(0//1 ± 1//1)
[-1//1, 1//1]

julia> [i, i]
ERROR: MethodError: no method matching atomic(::Type{Interval{Rational{Int64}}}, ::Interval{Rational{Int64}})
Closest candidates are:
  atomic(::Type{Interval{T<:AbstractFloat}}, ::Interval) where T<:AbstractFloat at /.julia/packages/IntervalArithmetic/1T5or/src/intervals/conversion.jl:123
  atomic(::Type{Interval{T<:AbstractFloat}}, ::S<:Real) where {T<:AbstractFloat, S<:Real} at /.julia/packages/IntervalArithmetic/1T5or/src/intervals/conversion.jl:96
  atomic(::Type{Interval{Rational{Int64}}}, ::Irrational) at /.julia/packages/IntervalArithmetic/1T5or/src/intervals/conversion.jl:133
  ...
Stacktrace:
 [1] convert(::Type{Interval{Rational{Int64}}}, ::Interval{Rational{Int64}}) at /.julia/packages/IntervalArithmetic/1T5or/src/intervals/conversion.jl:20
 [2] setindex!(::Array{Interval{Rational{Int64}},1}, ::Interval{Rational{Int64}}, ::Int64) at ./array.jl:767
 [3] vect(::Interval{Rational{Int64}}, ::Vararg{Interval{Rational{Int64}},N} where N) at ./array.jl:130
 [4] top-level scope at none:0

I still do not understand why this was no problem before, but the fix seems to be easy to do now.

@lbenet
Copy link
Member

lbenet commented Aug 13, 2019

So, should we restrict the types always?

@dpsanders
Copy link
Member

Maybe we should restrict the types, but that's a lot of code to work through...

@dpsanders
Copy link
Member

The Rational case works now after #320 . Thanks for the report!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants