Allow transfer of array views backed by contiguous memory (fix #76) #77
Conversation
|
This approach seems to work okay, but the use case may be a bit niche. @maleadt if you have an appetite to merge this I will add tests. |
src/array.jl
Outdated
| if length(dst) != length(src) | ||
| throw(ArgumentError("Inconsistent array length.")) | ||
| end | ||
| if any(strides(src) .!= 1) |
There was a problem hiding this comment.
This will fail for any multidimensional array, where even a dense array has a stride bigger than one along higher dimensions.
strides is not entirely safe yet, but see JuliaLang/julia#25321 (which should make it safe). If you rely on an unsafe strides, you might get wrong answers or segfaults.
There was a problem hiding this comment.
@timholy good point, thanks. I was concentrating on the particular problem I have in #76 and hadn't properly considered this.
I could fix this to work for views which only restrict the trailing dimension to a contiguous region, but that's too peculiar. Alternatively, I could restrict the method only to operate on views which result in a vector, which would still be useful.
That said, if strides isn't safe, then that might be a showstopper. Feel free to close the PR if that's the case - I can always implement these methods privately in my package until things stabilise.
No objections per se, but since we're transitioning away from CUDAdrv.CuArrays (just waiting for 0.7 to stabilize and be supported by GPUArrays) it might be better to add this to CuArrays.jl. |
|
I haven't yet started the transition to 0.7, so it would be useful to me if this were available here whilst I am still working on 0.6/updating packages. I suspect a more generic/well planned interface which takes account of various array types would be better for the future, and have seen a few related issues and PRs at CuArrays.jl. For the moment, I have updated the PR to attempt to ensure that it only attempts to copy views which represent a contiguous memory backed region, and added some tests accordingly. @timholy does this address your concerns, or is it still problematic? |
samuelpowell
changed the title
|
Yes, this seems safe. |
samuelpowell
changed the title
samuelpowell
changed the title
|
Thanks @timholy |
|
Thanks. CI failure unrelated, probably because this PR isn't based on master. |
|
Hi, I am having a similar problem. Is it really solved? julia> phi0 , flag = @time IterativeSolvers.gmres(Jl,v,tol = 1e-6,log = true,verbose=true,restart=200)
ERROR: conversion to pointer not defined for CuArray{Float32,1}
Stacktrace:
[1] copy!(::SubArray{Float32,1,Array{Float32,2},Tuple{Base.Slice{Base.OneTo{Int64}},Int64},true}, ::CuArray{Float32,1}) at /user/rveltz/home/.julia/v0.6/GPUArrays/src/abstractarray.jl:120
[2] #init!#55(::Bool, ::Function, ::IterativeSolvers.ArnoldiDecomp{Float32,LinearMaps.FunctionMap{Float32,JacobianNF,Void}}, ::Array{Float32,1}, ::CuArray{Float32,1}, ::IterativeSolvers.Identity, ::Array{Float32,1}) at /user/rveltz/home/.julia/v0.6/IterativeSolvers/src/gmres.jl:219
[3] (::IterativeSolvers.#kw##init!)(::Array{Any,1}, ::IterativeSolvers.#init!, ::IterativeSolvers.ArnoldiDecomp{Float32,LinearMaps.FunctionMap{Float32,JacobianNF,Void}}, ::Array{Float32,1}, ::CuArray{Float32,1}, ::IterativeSolvers.Identity, ::Array{Float32,1}) at ./<missing>:0
[4] #gmres_iterable!#52(::IterativeSolvers.Identity, ::IterativeSolvers.Identity, ::Float64, ::Int64, ::Int64, ::Bool, ::Function, ::Array{Float32,1}, ::LinearMaps.FunctionMap{Float32,JacobianNF,Void}, ::CuArray{Float32,1}) at /user/rveltz/home/.julia/v0.6/IterativeSolvers/src/gmres.jl:118
[5] (::IterativeSolvers.#kw##gmres_iterable!)(::Array{Any,1}, ::IterativeSolvers.#gmres_iterable!, ::Array{Float32,1}, ::LinearMaps.FunctionMap{Float32,JacobianNF,Void}, ::CuArray{Float32,1}) at ./<missing>:0
[6] #gmres!#54(::IterativeSolvers.Identity, ::IterativeSolvers.Identity, ::Float64, ::Int64, ::Int64, ::Bool, ::Bool, ::Bool, ::IterativeSolvers.#gmres!, ::Array{Float32,1}, ::LinearMaps.FunctionMap{Float32,JacobianNF,Void}, ::CuArray{Float32,1}) at /user/rveltz/home/.julia/v0.6/IterativeSolvers/src/gmres.jl:185
[7] (::IterativeSolvers.#kw##gmres!)(::Array{Any,1}, ::IterativeSolvers.#gmres!, ::Array{Float32,1}, ::LinearMaps.FunctionMap{Float32,JacobianNF,Void}, ::CuArray{Float32,1}) at ./<missing>:0
[8] #gmres#53(::Array{Any,1}, ::Function, ::LinearMaps.FunctionMap{Float32,JacobianNF,Void}, ::CuArray{Float32,1}) at /user/rveltz/home/.julia/v0.6/IterativeSolvers/src/gmres.jl:134
[9] (::IterativeSolvers.#kw##gmres)(::Array{Any,1}, ::IterativeSolvers.#gmres, ::LinearMaps.FunctionMap{Float32,JacobianNF,Void}, ::CuArray{Float32,1}) at ./<missing>:0 |
|
It was certainly working for me at the time, though I haven't used it recently. I suggest opening an issue with a minimum working example and xref accordingly. |
|
Thank you for your answer. I will do as suggested.
Best regards,
Romain
… On Jul 6, 2018, at 16:47, Samuel Powell ***@***.***> wrote:
It was certainly working for me at the time, though I haven't used it recently. I suggest opening an issue with a minimum working example and xref accordingly.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub <#77 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABhKBKOlCCpns3jUuxnhRQwt7Rr-lPMdks5uD3iXgaJpZM4RS79L>.
|
Ref #76