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

Refactor imresize using Interpolations #7

Merged
merged 2 commits into from
Feb 23, 2017
Merged

Refactor imresize using Interpolations #7

merged 2 commits into from
Feb 23, 2017

Conversation

Evizero
Copy link
Member

@Evizero Evizero commented Feb 18, 2017

On my plane ride back from a work-related trip, I experimented with writing an imresize function using interpolations. The best I could come up with was this approach. It seems to work quite well for upscaling an Image, but when downscaling it, it is more of a sub-sampling than anything, which may not be the best way to do it.

Also, while playing around with Interpolations and Extrapolation (which are both an AbstractArray which trigger ImageInTerminal rendering), I noticed that restrict triggered some exceptions. For example one cant to A[1] if A is an interpolation

Copy link
Member

@timholy timholy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good! What's the performance like?

@@ -18,7 +18,10 @@ function restrict{T,N}(A::AbstractArray{T,N}, dim::Integer)
return A
end
newsz = ntuple(i->i==dim?restrict_size(size(A,dim)):size(A,i), Val{N})
out = Array{typeof(A[1]/4+A[2]/2),N}(newsz)
# FIXME: The following line fails for interpolations because
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. I haven't coded with Interpolations recently, but if it supports what we need then

R = CartesianRange(indices(A))
f = A[first(R)]

should work. Though perhaps the better strategy would to add support for first to Interpolations.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first(A) works for interpolations. A[1] does not. For extrapolations neither work

src/resizing.jl Outdated
convertsafely{T<:Integer}(::Type{T}, val::AbstractFloat) = round(T, val)
convertsafely{T}(::Type{T}, val) = convert(T, val)
function imresize!{T,S,N}(resized::AbstractArray{T,N}, original::AbstractArray{S,N})
itp = interpolate(original, BSpline(Linear()), OnGrid())
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to dispatch to a version of imresize! that accepts original::AbstractInterpolation. That way if people want higher-order interpolation they can get it easily.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea

@Evizero
Copy link
Member Author

Evizero commented Feb 18, 2017

What's the performance like?

Have not benchmarked yet. To be honest I was unsure if this is even close to what you had in mind

@timholy
Copy link
Member

timholy commented Feb 18, 2017

It's basically spot on. Nice work. My only concern is the inner loop, it seems possible we'd need to optimize it to get rid of the splat. But it's possible that the compiler will do all the hard work for us (which would be much better than doing it by hand).

@Evizero
Copy link
Member Author

Evizero commented Feb 18, 2017

Understood. I will look into performance

@Evizero
Copy link
Member Author

Evizero commented Feb 18, 2017

To get a little glimpse of where I am at I tried the following little micro benchmark as reference for the status quo

current master before my refactor:

julia> using Colors, ImageTransformations, BenchmarkTools

julia> org = rand(RGB, 20, 20);

julia> dst = similar(org, (1000,1000));

julia> @benchmark ImageTransformations.imresize!($dst, $org)
BenchmarkTools.Trial: 
  memory estimate:  0 bytes
  allocs estimate:  0
  --------------
  minimum time:     9.838 ms (0.00% GC)
  median time:      9.869 ms (0.00% GC)
  mean time:        9.873 ms (0.00% GC)
  maximum time:     10.129 ms (0.00% GC)
  --------------
  samples:          506
  evals/sample:     1
  time tolerance:   5.00%
  memory tolerance: 1.00%

This PR currently without additional optimization:

julia> @benchmark ImageTransformations.imresize!($dst, $org)
BenchmarkTools.Trial: 
  memory estimate:  9.59 KiB
  allocs estimate:  5
  --------------
  minimum time:     17.118 ms (0.00% GC)
  median time:      17.126 ms (0.00% GC)
  mean time:        17.153 ms (0.00% GC)
  maximum time:     18.202 ms (0.00% GC)
  --------------
  samples:          292
  evals/sample:     1
  time tolerance:   5.00%
  memory tolerance: 1.00%

(results are pretty consistent for both 0.5 and today's master)

@Evizero
Copy link
Member Author

Evizero commented Feb 18, 2017

The 0.6 test issue on travis is related to JuliaArrays/StaticArrays.jl#106

@Evizero
Copy link
Member Author

Evizero commented Feb 18, 2017

Concerning benchmark. The following implementation using a generated function yields the same regression as the current state of the PR

@generated function imresize!{T,S,N}(resized::AbstractArray{T,N}, original::AbstractInterpolation{S,N})
    quote
        org_size = size(original)
        dst_size = size(resized)
        @inbounds @nexprs $N i->(scale_i = org_size[i] / dst_size[i])
        @inbounds for I in CartesianRange(size(resized))
            I_d = I.I
            @nexprs $N i->(O_i = I_d[i] * scale_i)
            resized[I] = @nref $N original i->O_i
        end
        resized
    end
end

I don't think the tuple splatting is the issue (I also tried hardcoding the tuple splat and it made no difference). I am guessing the overhead is from interpolations maybe? The memory certainly is from interpolations as just creating the interpolation allocates the same amount.

@timholy
Copy link
Member

timholy commented Feb 20, 2017

OK, let's go with the simple version and then figure out how to improve Interpolations performance.

With regards to sub-sampling etc: I think when making a smaller image we're going to first have to do some filtering. We could either use ImageFiltering, or perhaps one could just restrict until it's smaller and then interpolate up? I'm sure there's literature on this, but I confess I haven't read it.

This seems fine to me. I'd say merge whenever you feel ready. To me it looks like the tests bork before we get to the imresize tests, so just make sure those work properly and we can fix other issues separately if you prefer.

@Evizero Evizero merged commit 1e05355 into master Feb 23, 2017
@Evizero Evizero deleted the imresize branch February 23, 2017 20:58
@timholy
Copy link
Member

timholy commented Feb 23, 2017

Yay!

@Evizero
Copy link
Member Author

Evizero commented Feb 23, 2017

Thanks for your comments and advice so far. I am looking forward to further contribute to this package

@timholy
Copy link
Member

timholy commented Feb 24, 2017

I think this is an important area for future growth, so I'm very glad to hear of your interest in pitching in! Right now my main focus is on getting the whole Images suite passing on 0.6, and in preparing the way for GSoC, so it's great to have people driving things forward in more "fundamental" respects.

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

Successfully merging this pull request may close these issues.

2 participants