Support differing x,y eltypes for dwt #54
Conversation
src/mod/Transforms.jl
Outdated
| @@ -484,7 +484,7 @@ end # begin | |||
| end # for | |||
|
|
|||
| # Array with shared memory | |||
| function unsafe_vectorslice(A::Array{T}, i::Int, n::Int) where T | |||
| function unsafe_vectorslice(A::AbstractArray{T}, i::Int, n::Int) where T | |||
There was a problem hiding this comment.
I am not sure if this is safe in general. I think A must be contiguous. Do you know if there is a way to assert that?
There was a problem hiding this comment.
I admit that this was the one change that made me nervous, so nice catch :)
Here's what I know:
(@view ones(3,4)[1:2,3:4]) isa DenseArray
is false so requiring DenseArray would be too restrictive.
In contrast:
(@view ones(3,4)[1:2,3:4]) isa StridedArray
returns true so I think StridedArray would suffice instead of AbstractArray here.
Would this routine work for a strided array?
If not, then perhaps we should dispatch by Array and StridedArray.
I would be glad to refine the PR to address this, but I need some insight into what is going on here.
Could perhaps this be done with @view A[i:(i-1+n)] instead of the unsafe_wrap?
To make that work I'll need to replace additional Arrays with AbstractArrays and I want to see if you know already that it will fail (maybe you tried it?) before I make all those changes.
There was a problem hiding this comment.
I changed it to use @view and replaced several ::Vector with a new type FVector corresponding to::StridedVector (giving it a name makes it easy to change it back or to try other types).
With these changes, all but 1 of the 319 tests pass. The one that fails (in WPT) gives the error
BoundsError: attempt to access 8-element Array{Float64,1} at index [1:16]
I have a hunch that this is a bug in the WPT code that went unnoticed because of all the unsafe stuff.
Anyone who understands the WPT code willing to take a look?
Here is the runtest output:
Test Summary: | Pass Total
Util | 118 118
WPT: Error During Test at /Users/fessler/.julia/dev/Wavelets/test/transforms.jl:294
Got exception outside of a @test
BoundsError: attempt to access 8-element Array{Float64,1} at index [1:16]
Stacktrace:
[1] throw_boundserror(::Array{Float64,1}, ::Tuple{UnitRange{Int64}}) at ./abstractarray.jl:537
[2] checkbounds at ./abstractarray.jl:502 [inlined]
[3] view at ./subarray.jl:163 [inlined]
[4] unsafe_vectorslice at /Users/fessler/.julia/dev/Wavelets/src/mod/Transforms.jl:492 [inlined]
[5] _wpt!(::Array{Float64,1}, ::Array{Float64,1}, ::OrthoFilter{Wavelets.WT.PerBoundary}, ::BitArray{1}, ::Bool, ::Array{Float64,1}, ::Array{Float64,1}, ::Array{Float64,1}, ::Array{Float64,1}) at /Users/fessler/.julia/dev/Wavelets/src/mod/transforms_filter.jl:335
[6] _wpt!(::Array{Float64,1}, ::Array{Float64,1}, ::OrthoFilter{Wavelets.WT.PerBoundary}, ::BitArray{1}, ::Bool) at /Users/fessler/.julia/dev/Wavelets/src/mod/transforms_filter.jl:307
[7] wpt at /Users/fessler/.julia/dev/Wavelets/src/mod/Transforms.jl:420 [inlined]
[8] wpt at /Users/fessler/.julia/dev/Wavelets/src/mod/Transforms.jl:414 [inlined]
[9] macro expansion at /Users/fessler/.julia/dev/Wavelets/test/transforms.jl:299 [inlined]
[10] macro expansion at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.4/Test/src/Test.jl:1113 [inlined]
[11] top-level scope at /Users/fessler/.julia/dev/Wavelets/test/transforms.jl:295
[12] include(::String) at ./client.jl:439
[13] top-level scope at /Users/fessler/.julia/dev/Wavelets/test/runtests.jl:29
[14] top-level scope at /Users/julia/buildbot/worker/package_macos64/build/usr/share/julia/stdlib/v1.4/Test/src/Test.jl:1113
[15] top-level scope at /Users/fessler/.julia/dev/Wavelets/test/runtests.jl:29
[16] include(::String) at ./client.jl:439
[17] top-level scope at none:6
[18] eval(::Module, ::Any) at ./boot.jl:331
[19] exec_options(::Base.JLOptions) at ./client.jl:264
[20] _start() at ./client.jl:484
Test Summary: | Pass Error Total
Transforms | 318 1 319
Accuracy | 60 60
Accuracy non-square | 1 1
Lifting vs filter | 72 72
Types and sizes | 19 19
Errors | 5 5
Continuous Wavelet Transform | 144 144
WPT | 1 1
MODWT | 4 4
Wavelet types | 3 3
ERROR: LoadError: Some tests did not pass: 318 passed, 0 failed, 1 errored, 0 broken.
in expression starting at /Users/fessler/.julia/dev/Wavelets/test/runtests.jl:29
ERROR: Package Wavelets errored during testing
There was a problem hiding this comment.
Thanks for that. This is from around julia 0.5 era and I haven't had time to modernize the code. I will try to look at the wpt.
There was a problem hiding this comment.
I think I found the bug. One thing I would like you to fix additionally is to make an overload of this function as it was in addition to this, since the unsafe version is a bit faster.
There was a problem hiding this comment.
Ok I pulled your changed and then it passed the tests - yay!
I added back the Array version as you suggested and the tests still pass, so we're good.
Possibly the code coverage will be slightly slower because the tests may not exercise the @views version, but hopefully that is a minor issue for another day.
After you accept this could you please bump the version? Thanks!
There was a problem hiding this comment.
I added [compat] entry because I think that is needed for the @JuliaRegistrator register() bot to accept it, and I bumped the version, hoping to save you one step :)
Addresses #53 for the
dwt!andidwt!(but not for other transforms).It passes
test Wavelets.In addition to giving more flexibility with the
eltypeofxandy, I also made some typesAbstractArraybecause I am calling these withreshapeandviewsand a view is anAbstractMatrixbut not aMatrix.