JuliaImages / Images.jl

An image library for Julia
http://juliaimages.org/
Other
533 stars 141 forks source link

Cannot `display` OffsetArray of small image (<1000 pixels) in IJulia #650

Closed mprat closed 4 years ago

mprat commented 7 years ago

Minimum working example (in IJulia):

using TestImages, OffsetArrays, CoordinateTransformations;
const warp = ImageTransformations.warp_new
img = testimage("lighthouse");
imgr = imresize(img, (50, 50));
# works fine
display(imgr)

tfm = Translation(50);
img0 = warp(imgr, tfm);
# throws error
display(img0)

The last display call throws the error:

size not supported for arrays with indices (-49:0, -49:0);

With the stacktrace:

Stacktrace:
 [1] errmsg(::OffsetArrays.OffsetArray{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2,Array{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2}}) at /home/mprat/.julia/v0.6/OffsetArrays/src/OffsetArrays.jl:48
 [2] _repeat at ./abstractarraymath.jl:394 [inlined]
 [3] #repeat#117 at ./abstractarraymath.jl:366 [inlined]
 [4] (::Base.#kw##repeat)(::Array{Any,1}, ::Base.#repeat, ::OffsetArrays.OffsetArray{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2,Array{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2}}) at ./<missing>:0
 [5] #show#97(::Int64, ::Int64, ::Function, ::Function, ::IOContext{Base64EncodePipe}, ::MIME{Symbol("image/png")}, ::OffsetArrays.OffsetArray{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2,Array{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2}}) at /home/mprat/.julia/v0.6/Images/src/showmime.jl:33
 [6] limitstringmime(::MIME{Symbol("image/png")}, ::OffsetArrays.OffsetArray{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2,Array{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2}}) at /home/mprat/.julia/v0.6/IJulia/src/inline.jl:32
 [7] display_dict(::OffsetArrays.OffsetArray{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2,Array{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2}}) at /home/mprat/.julia/v0.6/IJulia/src/execute_request.jl:32
 [8] display(::IJulia.InlineDisplay, ::OffsetArrays.OffsetArray{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2,Array{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2}}) at /home/mprat/.julia/v0.6/IJulia/src/inline.jl:76
 [9] display(::OffsetArrays.OffsetArray{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2,Array{ColorTypes.RGB{FixedPointNumbers.Normed{UInt8,8}},2}}) at ./multimedia.jl:194

It arises because in showmime.jl, for images smaller than 10^4 pixels, the image gets upsampled (https://github.com/JuliaImages/Images.jl/blob/3bc4395928d97f162293e5b047a4357a9b9cbd44/src/showmime.jl#L33). The upsampling / repeat happens in Base, which eventually calls this: https://github.com/JuliaLang/julia/blob/903644385b91ed8d95e5e3a5716c089dd1f1b08a/base/abstractarraymath.jl#L373, which has a call to size(A), which fails for offset arrays.

I am new to the Julia ecosystem so would appreciate a suggestion about how to fix it. There seem to be a few options:

  1. Overload size(A) inside Images.jl with the official OffsetArrays fix:
_size(A::AbstractArray) = map(length, indices(A))
_size(A) = size(A)
  1. Replace the line in Base that calls size(A) with map(length, indices(A))
  2. Have showmime.jl implement a custom version of repeat with the OffsetArrays fix

Definitely happy to fix this with some guidance about best practices!

timholy commented 7 years ago

Excellent diagnosis @mprat! As you noticed, there are several options, and I'd agree that your second one is probably best since it fixes an error that has nothing to do with showing or Images:

julia> using OffsetArrays

julia> a = OffsetArray(rand(3,3), (-2, -2))
OffsetArrays.OffsetArray{Float64,2,Array{Float64,2}} with indices -1:1×-1:1:
 0.577663  0.96836   0.219044
 0.510247  0.841898  0.735284
 0.901021  0.473618  0.458248

julia> repeat(a; inner=(2, 2))
ERROR: size not supported for arrays with indices (-1:1, -1:1); see http://docs.julialang.org/en/latest/devdocs/offset-arrays/
Stacktrace:
 [1] errmsg(::OffsetArrays.OffsetArray{Float64,2,Array{Float64,2}}) at /home/tim/.julia/v0.6/OffsetArrays/src/OffsetArrays.jl:48
 [2] _repeat at ./abstractarraymath.jl:394 [inlined]
 [3] #repeat#117 at ./abstractarraymath.jl:366 [inlined]
 [4] (::Base.#kw##repeat)(::Array{Any,1}, ::Base.#repeat, ::OffsetArrays.OffsetArray{Float64,2,Array{Float64,2}}) at ./<missing>:0

If you're running (at least) 0.6, you can do the following sweet trick:

julia> eval(Base, quote
       rep_shapes(A, i, o) = _rshps((), (), map(length, indices(A)), i, o)
       end)
WARNING: Method definition rep_shapes(Any, Any, Any) in module Base at REPL[7]:2 overwritten at REPL[10]:2.
rep_shapes (generic function with 1 method)

so that you can test your solution without having to go to the effort/delay of rebuilding julia. When you do that, you verify that your solution works well, but you pretty quickly get the next error:

julia> repeat(a; inner=(2, 2))
ERROR: BoundsError: attempt to access 6×6 Array{Float64,2} at index [-3:-2, -3:-2]
Stacktrace:
 [1] throw_boundserror(::Array{Float64,2}, ::Tuple{UnitRange{Int64},UnitRange{Int64}}) at ./abstractarray.jl:433
 [2] checkbounds at ./abstractarray.jl:362 [inlined]
 [3] macro expansion at ./multidimensional.jl:487 [inlined]
 [4] _setindex! at ./multidimensional.jl:484 [inlined]
 [5] setindex!(::Array{Float64,2}, ::Float64, ::UnitRange{Int64}, ::UnitRange{Int64}) at ./abstractarray.jl:967
 [6] _repeat at ./abstractarraymath.jl:411 [inlined]
 [7] #repeat#117(::Tuple{Int64,Int64}, ::Tuple{Int64,Int64}, ::Function, ::OffsetArrays.OffsetArray{Float64,2,Array{Float64,2}}) at ./abstractarraymath.jl:366
 [8] (::Base.#kw##repeat)(::Array{Any,1}, ::Base.#repeat, ::OffsetArrays.OffsetArray{Float64,2,Array{Float64,2}}) at ./<missing>:0

There's a legitimate question about what the indices should be, but I think there's fairly clear justification for saying that repeat should act like collect, meaning that it returns an Array and discards any non-conventional indices.

If I were to try fixing this myself, I might set indsA = indices(A) at the beginning of _repeat, so I can access the results repeatedly without paying any performance hit for recomputing the indices (a few array types require some computation to return the indices). Then I'd see if I can modify the indexing computations in a way that uses 1-based indexing for R but the native indexing for A. Not sure how easy this will be, but a brief glance seemed to suggest it might not be too bad.

The other thing worth pointing out is that Base tests basically can't rely on packages, but there's a test/offsetarray.jl test file that you could add this test to. We basically copied the OffsetArrays package into test/TestHelpers.jl to make this possible.

If you do tackle this and submit a PR to base, please ping me and hopefully I'll notice it and give you a good review. And don't hesitate to ask for help in the meantime! Thanks for spotting this and for your willingness to dive in.

johnnychen94 commented 4 years ago

With using ImageShow this isn't an issue anymore so I'm closing it now.