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: method isn't called even though atsign-which finds one #8818

Closed
timholy opened this issue Oct 26, 2014 · 14 comments
Closed

convert: method isn't called even though atsign-which finds one #8818

timholy opened this issue Oct 26, 2014 · 14 comments
Labels
bug Indicates an unexpected problem or unintended behavior

Comments

@timholy
Copy link
Member

timholy commented Oct 26, 2014

This could be the same problem as #8631, but now I can trigger it even after pinning Color at 0.3.9 (so it clearly has nothing to do with new Dict syntax).

To debug, I modified these lines to generate some println statements so I know the call chain:

convert{T<:Real,N}(::Type{Array{T}}, img::AbstractImageDirect{T,N}) = convert(Array{T,N}, img)
function convert{T<:ColorType,N}(::Type{Array{T}}, img::AbstractImageDirect{T,N})
    println("here 2")
    convert(Array{T,N}, img)
end
function convert{T,N}(::Type{Array{T,N}}, img::AbstractImageDirect{T,N})
    assert2d(img)  # only well-defined in 2d
    p = permutation_canonical(img)
    dat = convert(Array{T}, data(img))
    if issorted(p)
        return dat
    else
        return permutedims(dat, p)
    end
end
function convert(::Type{Array}, img::AbstractImage)
    println("here 1")
    convert(Array{eltype(img)}, img)
end

Now:

julia> using Images, TestImages

julia> img = testimage("mandrill")
RGB Image with:
  data: 512x512 Array{RGB{UfixedBase{Uint8,8}},2}
  properties:
    IMcs: sRGB
    spatialorder:  x y
    pixelspacing:  1 1

julia> A = convert(Array, img)
ERROR: `convert` has no method matching convert(::Type{Array{T,N}}, ::Image{RGB{UfixedBase{Uint8,8}},2,Array{RGB{UfixedBase{Uint8,8}},2}})
 in convert at base.jl:13

julia> @which convert(Array, img)
convert(::Type{Array{T,N}},img::AbstractImage{T,N}) at /home/tim/.julia/v0.3/Images/src/core.jl:227

So you can see it never calls the method found by @which (it never prints out "here 1").

julia> versioninfo()
Julia Version 0.3.2-pre+36
Commit 275afc8* (2014-10-02 03:42 UTC)
Platform Info:
  System: Linux (x86_64-linux-gnu)
  CPU: Intel(R) Core(TM) i7 CPU       L 640  @ 2.13GHz
  WORD_SIZE: 64
  BLAS: libopenblas (USE64BITINT DYNAMIC_ARCH NO_AFFINITY Nehalem)
  LAPACK: libopenblas
  LIBM: libopenlibm
  LLVM: libLLVM-3.3
@timholy
Copy link
Member Author

timholy commented Oct 26, 2014

I just verified that this behavior is present in julia 0.3.0 as well as the latest julia 0.3.3-pre. I swear this worked at one point (you can even see it in the Images README), so I bet it's a bug triggered by a change in a package. I'm going to start going through older versions of Color, since that seems to have triggered other convert problems.

@timholy
Copy link
Member Author

timholy commented Oct 26, 2014

Here's a more minimal test case:

  • In ~/.julia/v0.3/Color, say git checkout 034db53a7c6dcf532d667c91a097d7b84c985ef1
  • Implement the following:
diff --git a/src/conversions.jl b/src/conversions.jl
index 8c0d2d7..ff0f2c9 100644
--- a/src/conversions.jl
+++ b/src/conversions.jl
@@ -688,6 +688,10 @@ convert{C,T}(::Type{AlphaColorValue{C,T}}, c::AlphaColorValue{C,T}) = c
 function convert{C,T,D,U}(::Type{AlphaColorValue{C,T}}, c::AbstractAlphaColorValue{D,U})
     AlphaColorValue{C,T}(convert(C, c.c), c.alpha)
 end
+function convert{D,T}(AC::TypeConstructor, c::AbstractAlphaColorValue{D,T})
+    AlphaColorValue(convert(colortype(AC), c.c), c.alpha)
+end
+
 convert{C,T}(::Type{AlphaColorValue{C,T}}, c::ColorValue) =
     AlphaColorValue{C,T}(convert(C, c), one(T))
  • Now test like this:
using Images, TestImages
img = testimage("mandrill")
A = convert(Array, img)

Without that diff, it works fine.

IIRC, the motivation for the TypeConstructor convert method is to support convert(RGBA, c), where RGBA is a typealias.

@JeffBezanson
Copy link
Member

I'll look at this. But, there should definitely not be definitions for TypeConstructor; that doesn't really make sense. {T<:AlphaColorValue}(::Type{T}) might work instead. I don't know if this is the source of the problem though.

@timholy
Copy link
Member Author

timholy commented Oct 26, 2014

That specific change doesn't work: tests fail on convert(RGBA, color("red")). Playing around a bit, I didn't find another variant that works on Julia 0.3, either.

However, it's clearly not the only problem with convert, as commenting out that method does not fix JuliaAttic/Color.jl#68.

@aviks
Copy link
Member

