Added support for CuArray via Adapt.jl #67
Conversation
ext/CUDASupportExt.jl
Outdated
| function Base.show(io::IO, mm::MIME"text/plain", cs::CircShiftedArray) | ||
| CUDA.@allowscalar invoke(Base.show, Tuple{IO, typeof(mm), AbstractArray}, io, mm, cs) | ||
| end | ||
|
|
||
| function Base.show(io::IO, mm::MIME"text/plain", cs::ShiftedArray) | ||
| CUDA.@allowscalar invoke(Base.show, Tuple{IO, typeof(mm), AbstractArray}, io, mm, cs) |
There was a problem hiding this comment.
This part makes me a little uncomfortable (it's type-piracy in some sense), maybe it's best to remove it for the time being and accept that showing errors and we can see how to fix it later. How do other wrapping packages do it? Should we maybe just depend on GPUArraysCore and take @allowscalar from there?
There was a problem hiding this comment.
I agree. I will commend this out for the time being and test your other great suggestions,
| use_cuda = false; # set this to true to test ShiftedArrays for the CuArray datatype | ||
| if (use_cuda) | ||
| using CUDA | ||
| CUDA.allowscalar(true); # needed for some of the comparisons | ||
| end |
There was a problem hiding this comment.
Maybe we could use https://github.com/JuliaGPU/GPUArrays.jl/blob/4278412a6b9b1d859c290232a9f8223eb4416d1e/lib/JLArrays/src/JLArrays.jl#L6 to test without a GPU? Also, maybe we can try and use @allowscalar to selectively allow scalar indexing where it's needed.
There was a problem hiding this comment.
I did not want CUDA to be a dependency on testing, if we move @allowscalar outside the if clause, we would need to always have CUDA loaded for testing. If JLArrays.jl is a lightweight way of doing this, then this may be a good idea.
|
Thanks for the PR, I've left a review with some suggestions. I'll be traveling for the holidays so I might be slow in responding for the next couple of weeks. |
Co-authored-by: Pietro Vertechi <pietro.vertechi@protonmail.com>
Co-authored-by: Pietro Vertechi <pietro.vertechi@protonmail.com>
Co-authored-by: Pietro Vertechi <pietro.vertechi@protonmail.com>
|
... there were a couple of typos. Now the |
This second attempt, uses a very light-weight implementation to add
CuArraysupport toShiftedArrays.jl.It is based on using an Extension Package, such that
ShiftedArraysdoes not drag in any extra packages, but ifCUDA.jlis present, the adaptation is used.Note that the
show()function was specified by CuArrays preventing errors via the@allowscalarmacro.The tests were extended to CuArray usage, but this has to be enabled manually in the
runtests.jlfile.