add n-dimensional unwrap #198

platawiec · 2018-04-15T22:46:58Z

I've added the n-dimensional unwrap from Unwrap.jl here. The doc-string for unwrap goes over how to use it. The new functionality allows the user to take arbitrary arrays that are "wrapped" (or modulo some arbitrary number) and recover the original array. This is primarily useful when looking at the recovered phases for 2D/3D scenes. Also included is the ability to specify which dimensions are periodic.

There are two other changes to the old unwrap:

There is no longer a dim keyword, a user who wants to recover that functionality will have to pass the appropriate views.
The old unwrap would perform the modulo function on the input array. In other words, an array like [0.0, 6pi, 0.0] evaluated to [0.0, 0.0, 0.0]. If the user is worried that the array they are passing into unwrap has values which lie outside the range, then they should do .% before calling unwrap. Typically, the array passed in is already module some number, because that is why unwrap is being called in the first place.

I have not tested against v0.7. Let me know if there are any other tweaks to do.

martinholters · 2018-04-17T11:33:14Z

There is no longer a dim keyword, a user who wants to recover that functionality will have to pass the appropriate views.

AFAICT, it wasn't a keyword, but a positional argument. Anyway, we should (at least) add a deprecation, to avoid breaking anyones code. (I'm also not quite sure what you mean with the "appropriate views", so a deprecation will clarify things for me.)

platawiec · 2018-04-17T13:26:20Z

Sure, will add the deprecation warning.

The previous algorithm would give errors as seen in Fig. 5 (g, h) here: pdf

For multi-dimensional unwrapping this algorithm is far more robust, so if someone was interested in doing a true n-d unwrap they shouldn't be using the old algorithm. However, if they have some array A::Matrix{T}, where adjacent columns are independent, then they could instead perform unwrapping on each column:

for i in 1:size(A, 2)
    unwrap!(view(A, :, i))
end

platawiec · 2018-04-28T15:36:30Z

Added a deprecation warning by preserving the old code. Existing code will be unaffected, but users will be notified that there is now a better option available if their code is > one-dimensional.

martinholters · 2018-05-08T09:59:45Z

src/unwrap.jl

