Streamline API for warp and WarpedView #24

Evizero · 2017-04-21T16:25:08Z

With this API refactor the following is guaranteed to hold in all cases (well I still need to write the tests, but that is the basic idea)

wv = WarpedView(A, tfm)
parent(wv) === A

I introduce a new function warpedview which allows a little convenience concerning the interpolation/extrapolation settings. It will still enforce "view" property (so no Quadratic etc), but it will make the API nicer to work with.

wv = warpedview(A, tform, Constant())
wv = warpedview(A, tform, Linear())
wv = warpedview(A, tform, 1) # just extrapolation fill value
wv = warpedview(A, tform, Linear(), 1)

The functions above will cause parent(wv) !== A, because they are convenience functions that simulate a user calling WarpedView(extrapolate(interpolate(A,...),...), tform). This is why these signatures aren't part of WarpedView. I think this makes the design cleaner. Also with this change, the summary string is now always accurate and no longer based on guessing.

Note I have some follow up plans that would expand warpedview further, so this isn't the only reason for this function to exist.

absorbs #23

tests are currently broken because I did not update them yet

Evizero · 2017-04-21T16:32:59Z

Sorry that my PRs tend to be so difficult to review in this package. Since the package is quite young, I am still moving code around if it things look out of place. Better now than later

timholy · 2017-04-21T18:07:46Z

I like the WarpedView guarantee at top. I also like warpedview, and on balance it may be the right API. However, the obvious concern is that despite its name it doesn't create a view of the input.

A relevant comparison is colorview vs ColorView. Both of these create a view, but the distinction is that the return type of ColorView is ColorView, whereas colorview might return any suitable view type that "acts like" ColorView. (e.g., it will call reinterpret when appropriate.) I worry a bit that introducing an uppercase/lowercase pair with different behavior might be a bit confusing.

The main alternative that occurs to me is to introduce a warp_prepare or warp_extrapolant function that focuses on the preparation of the etp and then pass that as an argument to WarpedView. I'm not entirely thrilled with this, either, so this is firmly in "discussion" mode and not meant to be a TODO item.

Evizero · 2017-04-21T18:15:49Z

However, the obvious concern is that despite its name it doesn't create a view of the input.

Can you elaborate on that? Why is it not a view? Because of the interpolation?
Other than that my current mental model of what makes something a view seems satisfied.

Evizero · 2017-04-21T18:16:26Z

or are you explicitly talking about the lower case version ?

timholy · 2017-04-21T18:19:40Z

Perhaps I was confused. I assumed you meant you would support warpedview(A, tform, Quadratic(Flat())). Now that I suspect the answer is "no," then I rescind my complaint. 👍

Evizero · 2017-04-21T18:29:39Z

Oh, understood. No, I had the same concerns actually.

For WarpedView I do not allow convenience parameters at all. Only the array, the transform, and the indices. But it is possible to supply a prepared interpolation/extrapolation as the array.

For warpedview, I restrict the optional convenience parameters for the interpolation to Linear() or Constant() (the BSpline(...) part is assumed and hard-coded). I also create the BSplineInterpolation manually to prevent prefilter. The extrapolation can be numbers, colorants, or flags such as Flat() and Periodic(). If one calls warpedview with custom settings, it will act as if one called WarpedView with a manually prepared extrapolation (that must be a view of the original array). Thus this extrapolation will be the parent, not the inital array. However, the extrapolation will be a view on the array. This fact is also accurately displayed in the summary string.

Naturally it is allowed to manually pass any interpolation or extrapolation to both functions as first parameter. But then it was a user decision and the WarpedView will be a view on that input

timholy

👍 One tiny change, and one scary discussion point.

timholy · 2017-04-21T18:39:35Z

src/warp.jl

+end
+
+function warp{T,N}(img::AbstractArray{T,N}, tform, fill::FillType, args...)
+    warp(img, tform, Linear(), fill)


args is missing, or more likely shouldn't be in the signature.

timholy · 2017-04-21T18:53:01Z

src/warpedview.jl

    indices::I
+    inverse::F2


Uh oh...I am finally noticing a consequence of my very first commit to this repo, 3b9f9ff. There, I defined warp by iterating over destination pixels, and applying tinv to its coordinates.

The problem is, that works fine only if the transformation is invertible. But some day we might want to support, e.g., a deformation encoded as a vector field, and in that case the transformation is not trivially invertible. We can still use the transformation if we supply the inverse directly, and never use the "forward" transformation. (Getting the indices of dest might require solving a nonlinear equation, of course.)

