-
Notifications
You must be signed in to change notification settings - Fork 10
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
Added support for CuArray via Adapt.jl #67
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 <[email protected]>
Co-authored-by: Pietro Vertechi <[email protected]>
Co-authored-by: Pietro Vertechi <[email protected]>
... there were a couple of typos. Now the |
This second attempt, uses a very light-weight implementation to add
CuArray
support toShiftedArrays.jl
.It is based on using an Extension Package, such that
ShiftedArrays
does not drag in any extra packages, but ifCUDA.jl
is present, the adaptation is used.Note that the
show()
function was specified by CuArrays preventing errors via the@allowscalar
macro.The tests were extended to CuArray usage, but this has to be enabled manually in the
runtests.jl
file.