-
Notifications
You must be signed in to change notification settings - Fork 110
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
add n-dimensional unwrap #198
Conversation
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 |
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
|
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. |
src/unwrap.jl
Outdated
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."), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. |
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 Another thing to note is that before this PR, the 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? |
I had tried something with the
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 |
I'd say what you call the "intended use case" is not handled by the current
The mutating version should probably take input and output arguments, which may be equal for true in-place operation. This suggests the following route: If we can agree on something like this, I could take care of 1a while you think of a nice name for 1b 😄 |
Sounds like a plan, very clear thanks. So the multi-dimensional unwrap should look something like |
I haven't looked into the code deeply yet, but from an API standpoint I support a That said, I think it's OK to currently only support the 1D case where So my vote would be for an interface like:
I'm on-board with a 2-step deprecation where One alternative to a separate function as a side note: |
Great discussion points, thanks @ssfrr . I think the analogy to Just wanted to point out, for educational reason:
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. ;) |
Yes, that's much better. So the revised plan: |
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.
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.
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
Can you rebase this on top of #198? |
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.
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 I haven't tested against 0.7 yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
src/unwrap.jl
Outdated
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this whole docstring needs to be rewritten to make sense. The first two paragraphs and this one just don't fit together.
src/unwrap.jl
Outdated
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find wrap_around
a bit unfortunate due to the overloaded meaning of "wrap" here. How about circular_dims
?
src/unwrap.jl
Outdated
- `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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"image" is unfortunate. "array"?
src/unwrap.jl
Outdated
- `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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why pass the seed, if you can just manually seed the RNG before calling unwrap
? Alternatively, one might wish to pass the RNG.
src/unwrap.jl
Outdated
- `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} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for the explicit range
here, can just be passed on as part of kwargs...
test/unwrap.jl
Outdated
offset = first(f_uw) - first(f_wr) | ||
@test (f_wr+offset) ≈ f_uw #oop, 3wrap | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIUC, calculate_pixel_reliability
is special-cased for 2d and 3d. So maybe even test for even more dimensions?
src/unwrap.jl
Outdated
@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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Omission of 16 here is intentional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good eye... not intentional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have benchmarked whether the manual unrolling is any faster than a for loop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/unwrap.jl
Outdated
end | ||
|
||
A = rand(10) | ||
unwrap(A; range=10) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to delete these?
Quick ping, is there anything else you'd like to see? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicking the docstrings mostly, otherwise LGTM. I'm fairly optimistic we'll be there soon 😄
src/unwrap.jl
Outdated
export unwrap, unwrap! | ||
|
||
""" | ||
unwrap!(m; dims=nothing, range=2pi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not mention all keywords args. Maybe just m; kwargs...
?
src/unwrap.jl
Outdated
end | ||
|
||
""" | ||
unwrap!(y, m; dims=nothing, range=2pi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. Just y, m; kwargs...
?
dims = 1 | ||
end | ||
if dims isa Integer | ||
Compat.accumulate!(unwrap_kernel(range), y, m, dims=dims) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably check that kwargs
is empty and throw an error otherwise.
src/unwrap.jl
Outdated
unwrap_kernel(range=2π) = (x, y) -> y - round((y - x) / range) * range | ||
|
||
""" | ||
unwrap(m; dims=nothing, range=2pi) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Either list all keyword args here or again just kwargs...
.
src/unwrap.jl
Outdated
|
||
# Arguments | ||
- `m::AbstractArray{T, N}`: Array to unwrap | ||
- `range=2pi`: Range of wrapped array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should add dims
to this list, too.
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. |
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! |
src/unwrap.jl
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"can be pass"
Thanks for not giving up here! Save for the one type, LGTM now. |
@ssfrr, do you want to take another look? Otherwise, I'll merge soonish. |
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. |
Ref #196
I've added the n-dimensional
unwrap
fromUnwrap.jl
here. The doc-string forunwrap
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
:dim
keyword, a user who wants to recover that functionality will have to pass the appropriateview
s.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 intounwrap
has values which lie outside the range, then they should do.%
before callingunwrap
. Typically, the array passed in is already module some number, because that is whyunwrap
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.