That seems potentially important for the constructor and parametrization of WarpedView. We might consider adding a new abstract type, InvertibleTransformation, and use this parametrization for just this class of transformations.

But now I'm struggling to remember why I used inv(tfm). It looks like we're consistent with Matlab, which I guess is a good thing. But now I worry that I may have made a mistake by choosing w = img[inv(tfm)(x)] rather than w = img[tfm(x)]. What are your thoughts?

I did notice the inv part quite early. Well, that sounds more observant than it is, because basically I had to reverse engineer what the code does in order to understand what is going on. I also spend some thoughts on the issue you describe.

I like this approach for the simple reason as it behaves how I want it to behave. I like to reason about how the image changes, not how the indices change. Also I do want the option to nest WarpedView for a use case I have. Basically I like this way because it is convenient for me.

Why don't we avoid commitment to one interpretation altogether. We could add a InvWarpedView (and invwarp) which does w = img[tfm(x)] as an alternative.

That sounds like the sanest approach. We should probably document somewhere that the transformation is effectively applied to the image rather than the coordinates. It's probably more mathematically general/elegant to have it apply to the coordinates, but I agree that intuition goes the other way. And consistency with other languages (assuming Matlab isn't the only one) is a good thing.

We do also have a choice which one we call InvWarpedView

In the R package imager it seems that a user has the choice to specify "forward" or "backward" mode, where forward (which is ironically what we do right now with inv; well not quite, they literally perform it forward by iterating over the source pixel) is the default

ref: https://dahtah.github.io/imager/imager.html#warping

Maybe FwdWarpedView and BwdWarpedView? Although I do think that one of them should just be WarpedView. Personally I vote for keeping the current one as is, and adding the alternative with one of the following names (any other ideas?)

InvWarpedView

BwdWarpedView

IdsWarpedView

WarpedIndicesView

ReverseWarpedView

RevWarpedView

Sure, let's keep the current one WarpedView. I like WarpedIndicesView, could also considerWarpedCoordsView.

Evizero · 2017-04-21T20:10:35Z

Ok, so now I kinda changed my mind again. So here are my thoughts on the inv issue.

First I thought we are kinda performing forward mode warping, since we supply the forward transformation. However, I have come to appreciate that, no, we are not. We are performing backward warping with an - for that functionality - unintuitive interface. We do backward warping by specifying the forward transform.

So really I think we should change how warp itself works to "backward" mode, which means that it should do w = img[tfm(x)] instead of using inv. ~~Actually I tend to think we should additionally add some parameter for dispatch warp(A, tfm, WarpMode.Backward()). This would allow us to add Forward() in the future.~~ EDIT: let's leave that for some other time

We can change WarpedView to then perform backward mode as well (i.e. avoid inv).

The functionality I am after right now with the current WarpedView could be renamed to InvWarpedView, which I'd be fine with. Edit: InvWarpedView because it does backward warping by specifying the inverse transform of the required one. which allows it to keep track easily by composition

EDIT: also it would still be simple to get the current functionality back by doing warp(img, inv(tform))

Evizero · 2017-04-21T20:28:03Z

sorry for causing this kind of confusions. Since I am leaning out of my field of expertise I often have to study up on topics/related work as I go along or issues emerge. My background is ML and not image processing specifically. Thanks for all that feedback. These quick responses you keep giving me are really appreciated.

Evizero · 2017-04-21T21:27:06Z

src/warp.jl

