-
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
Modernize unwrap API and prepare for #198 #201
Conversation
Can you give this a look, @platawiec and @ssfrr? |
src/util.jl
Outdated
@@ -29,46 +30,58 @@ export unwrap!, | |||
shiftin! | |||
|
|||
""" | |||
unwrap!(m, dim=ndims(m); range=2pi) | |||
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.
I understand the value of separating the src and dest arrays, but is there a reason we can't also keep the 1-arg in-place version? We could define a fallback like:
unwrap!(m::AbstractArray; kwargs...) = unwrap!(m, m; kwargs...)
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.
Sure, we can easily keep them.
src/util.jl
Outdated
@deprecate(unwrap!(m::Array{T}; range::Number=2pi) where T<:AbstractFloat, | ||
unwrap!(m, m, dims=ndims(m), range=range)) | ||
@deprecate(unwrap!(m::Array{T}, dim::Integer; range::Number=2pi) where T<:AbstractFloat, | ||
unwrap!(m, m, dims=dim, range=range)) |
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.
These should be AbstractVector
/ AbstractArray
, I think. Technically this is more general than the original definition that's being deprecated, but if someone calls this with some AbstractArray
subtype I think the deprecation warning will be more useful than a MethodError
.
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.
Probably won't make much of a difference in practice, but yeah, I'll widen the signatures.
src/util.jl
Outdated
# println("slice_tuple: ", slice_tuple) | ||
# println("m[slice_tuple...]: ", m[slice_tuple...]) | ||
m[slice_tuple...] = m[slice_tuple...] - offset | ||
if dims isa Integer |
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 wonder if it's cleaner to use dispatch to handle the Vector
/ dims
logic, though I guess to do that would require an internal _unwrap!
function that passed dims
as a positional arg rather than a keyword arg. So maybe not worth it.
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.
That was my thinking. Might be reconsidered as part of #198.
src/util.jl
Outdated
unwrap_kernel(range=2π) = (x, y) -> y - round((y - x) / range) * range | ||
|
||
@deprecate(unwrap!(m::Vector{T}; range::Number=2pi) where T<:AbstractFloat, | ||
unwrap!(m, m, range=range)) |
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.
remove this deprecation if you end up going with the in-place fallback suggestion above.
Sounds good. I'm 👍 to merge with the tweaks above |
Ok, I think I have addressed all your comments. I've widened the type signatures to just julia> unwrap([1,2,3,14,15,6], range=10)
6-element Array{Int64,1}:
1
2
3
4
5
6 Not that this seems like a particularly useful feature, but why disallow it?
We could make accumulate(DSP.Util.unwrap_kernel(), [1,2,3,14,15,6], dims=1) It returns an |
src/util.jl
Outdated
unwrap!(copy(m), args...; kwargs...) | ||
function unwrap(m::AbstractArray{<:Any,N}; dims=nothing, range=2pi) where {N} | ||
if dims === nothing && N != 1 | ||
Base.depwarn("`unwrap(m):AbstractArray` is deprecated, use `unwrap(m, dims=ndims(m))` instead", :unwrap) |
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.
whoops, I missed this last time - should this be unwrap(m::AbstractArray)
rather than unwrap(m):AbstractArray
?
Other than that I think this is good to merge.
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 catch! I probably had too little coffee before reworking the deprecation messages for the widened type signatures. Update incoming...
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.
6781ec4
to
4e0535c
Compare
Ok, one (hopefully) last look? |
Something weird is going on with coverage - most of the lines it says are uncovered seem to be unrelated to this PR. I don't have time to look into it now but hopefully it'll resolve itself. |
Thanks, having a second pair of eyes here definitely was worth it. I think there is something fishy with to coverage data on 0.7, but I haven't looked at the details, yet. |
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.
Make the API of
unwrap
/unwrap!
similar to the one ofaccumulate
/accumulate!
and friends and deprecate the case of unspecifieddims
to make room for proper multi-dimensional unwrapping(#198), CC @platawiec.
Also switch the implementation to use
accumulate!
for simpler and more efficient code.I've realized that
unwrap!(y, x)
could actually use defaultdims=1:ndims(x)
right now (well, once the functionality of #198 is in), but that would be very inconsistent withunwrap(x)
defaulting todims=ndims(x)
(with a deprecation warning). So I suggest we provide no defaultdims
forunwrap!
until we remove that deprecation and then use the same default in both.