[WIP] fixed point perspective for imresize, zoom and imrotate
#142
Conversation
- (Breaking change) `imresize(::OffsetArray, ...)` now returns also an `OffsetArray` and uses `map(first, axes(img))` as the fixed point. - (Enhancement) `imresize(img, ::Indices)` now unconditionally returns an `OffsetArray`; this fixes the legacy type instability.
Codecov Report
@@ Coverage Diff @@
## master #142 +/- ##
==========================================
- Coverage 92.09% 86.00% -6.09%
==========================================
Files 8 9 +1
Lines 215 243 +28
==========================================
+ Hits 198 209 +11
- Misses 17 34 +17
Continue to review full report at Codecov.
|
| copyto!(dest, CartesianIndices(axes(dest)), original, CartesianIndices(inds)) | ||
| end | ||
| else | ||
| imresize!(similar(original, Tnew, new_size), original; kwargs...) | ||
| end | ||
| end | ||
|
|
||
| function imresize(original::OffsetArray{T,N}, new_size::Dims{N}; kwargs...) where {T,N} |
There was a problem hiding this comment.
OffsetArray is not necessarily the only array type that doesn't have indexing starting with 1; there are several other examples. What's the reason for the special method? It looks almost identical to the above.
There was a problem hiding this comment.
The only difference here is that the above method returns an Array while this new method for OffsetArray unconditionally returns an OffsetArray.
I also have the same concerns and I'm not very satisfied with this version very much, but it seems there's no other way to work around the type instability if we put them into one method that returns either OffsetArray or Array.
| if axes(original) == new_inds | ||
| copyto!(similar(original, Tnew), original) | ||
| OffsetArray(copyto!(similar(original, Tnew), original), origin) |
There was a problem hiding this comment.
Rather than manually create an OffsetArray, how about use similar(original, Tnew, new_inds)? That will create an OffsetArray if new_inds[i] isa UnitRange rather than a Base.OneTo.
There was a problem hiding this comment.
I believe this won't be type stable due to the if axes(original) == new_inds branch.
With this change:
- OffsetArray(copyto!(similar(original, Tnew), original), origin)
+ copyto!(similar(original, Tnew, new_inds), original)julia> img = testimage("cameraman");
julia> @inferred imresize(img, axes(img))
ERROR: return type Matrix{Gray{N0f8}} does not match inferred return type Union{Matrix{Gray{N0f8}}, OffsetMatrix{Gray{N0f8}, Matrix{Gray{N0f8}}}}| @test test_imresize_interface(img, (20,10), (1:20, 1:10), ratio = (2, 1)) isa Array | ||
|
|
||
| # indices method always return OffsetArray | ||
| @test test_imresize_interface(img, (5,5), (1:5, 1:5), (1:5,1:5)) isa OffsetArray |
There was a problem hiding this comment.
Also add
@test test_imresize_interface(img, (5,5), (1:5, 1:5), (Base.OneTo(5),Base.OneTo(5))) isa Array
|
How about this as a proposal:
|
|
This PR becomes more delicate than I expected, and it's quite hard to find a way out for |
Let me recite our newly-added documentation for the symbols and terminology:
A (backward-mode) warp operation consists of two parts: the value estimator τ (which we always use Interpolations.jl in this package), and the backward coordinate map ϕ. For some special warping operations such as scale(
imresize), zoom, and rotation: unless it's an identity map, there exists one and only one point that satisfiesϕ(p) == p, and thispis defined as the fixed point.This PR aims to provide consistent reasoning on this "fixed point" concept for these three operations.
imresizeBecause
imresizealways spans the entire source domain, i.e.,CartesianIndices(src_img), exposing a control keyword for fixed point only changes the offset of the output. For example:imresize(rand(10, 10), (5, 5); fixed_point = (5, 5))requires the output array (an offset array) axes be(3:7, 3:7)so thatϕ((5, 5)) == (5, 5).Also note that
imresize(rand(10, 10), (5, 5))returns anArrayin previous ImageTransformations versions, and I don't think we should change this because 1) it exists for so long andArrayas the base julia type has the best support among the entire ecosystem, and 2)OffsetArrayintroduces an extra step when computing the indices, so it might hurt the performance (although in many cases that I've observed, Julia compiler just optimizes this overhead away).Thus for type stability, we can't make
fixed_pointa keyword, and this is why we didCenterPointthing in #129 and it works pretty well.However, one key question that #129 doesn't answer is: what is the default fixed point? And trying to answer this leads me to open a new PR here.
The current master version of ImageTransformation doesn't have it well-defined: for
Arrayit is(1, 1). But forOffsetArrayit's quite inconsistant because it mapsp0 = map(first, axes(offset_img))toq0 = (1, 1), which says the fixed point is neither(1, 1)orp0.The first commit of this PR 605be80 does two things:
imresize(img, inds)method outputOffsetArray.map(first, axes(img)):imresize(offset_img, ...)now outputs anOffsetArrayinstead of anArray. This is no doubt a breaking change but it's for a more consistent result when speaking of the fixed point things.To support #129, we then just need some manual axes shifts on the output:
I'm now unsure if we should support fixed point concept for
imresizebecause it is really just a matter of offset shift.[WIP]
zoomzoomworks quite similar toimresizeexcept that the canvas size isn't changed, and the fixed point concept plays a key role in zoom operation.The second commit 33b5559 is a WIP for this new high-level API.
The currently implemented API is
zoom(img; ratio, [fixed_point], [method], [fillvalue]), I might also want to introducezoom(img, size_or_indices; [fixed_point], kwargs...)but I still need to think about if it's possible to interpret theindicesinput.Top-left point as fixed point :
map(first, axes(img))Center point as fixed point:
OffsetArrays.center(img)[TODO]
imrotateThe proposed API is to add
fixed_pointkeyword.Todo:
imresizezoomfixed_pointforimrotatecc: @ginkulv