@@ -70,39 +94,26 @@ julia> indices(imgr)
 (Base.OneTo(906),Base.OneTo(905))
 ```
 """
-function warp{T}(img::AbstractExtrapolation{T}, tform)
-    inds = autorange(img, tform)
+function warp{T}(img::AbstractExtrapolation{T}, tform, inds::Tuple = autorange(img, inv(tform)))


we still need to use inv to compute the indices. But I changed it so that a user can provide them instead and thus avoid inv

Evizero · 2017-04-21T22:36:30Z

I ported and adapted the ImageInTerminal style of comparing to small reference images (each takes around 5-15 kb so their git footprint is on the level of a source code file). For testing the warping functionality, these small thumbnail images are actually informative enough for most tests, so I thought this will make it a lot easier to really test the API properly going forward. Well, at least as far as warping is concerned. I don't think they will be good enough to pick up on interpolation differences

The one downside of this is that the reference files aren't really that useful to look at on github.

andyferris · 2017-04-21T23:15:13Z

The forward and backward thing is interesting. This seems to also be a problem in CoordinateTransformations, where it's not really clear whether a Transformation is active or passive.

In some cases like changing coordinate systems it is passive (the point doesn't move - it's expressed in a new frame). However, it seems things like LinearMap are more designed as active transformations e.g. you want to rotate the points about an axis, say.

My point being that if we find a way of expressing the difference upstream in CoordinateTransformations, perhaps it would solve the ambiguity here. Cc @c42f

timholy · 2017-04-22T14:45:09Z

To avoid polluting this PR, I've opened #25 to discuss the forward/backward thing.

Evizero · 2017-04-22T16:01:53Z

I feel bad about triggering all the codecov spam. Any way we can reduce these messages?

Evizero · 2017-04-23T21:54:09Z

latest tests require JuliaMath/Interpolations.jl#150

Evizero · 2017-04-27T12:24:14Z

Open points / debates before merging

Allow Quadratic and Cubic to be passed to warp directly? Can we do a vote on that? I do like the "view" guarantee but I am not opposed to doing this if everyone here thinks it is worth doing
Grid discussion Streamline API for warp and WarpedView #24 (comment)
Some more tests for the different warp signatures (I know which ones, just need to implement them).
docstring for warpedview, invwarpedview, and types
tests for warpedview, invwarpedview, and types
Use or don't use a @generated function to print the dep warning? I think looking at the same warning every time one calls the method might be a bit of an overkill.
(maybe another PR) Add WarpedView and use as basis for InvWarpedView

anything else?

timholy · 2017-04-27T12:56:59Z

I am in favor of the "view" guarantee if you're explicitly creating a view via, e.g., invwarpedview. But warp doesn't promise a view, and that's where I think it's reasonable to support non-view interpolation schemes. As you say, I don't think this is a make-or-break point, since it's not hard to create the interpolation object directly. So I'll be fine however this decision comes out.

timholy · 2017-04-27T12:58:16Z

I think looking at the same warning every time one calls the method might be a bit of an overkill.

Base.depwarn collects a stacktrace and then doesn't print the warning if it's from the same originating line of code. So (when the line numbering works, which it doesn't always), you get exactly one depwarn per executed line of code that needs changing, even if you're looping over something.

Evizero · 2017-04-27T21:18:37Z

good feedback. will work it in

Evizero · 2017-04-29T19:07:12Z

I think the code is in place now. Just need to verify that with some tests and write some docstrings

Evizero · 2017-04-29T19:10:00Z

One discussion point maybe. We tend to call the transformation variables tform or tinv. Should be consider naming them passive and active instead?

c42f · 2017-04-30T00:21:41Z

I prefer tform and tinv over the words active and passive. I think the terms are uncommon enough and the concept subtle enough that users will have to look at the documentation anyway. And if they do, it's going to be easier to discuss in the documentation with words like tform which are more of an obvious placeholder for the object than a normal word like `active.

Evizero · 2017-04-30T21:27:19Z

src/warp.jl

    warp!(out, img, tform)
 end

+_allocate_array{T,N}(::Type{T}, inds::NTuple{N,Base.OneTo}) = Array{T}(map(length, inds))
+_allocate_array{T,N}(::Type{T}, inds::NTuple{N,AbstractUnitRange}) = OffsetArray(Array{T}(map(length, inds)), inds)


unsure if this change could be considered controversial, but I did it because now behaviour should be consistent between warp(...) and collect(warpedview(...)). It does not negatively affect type inference

julia> summary(warp(img_camera, tfm, indices(img_camera))) "512×512 Array{Gray{N0f8},2}" julia> summary(collect(warpedview(img_camera, tfm, indices(img_camera)))) "512×512 Array{Gray{N0f8},2}"

and

julia> summary(warp(img_camera, tfm)) "-78:591×-78:591 OffsetArray{Gray{N0f8},2}" julia> summary(collect(warpedview(img_camera, tfm))) "-78:591×-78:591 OffsetArray{Gray{N0f8},2}"

The behavior of collect is changing in 0.6, see JuliaLang/julia#21257. In particular we decided that collect was the right way to spell "throw away the custom indices and return an Array with the same values." (See JuliaArrays/IdentityRanges.jl#1.)

Nevertheless I like this change overall. I would write the second, however, as similar(Array{T}, inds). That way the indices are in charge of the output type. When inds are UnitRanges, then it will allocate an OffsetArray.

oh, yeah. that is a lot nicer. thanks

Evizero · 2017-05-03T07:40:14Z

Ready for the last round of reviews

timholy