aviks commented Nov 1, 2014

Using released Color 0.3.10:

./julia
               _
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "help()" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.3.3-pre+18 (2014-10-28 09:26 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit bdd4597* (4 days old release-0.3)
|__/                   |  x86_64-apple-darwin13.4.0

julia> using Color

julia> convert(Vector{Symbol}, {"a","b"})
ERROR: `convert` has no method matching convert(::Type{Array{Symbol,1}}, ::Array{Any,1})
 in convert at base.jl:13

julia> 

Executing the convert method before the import makes the call work even after the import.

./julia
               _
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "help()" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.3.3-pre+18 (2014-10-28 09:26 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit bdd4597* (4 days old release-0.3)
|__/                   |  x86_64-apple-darwin13.4.0

julia> convert(Vector{Symbol}, {"a","b"})
2-element Array{Symbol,1}:
 :a
 :b

julia> using Color

julia> convert(Vector{Symbol}, {"a","b"})
2-element Array{Symbol,1}:
 :a
 :b

This bug effectively prevents many packages (eg GLM) from working in conjunction with Gadfly. Quite a few of the bugs in packages are linked to #8631 , but given @JeffBezanson 's comment there about 0.4 breakage, I'm uncertain if they are related.

@timholy
Copy link
Member Author

timholy commented Nov 2, 2014

"convertalypse" is quite troublesome.

I hope to be able to look at it myself starting Nov 6, but no promises about whether I'll make any progress. In the meantime, I'd definitely recommend experimenting with pinning Color at earlier versions.

@aviks
Copy link
Member

aviks commented Nov 2, 2014

"convertalypse" indeed!

Pinning Color to 0.3.9 does make the issue go away.

./julia
               _
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "help()" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.3.3-pre+18 (2014-10-28 09:26 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit bdd4597* (4 days old release-0.3)
|__/                   |  x86_64-apple-darwin13.4.0

julia> using Color

julia> convert(Vector{Symbol}, {"a","b"})
2-element Array{Symbol,1}:
 :a
 :b

Unfortunately, something else in Gadfly causes this to recur. So the issue is not isolated to Color. :(

./julia
               _
   _       _ _(_)_     |  A fresh approach to technical computing
  (_)     | (_) (_)    |  Documentation: http://docs.julialang.org
   _ _   _| |_  __ _   |  Type "help()" for help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 0.3.3-pre+18 (2014-10-28 09:26 UTC)
 _/ |\__'_|_|_|\__'_|  |  Commit bdd4597* (4 days old release-0.3)
|__/                   |  x86_64-apple-darwin13.4.0

julia> using Gadfly
Warning: could not import Base.has into Gadfly
Warning: could not import StatsBase.bandwidth into Stat
Warning: could not import StatsBase.kde into Stat

julia> convert(Vector{Symbol}, {"a","b"})
ERROR: `convert` has no method matching convert(::Type{Array{Symbol,1}}, ::Array{Any,1})
 in convert at base.jl:13

@timholy
Copy link
Member Author

timholy commented Nov 3, 2014

What if you pin to 0.3.6?

@aviks
Copy link
Member

aviks commented Nov 3, 2014

I recompiled my julia install (as part of trying to git bisect) and now cannot replicate this issue after a using Color even with Color 0.3.10 . However it reliably fails when using Gadfly.

The good news is, with Color pinned to 0.3.6, using Gadfly no longer produces a error with convert

@timholy
Copy link
Member Author

timholy commented Nov 3, 2014

I just completed a bisect for a similar problem over at #8631. I'm not certain the two are related, which is why there are two issues, but one can always hope...

@aviks
Copy link
Member

aviks commented Nov 5, 2014

@timholy Another report that pinning to 0.3.6 fixes it. JuliaStats/MixedModels.jl#26

@timholy
Copy link
Member Author

timholy commented Nov 7, 2014

OK, now I know a lot more about this. First, this bug doesn't apply to master, so it's 0.3-specific. Second, the commit that triggered this is e3c5c11. Third, my attempts to fix this (e.g., make it more like the function in master) failed, and I don't understand inference.jl, so I may have to ask @carnaval or @JeffBezanson for advice or help.

A simple test script:

import Base.convert

convert(E::TypeConstructor, e) = error("oops")

immutable Container{T}
    data::Array{T}
end

function convert(::Type{Array}, c::Container)
    println("Here")
    c.data
end

A = Container([5])

and then, at the REPL, do this:

include("<name_of_test_script>")
convert(Array, A)

(I wasn't able to trigger the bug if it's run from a script.)

timholy added a commit to JuliaAttic/Color.jl that referenced this issue Nov 7, 2014
This should fix numerous related errors in other packages
@timholy
Copy link
Member Author

timholy commented Nov 7, 2014

Pkg.free("Color"); Pkg.update() should give you a version of Color that works around this problem.

@vtjnash
Copy link
Member

vtjnash commented Jul 20, 2016

should be fixed now

@vtjnash vtjnash closed this as completed Jul 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Indicates an unexpected problem or unintended behavior
Projects
None yet
Development

No branches or pull requests

5 participants