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

Streamline API for warp and WarpedView #24

Merged
merged 18 commits into from
May 4, 2017
Merged

Streamline API for warp and WarpedView #24

merged 18 commits into from
May 4, 2017

Conversation

Evizero
Copy link
Member

@Evizero Evizero commented Apr 21, 2017

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
Copy link
Member Author

Evizero commented Apr 21, 2017

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
Copy link
Member

timholy commented Apr 21, 2017

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
Copy link
Member Author

Evizero commented Apr 21, 2017

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.

screen shot 2017-04-21 at 20 11 43

@Evizero
Copy link
Member Author

Evizero commented Apr 21, 2017

or are you explicitly talking about the lower case version ?

@timholy
Copy link
Member

timholy commented Apr 21, 2017

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
Copy link
Member Author

Evizero commented Apr 21, 2017

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.

screen shot 2017-04-21 at 20 25 37

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

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.

👍 One tiny change, and one scary discussion point.

src/warp.jl Outdated
end

function warp{T,N}(img::AbstractArray{T,N}, tform, fill::FillType, args...)
warp(img, tform, Linear(), fill)
Copy link
Member

Choose a reason for hiding this comment

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

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

indices::I
inverse::F2
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

@Evizero Evizero Apr 21, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do also have a choice which one we call InvWarpedView

Copy link
Member Author

@Evizero Evizero Apr 21, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@Evizero Evizero Apr 21, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@Evizero
Copy link
Member Author

Evizero commented Apr 21, 2017

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
Copy link
Member Author

Evizero commented Apr 21, 2017

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.

src/warp.jl Outdated
@@ -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)))
Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

Evizero commented Apr 21, 2017

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
Copy link
Contributor

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
Copy link
Member

timholy commented Apr 22, 2017

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

@Evizero
Copy link
Member Author

Evizero commented Apr 22, 2017

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

@Evizero
Copy link
Member Author

Evizero commented Apr 23, 2017

latest tests require JuliaMath/Interpolations.jl#150

@Evizero
Copy link
Member Author

Evizero commented Apr 27, 2017

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
Copy link
Member

timholy commented Apr 27, 2017

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
Copy link
Member

timholy commented Apr 27, 2017

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
Copy link
Member Author

Evizero commented Apr 27, 2017

good feedback. will work it in

@Evizero
Copy link
Member Author

Evizero commented Apr 29, 2017

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

@Evizero
Copy link
Member Author

Evizero commented Apr 29, 2017

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

@c42f
Copy link
Member

c42f commented Apr 30, 2017

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.

src/warp.jl Outdated
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)
Copy link
Member Author

@Evizero Evizero Apr 30, 2017

Choose a reason for hiding this comment

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

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}"

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, yeah. that is a lot nicer. thanks

@Evizero Evizero changed the title WIP: Streamline API for warp and WarpedView Streamline API for warp and WarpedView May 3, 2017
@Evizero
Copy link
Member Author

Evizero commented May 3, 2017

Ready for the last round of reviews

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.

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.

@inline _default_fill{T<:FloatColorant}(::Type{T}) = _nan(T)
@inline _default_fill{T}(::Type{T}) = zero(T)

box_extrapolation(etp::AbstractExtrapolation) = etp
Copy link
Member

@timholy timholy May 4, 2017

Choose a reason for hiding this comment

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

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!)

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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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,

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

src/warp.jl Outdated
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)
Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

timholy commented May 4, 2017

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

@Evizero
Copy link
Member Author

Evizero commented May 4, 2017

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

@codecov-io
Copy link

codecov-io commented May 4, 2017

Codecov Report

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

Impacted file tree graph

@@            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 Evizero merged commit 0551a0d into master May 4, 2017
@Evizero
Copy link
Member Author

Evizero commented May 4, 2017

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

@Evizero Evizero deleted the cs/parent branch May 4, 2017 21:42
@c42f
Copy link
Member

c42f commented May 5, 2017

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

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.

5 participants