Sorry for the delay in getting to this. Really nice! I think the only discussion-worthy one is the tinv issue in the docstring for invwarpedview etc. It's hard to figure out the right thing to do here.

timholy · 2017-05-04T10:15:22Z

src/interpolations.jl

+@inline _default_fill{T<:FloatColorant}(::Type{T}) = _nan(T)
+@inline _default_fill{T}(::Type{T}) = zero(T)
+
+box_extrapolation(etp::AbstractExtrapolation) = etp


I was tempted by wrap_extrapolation, but I think that could be confusing: people might think it's a typo of warp! So let's stay with box. (EDIT: originally I had a typo in the word typo. Amusing!)

timholy · 2017-05-04T10:19:03Z

src/invwarpedview.jl

+Create a view of `img` that lazily transforms any given index `I`
+passed to `wv[I]` to correspond to `img[inv(tinv)(I)]`. While
+technically this approach is known as backward mode warping,
+`InvWarpedView` is created using the forward transformation.


Perhaps "note that InvWarpedView is created by supplying the forward transformation"?

I wonder if it's a little confusing to call this parameter tinv, and then talk about inv(tinv)? I guess the alternative is to just call it tform. I recognize that this has the problem of having tform mean different things for WarpedView and InvWarpedView.

My personal justification for using tinv is that it is the inverse of the transformation that is actually requested by the algorithm (which is backward mode warping). I agree it doesn't look nice, but at least it gives it some consistency

If you feel like InvWarpedView might be a bit too far out there for the context of this package I could also just move it to the package where I actually need it,

I'm fine either way, whichever you think will be best.

Lets keep it here then. I like this additional layer of quality control. I also suspect that between InvWarpedView and WarpedView, the former is more convenient for a user to work with

timholy · 2017-05-04T10:33:00Z

src/warp.jl

    warp!(out, img, tform)
 end

+_allocate_array{T,N}(::Type{T}, inds::NTuple{N,Base.OneTo}) = Array{T}(map(length, inds))
+_allocate_array{T,N}(::Type{T}, inds::NTuple{N,AbstractUnitRange}) = OffsetArray(Array{T}(map(length, inds)), inds)


The behavior of collect is changing in 0.6, see JuliaLang/julia#21257. In particular we decided that collect was the right way to spell "throw away the custom indices and return an Array with the same values." (See JuliaArrays/IdentityRanges.jl#1.)

Nevertheless I like this change overall. I would write the second, however, as similar(Array{T}, inds). That way the indices are in charge of the output type. When inds are UnitRanges, then it will allocate an OffsetArray.

timholy · 2017-05-04T15:48:38Z

If I don't notice, feel free to merge once this gets through CI. Thanks for doing this! A huge step forward.

Evizero · 2017-05-04T15:56:00Z

This should be a squash merge since I was very casual with my commits

codecov-io · 2017-05-04T16:53:44Z

Codecov Report

Merging #24 into master will increase coverage by 0.7%.
The diff coverage is 98.57%.

@@            Coverage Diff            @@
##           master      #24     +/-   ##
=========================================
+ Coverage   97.28%   97.99%   +0.7%     
=========================================
  Files           5        7      +2     
  Lines         221      249     +28     
=========================================
+ Hits          215      244     +29     
+ Misses          6        5      -1