+function unwrap!(m::Array{T}, dim::Integer; range::Number=2pi) where T<:AbstractFloat
+    Base.depwarn(string("`unwrap!(m::Array{T}, dim::Integer; range::Number=2pi) ",
+        " where T<:AbstractFloat` is deprecated, ",
+        "use `unwrap!(m; range)` instead if your data has more than one dimension."),


This doesn't tell me how I get the old behavior that I'd need if I have e.g. multiple one-dimensional signals collected in a matrix that I'd like to individually unwrap.

platawiec · 2018-05-08T12:55:54Z

Added some more detail to the depwarn. I didn't add a code example for brevity, and because passing a n-dimensional view of n-dimensional data embedded in an n<m-dimensional array is generally applicable advice. A code example would lose generality.

Let me know if there's anything else you'd like to see.

martinholters · 2018-05-08T13:55:19Z

What I had been imagining was more like

@deprecate(unwrap!(m::Array{T}, dim::Integer; range::Number=2pi) where T<:AbstractFloat,
    whatever_code_construct_is_needed_here)

If whatever_code_construct_is_needed_here turns out to be too long, that might be an indication that we actually don't want to deprecate the current method. OTOH, I agree that writing this for the N-dimensional case is both non-trivial and probably rarely needed (if at all).

Another thing to note is that before this PR, the dim argument defaulted to ndims(m), so the meaning of unwrap(X) for X having more than one dimension would silently change. If anyone was relying on the present behavior, that would come as a nasty surprise.

What we could do is deprecate the multi-dimensional case now, and add the new version later (or even now but with a different name). However, I'm all ears for better ideas. Anyone else willing to chime in? @ssfrr perhaps?

platawiec · 2018-05-08T15:08:55Z

I had tried something with the @deprecate macro, but because the unwrap(m, args...; kwargs...) function signature is generic, it ended up "deprecating" function calls which should not have been. It's possible I mis-implemented something, but this is the behavior I observed going the @deprecate route. This is also why I dropped the dim argument, that way only people who explicitly used the dim functionality would be warned. Unfortunately, as you pointed out, this means the behavior is silently changed for people simple calling unwrap!(m).

Another thing to note is that before this PR, the dim argument defaulted to ndims(m), so the meaning of unwrap(X) for X having more than one dimension would silently change. If anyone was relying on the present behavior, that would come as a nasty surprise.

This is the main issue I see as well, and I think it's important to get it right. At the moment, I feel we may be prioritizing a valid, but "side-effect" use case (unwrap 1D data stored in N-D array) over the intended use case (unwrap N-D data). The previously implemented algorithm does the intended use case (poorly), via the side-effect. A complicating factor is that the previous doc-string seems to sanction the side-effect use case - I suppose this is coming from people who are more used to Audio/time-series DSP vs. image processing.

I'm fine with doing a staged deprecation, with a differently-named function in the interim, as long as the deprecation time frames are set beforehand and the interface is unified into a common unwrap! function in the end.

martinholters · 2018-05-09T07:36:32Z

I'd say what you call the "intended use case" is not handled by the current unwrap, it's only aiming at the "side-effect use case". And I whole-heartedly agree that we should add the proper multi-dimensional unwrap and that it should eventually be the default thing to do for unwrap(A). I'm not entirely convinced we should deprecated the case with dim specified, though. If we look at the direction base has taken (e.g. cumsum is somewhat comparable), the following API might make sense (optional range keyword argument omitted for brevity):

unwrap(A): multi-dimensional unwrap as added by this PR; coinciding with current behavior for one-dimensional A
unwrap(A, dims=n): one-dimensional unwrap along dimension n (currently unwrap(A, n))

The mutating version should probably take input and output arguments, which may be equal for true in-place operation.

This suggests the following route:
1a. Deprecate unwrap(A, dim) to unwrap(A; dims=dim) and unwrap(A) where A has more than one dimension to unwrap(A; dims=ndims(A)). Deprecate unwrap!(A, dim) to unwrap!(A, A, dims=dim) and unwrap!(A) to unwrap!(A, A) (for one-dimensional A) or unwrap!(A, A, dims=ndims(A)) (for multi-dimensional A).
1b. Add the multi-dimensional unwrap with a distinct name.
2. Tag a release, let some time pass for users to adjust their code.
3. Rename the multi-dimensional unwrap to unwrap (with a deprecation).
4. Tag a release, let some time pass for users to adjust their code.
5. Remove the old name for the multi-dimensional unwrap.

If we can agree on something like this, I could take care of 1a while you think of a nice name for 1b 😄

platawiec · 2018-05-09T13:36:26Z

Sounds like a plan, very clear thanks.

So the multi-dimensional unwrap should look something like unwrapimage!(dest, src)?

ssfrr · 2018-05-09T16:52:45Z

I haven't looked into the code deeply yet, but from an API standpoint I support a dim argument vs. relying on view, though I think that sum might be a better model than cumsum. cumsum is an inherently 1D operation that could be embedded inside a higher-dimensional Array. sum is a multi-dimensional operation like the new unwrap, (but that could also be embedded inside a higher-dimensional Array) so rather than a dim argument there's dims that describes along which dimensions you want to unwrap.

That said, I think it's OK to currently only support the 1D case where dims::Integer, and then in the future we could extend to support iterable dims without breaking anything.

So my vote would be for an interface like:

unwrap(A) - multidimensional unwrap
unwrap(A; dims=2) - 1D unwrap along dimension 2
unwrap(A; dims=(1,2)) - 2D unwrap inside a higher-dimensional array (like a 3D Array representing frames of a video) (maybe to be implemented in the future)

I'm on-board with a 2-step deprecation where unwrap(A) deprecated for multidimensional arrays. I don't have a good sense for the right timeframe - maybe a few months?

One alternative to a separate function unwrapimage would be unwrap(A, dims=1:ndims(A)), basically we just force that people specify what they want explicitly during the deprecation while we change the default.

as a side note: fft seems like it would be an appropriate model, but it uses dims as a positional argument rather than keyword. I filed an issue to ask whether it makes sense to unify with the Base convention.

platawiec · 2018-05-09T20:26:25Z

Great discussion points, thanks @ssfrr . I think the analogy to fft is good, where multi-dimensional is the default.

Just wanted to point out, for educational reason:

unwrap(A; dims=(1,2)) - 2D unwrap inside a higher-dimensional array (like a 3D Array representing frames of a video) (maybe to be implemented in the future)

Frames of a video should be unwrapped via a 3D unwrap - presumably the frames of a video are looking at an object in time, and the phase information is not entirely scrambled between frames. Because there is a dimensional relationship, 3D unwrap would be the appropriate function to call. If, on the other hand, a 3D array was storing a bunch of 2D images, then the signature you quote would be correct.

ssfrr · 2018-05-09T21:05:06Z

Frames of a video should be unwrapped via a 3D unwrap - presumably the frames of a video are looking at an object in time, and the phase information is not entirely scrambled between frames. Because there is a dimensional relationship, 3D unwrap would be the appropriate function to call. If, on the other hand, a 3D array was storing a bunch of 2D images, then the signature you quote would be correct.

Ah yes, excellent point. Assume I meant a collection of unrelated images then. ;)

