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.
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.
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.
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.
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.
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.
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.
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
6Not 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.
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.
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.
martinholters
force-pushed
the
mh/modern_unwrap
branch
from
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 unspecifieddimsto 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 defaultdimsforunwrap!until we remove that deprecation and then use the same default in both.