Impacted Files	Coverage Δ
src/resizing.jl	`98.54% <ø> (ø)`	⬆️
src/ImageTransformations.jl	`100% <100%> (ø)`	⬆️
src/warpedview.jl	`100% <100%> (ø)`	⬆️
src/warp.jl	`100% <100%> (+7.69%)`	⬆️
src/invwarpedview.jl	`100% <100%> (ø)`
src/interpolations.jl	`95.83% <95.83%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ee9d526...1ae4634. Read the comment docs.

Evizero · 2017-05-04T21:39:52Z

Thanks for all the feedback and discussions. This was a fun PR!

c42f · 2017-05-05T12:26:02Z

Sorry I didn't have more time to put into reviewing this, I don't know how Tim does it! Great effort btw.

Evizero added 2 commits April 21, 2017 18:11

refactor warp to be consistent with WarpedView

1957490

clean up WarpedView API and add warpedview

fccebb1

Evizero mentioned this pull request Apr 21, 2017

refactor warp to be consistent with WarpedView #23

Closed

timholy approved these changes Apr 21, 2017

View reviewed changes

Evizero force-pushed the cs/parent branch from 7f6288f to 77b2228 Compare April 21, 2017 21:24

Evizero commented Apr 21, 2017

View reviewed changes

refactor warp and WarpedView to proper backward mode

1015bfd

Evizero force-pushed the cs/parent branch from 77b2228 to 1015bfd Compare April 21, 2017 21:33

Evizero force-pushed the cs/parent branch from 596b0e3 to a935be4 Compare April 22, 2017 14:32

add ImageInTerminal style visual tests

c257622

Evizero force-pushed the cs/parent branch from a935be4 to c257622 Compare April 22, 2017 14:34

timholy mentioned this pull request Apr 22, 2017

Consider changing sense of transformation #25

Closed

Evizero added 2 commits April 22, 2017 17:04

enable color on travis

38bcb40

try to enable color on appveyor

6a2d808

Evizero force-pushed the cs/parent branch from eca7118 to 6a2d808 Compare April 22, 2017 15:36

add tests for warp helper functions

e5e6ccb

Evizero mentioned this pull request Apr 24, 2017

nan not defined for HSV JuliaGraphics/ColorVectorSpace.jl#75

Closed

Evizero force-pushed the cs/parent branch from e838b60 to 378dde6 Compare April 27, 2017 22:02

work in a round of reviews

a2aa627

Evizero force-pushed the cs/parent branch from 378dde6 to a2aa627 Compare April 27, 2017 22:11

reintroduce WarpedView

b79619d

Evizero added 2 commits April 30, 2017 16:36

add warp support for OneTo inds

a805cc3

finish warpedview tests

fcf939c

Evizero commented Apr 30, 2017

View reviewed changes

Evizero added 2 commits May 2, 2017 12:05

add docstring for warpedview

5ca0028

add docstring for invwarpedview

86d0afb

Evizero changed the title ~~WIP: Streamline API for warp and WarpedView~~ Streamline API for warp and WarpedView May 3, 2017

fix 0.6 ambiguities

ad5cb02

Evizero mentioned this pull request May 3, 2017

Special treatment for SubArray of InvWarpedView #26

Closed

timholy approved these changes May 4, 2017

View reviewed changes

work in a round of reviews

1ae4634

Evizero merged commit 0551a0d into master May 4, 2017

Evizero deleted the cs/parent branch May 4, 2017 21:42

johnnychen94 mentioned this pull request Jul 18, 2021

deprecate invwarpedview in favor of InvWarpedView #138

Merged

1 task

Streamline API for warp and WarpedView #24

Streamline API for warp and WarpedView #24

Conversation

Evizero commented Apr 21, 2017 • edited Loading

Evizero commented Apr 21, 2017

timholy commented Apr 21, 2017

Evizero commented Apr 21, 2017

Evizero commented Apr 21, 2017 • edited Loading

timholy commented Apr 21, 2017

Evizero commented Apr 21, 2017 • edited Loading

timholy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Evizero Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Evizero Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Evizero Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Evizero commented Apr 21, 2017 • edited Loading

Evizero commented Apr 21, 2017

Choose a reason for hiding this comment

Evizero commented Apr 21, 2017 • edited Loading

andyferris commented Apr 21, 2017

timholy commented Apr 22, 2017

Evizero commented Apr 22, 2017

Evizero commented Apr 23, 2017

Evizero commented Apr 27, 2017 • edited Loading

timholy commented Apr 27, 2017

timholy commented Apr 27, 2017

Evizero commented Apr 27, 2017

Evizero commented Apr 29, 2017

Evizero commented Apr 29, 2017

c42f commented Apr 30, 2017

Evizero Apr 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Evizero commented May 3, 2017

timholy left a comment

Choose a reason for hiding this comment

timholy May 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timholy commented May 4, 2017

Evizero commented May 4, 2017

codecov-io commented May 4, 2017 • edited Loading

Codecov Report

Evizero commented May 4, 2017

c42f commented May 5, 2017

Evizero commented Apr 21, 2017 •

edited

Loading

Evizero commented Apr 21, 2017 •

edited

Loading

Evizero commented Apr 21, 2017 •

edited

Loading

Evizero Apr 21, 2017 •

edited

Loading

Evizero Apr 21, 2017 •

edited

Loading

Evizero Apr 21, 2017 •

edited

Loading

Evizero commented Apr 21, 2017 •

edited

Loading

Evizero commented Apr 21, 2017 •

edited

Loading

Evizero commented Apr 27, 2017 •

edited

Loading

Evizero Apr 30, 2017 •

edited

Loading

timholy May 4, 2017 •

edited

Loading

codecov-io commented May 4, 2017 •

edited

Loading