martinholters · 2018-05-14T10:06:20Z

One alternative to a separate function unwrapimage would be unwrap(A, dims=1:ndims(A))

Yes, that's much better. So the revised plan:
1a. Deprecate unwrap(A, dim) to unwrap(A; dims=dim) and unwrap(A) where A has more than one dimension to unwrap(A; dims=ndims(A)). Deprecate unwrap!(A, dim) to unwrap!(A, A, dims=dim) and unwrap!(A) to unwrap!(A, A) (for one-dimensional A) or unwrap!(A, A, dims=ndims(A)) (for multi-dimensional A).
1b. Add the multi-dimensional unwrap for unwrap(A, dims=1:ndims(A)).
2. Tag a release, let some time pass for users to adjust their code.
3. Make dims=1:ndims(A) the default.

Make the API of `unwrap`/`unwrap!` similar to the one of `accumulate`/`accumulate!` and friends and deprecate the case of unspecified `dims` to make room for proper multi-dimensional unwrapping (#198). Also switch the implementation to use `accumulate!` for simpler and more efficient code.

Modernize unwrap API and prepare for #198

martinholters · 2018-06-05T08:01:32Z

Can you rebase this on top of #198?

platawiec · 2018-06-05T22:12:37Z

Yup, I'll have a look, thanks everyone for getting #201 over the finish line.

Added multi-dimensional unwrap when calling dims=1:N, added relevant keyword arguments, added and fixed tests, fixed issue where range keyword was incorrectly typed.

platawiec · 2018-06-08T23:58:08Z

Alright, I think I've implemented the functionality as @martinholters laid out above. Most of the changes are in my code, but I had to make a few changes in #201 where tests were failing because the range keyword was set to 2pi, causing it to lose precision when dealing with non-Float64 numbers.

I haven't tested against 0.7 yet.

martinholters

Mostly minor things, but the docstring needs some love.

Also, if anyone who is at least a little familiar with the algorithm could take a look, that would be great. But I have no idea who that might be...

martinholters · 2018-06-11T12:05:24Z

src/unwrap.jl

+comparing successive frames of a short-time-fourier-transform, as
+each frame is wrapped to stay within (-pi, pi].
+
+If `dims` is a `Range`, then the data is unwrapped according to


I think this whole docstring needs to be rewritten to make sense. The first two paragraphs and this one just don't fit together.

martinholters · 2018-06-11T12:07:35Z

src/unwrap.jl

+# Arguments
+- `m::AbstractArray{T, N}`: Array to unwrap
+- `range=2pi`: Range of wrapped array
+- `wrap_around=`:  When an element of this tuple is `true`, the


I find wrap_around a bit unfortunate due to the overloaded meaning of "wrap" here. How about circular_dims?

martinholters · 2018-06-11T12:08:17Z

src/unwrap.jl

+- `range=2pi`: Range of wrapped array
+- `wrap_around=`:  When an element of this tuple is `true`, the
+    unwrapping process will consider the edges along the corresponding axis
+    of the image to be connected


"image" is unfortunate. "array"?

martinholters · 2018-06-11T12:09:12Z

src/unwrap.jl

+- `wrap_around=`:  When an element of this tuple is `true`, the
+    unwrapping process will consider the edges along the corresponding axis
+    of the image to be connected
+- `seed=-1`: Unwrapping of arrays with dimension > 1 uses a random initialization. This


Why pass the seed, if you can just manually seed the RNG before calling unwrap? Alternatively, one might wish to pass the RNG.

martinholters · 2018-06-11T12:11:35Z

src/unwrap.jl

+- `seed=-1`: Unwrapping of arrays with dimension > 1 uses a random initialization. This
+    sets the seed of the RNG.
+"""
+function unwrap(m::AbstractArray{T,N}; dims=nothing, range=2T(pi), kwargs...) where {T, N}


No need for the explicit range here, can just be passed on as part of kwargs...

martinholters · 2018-06-11T12:19:45Z

test/unwrap.jl

+        offset = first(f_uw) - first(f_wr)
+        @test (f_wr+offset) ≈ f_uw #oop, 3wrap
+    end
+end


IIUC, calculate_pixel_reliability is special-cased for 2d and 3d. So maybe even test for even more dimensions?

martinholters · 2018-06-18T06:52:25Z

src/unwrap.jl

+    @inbounds sum_val += (wrap_val(pixel_image[pixel_index+pixel_shifts[12]].val - pixel_image[pixel_index].val, range))^2
+    @inbounds sum_val += (wrap_val(pixel_image[pixel_index+pixel_shifts[13]].val - pixel_image[pixel_index].val, range))^2
+    @inbounds sum_val += (wrap_val(pixel_image[pixel_index+pixel_shifts[15]].val - pixel_image[pixel_index].val, range))^2
+    @inbounds sum_val += (wrap_val(pixel_image[pixel_index+pixel_shifts[17]].val - pixel_image[pixel_index].val, range))^2


Omission of 16 here is intentional?

Good eye... not intentional

Have benchmarked whether the manual unrolling is any faster than a for loop?

Yup, the hand-unrolled calculate_reliability function is modestly (~15%) faster, with 2 fewer allocations. This isn't where most of the time spent is, though. When I profiled, most of the time was spent in sort! for moderately-sized arrays (1000x1000) and gather_pixels! for even larger arrays.

When I re-checked my benchmarks, I was able to track down a small type-instability that snuck through.

martinholters · 2018-06-18T06:53:15Z

src/unwrap.jl

+end
+
+A = rand(10)
+unwrap(A; range=10)


Forgot to delete these?

platawiec · 2018-06-25T14:29:01Z

Quick ping, is there anything else you'd like to see?

martinholters

Some nitpicking the docstrings mostly, otherwise LGTM. I'm fairly optimistic we'll be there soon 😄

martinholters · 2018-06-26T07:41:50Z

src/unwrap.jl

+export unwrap, unwrap!
+
+"""
+    unwrap!(m; dims=nothing, range=2pi)


Does not mention all keywords args. Maybe just m; kwargs...?

martinholters · 2018-06-26T07:42:46Z

src/unwrap.jl

+end
+
+"""
+    unwrap!(y, m; dims=nothing, range=2pi)


Same here. Just y, m; kwargs...?

martinholters · 2018-06-26T07:44:42Z

src/unwrap.jl

+        dims = 1
+    end
+    if dims isa Integer
+        Compat.accumulate!(unwrap_kernel(range), y, m, dims=dims)


Should probably check that kwargs is empty and throw an error otherwise.

martinholters · 2018-06-26T07:45:50Z

src/unwrap.jl

+unwrap_kernel(range=2π) = (x, y) -> y - round((y - x) / range) * range
+
+"""
+    unwrap(m; dims=nothing, range=2pi)


Either list all keyword args here or again just kwargs....

martinholters · 2018-06-26T07:46:38Z

src/unwrap.jl

+
+# Arguments
+- `m::AbstractArray{T, N}`: Array to unwrap
+- `range=2pi`: Range of wrapped array


Should add dims to this list, too.

martinholters · 2018-06-26T07:49:55Z

Also, if you could try to eliminate the deprecation warnings on 0.7, that would be appreciated. Otherwise, I can have a go over it after merging.

platawiec · 2018-07-01T16:35:59Z

It's done, 0.7 dep warns are taken care of too.

Thanks for keeping your eye on this, really appreciate the help and fine-tuning!

martinholters · 2018-07-03T07:00:21Z

src/unwrap.jl

+    unwrapping process will consider the edges along the corresponding axis
+    of the array to be connected
+- `rng=GLOBAL_RNG`: Unwrapping of arrays with dimension > 1 uses a random
+    initialization. A user can be pass their own RNG through this argument.


"can be pass"

martinholters · 2018-07-03T07:02:25Z

Thanks for not giving up here! Save for the one type, LGTM now.

martinholters · 2018-07-03T12:20:34Z

@ssfrr, do you want to take another look? Otherwise, I'll merge soonish.

ssfrr · 2018-07-03T13:57:09Z

I've got a pretty long queue ATM and don't want to hold this up any longer, so I say if you're happy with it go for it!

Thanks both of you for bringing it home.

add n-dimensional unwrap

ada5cc5

Pawel and others added 2 commits April 28, 2018 11:28

Deprecate old unwrap signature

4e7e5e3

Merge branch 'master' into unwrap

64dce79

martinholters requested changes May 8, 2018

View reviewed changes

Pawel added 2 commits May 8, 2018 08:50

Add depwarn detail

c35ecc0

Merge branch 'unwrap' of https://github.com/platawiec/DSP.jl into unwrap

162dab0

martinholters mentioned this pull request May 16, 2018

Modernize unwrap API and prepare for #198 #201

Merged

ssfrr added a commit that referenced this pull request Jun 1, 2018

Merge pull request #201 from JuliaDSP/mh/modern_unwrap

f92dcd7

Modernize unwrap API and prepare for #198

Merge JuliaDSP#199 into JuliaDSP#201

947238c

Added multi-dimensional unwrap when calling dims=1:N, added relevant keyword arguments, added and fixed tests, fixed issue where range keyword was incorrectly typed.

martinholters requested changes Jun 11, 2018

View reviewed changes

Improve docstring/interface

32cc545

martinholters reviewed Jun 18, 2018

View reviewed changes

src/unwrap.jl Outdated

end

A = rand(10)

unwrap(A; range=10)

Copy link

Member

martinholters Jun 18, 2018

Choose a reason for hiding this comment

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

Forgot to delete these?

Pawel added 2 commits June 18, 2018 10:01

Fix typos

7a4f937

Fix type-instability

5b5b810

martinholters requested changes Jun 26, 2018

View reviewed changes

Pawel added 3 commits July 1, 2018 10:15

interface updates

c50e824

Fix 0.7 depwarns

185faed

Clean docstrings

de70b85

martinholters reviewed Jul 3, 2018

View reviewed changes

martinholters approved these changes Jul 3, 2018

View reviewed changes

Fix docstring typos

0eb073f

martinholters merged commit 6e8901b into JuliaDSP:master Jul 4, 2018

add n-dimensional unwrap #198

add n-dimensional unwrap #198

Conversation

platawiec commented Apr 15, 2018

martinholters commented Apr 17, 2018

platawiec commented Apr 17, 2018

platawiec commented Apr 28, 2018

Choose a reason for hiding this comment

platawiec commented May 8, 2018 • edited Loading

martinholters commented May 8, 2018

platawiec commented May 8, 2018

martinholters commented May 9, 2018

platawiec commented May 9, 2018

ssfrr commented May 9, 2018

platawiec commented May 9, 2018

ssfrr commented May 9, 2018

martinholters commented May 14, 2018

martinholters commented Jun 5, 2018

platawiec commented Jun 5, 2018

platawiec commented Jun 8, 2018

martinholters left a 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

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

Choose a reason for hiding this comment

platawiec commented Jun 25, 2018

martinholters left a 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

Choose a reason for hiding this comment

martinholters commented Jun 26, 2018

platawiec commented Jul 1, 2018

Choose a reason for hiding this comment

martinholters commented Jul 3, 2018

martinholters commented Jul 3, 2018

ssfrr commented Jul 3, 2018

platawiec commented May 8, 2018 •

edited

Loading