Add native GPU support. #504
Conversation
Codecov Report
@@ Coverage Diff @@
## master #504 +/- ##
==========================================
+ Coverage 86.91% 87.10% +0.18%
==========================================
Files 27 28 +1
Lines 1819 1845 +26
==========================================
+ Hits 1581 1607 +26
Misses 238 238
Continue to review full report at Codecov.
|
|
I'm a bit wary of dependency creep. Is there a way to do this via a subpackage? |
|
The additional dependencies are quite small (loading time from 0.74s to 0.79s in my machine). But they request bumping Julia compat to 1.6, which is okay as well (I think). Either living in here or in the new interpolationsGPU would be great! cc: @timholy @ChantalJuntao (from JuliaImages/ImageTransformations.jl#156) |
|
In particular, can you please update this document: |
|
@mkitti devdoc has been updated (including the new |
docs/src/devdocs.md
Outdated
| To achieve this, an `ITP <: AbstractInterpolation` should define it's own `Adapt.adapt_structure(to, itp::ITP)`, which constructs a new `ITP` with the adapted | ||
| fields (`adapt(to, itp.fieldname)`) of `itp`. The field adaption could be skipped | ||
| if we know that it has been GPU-compatable, e.g. a bit range. |
There was a problem hiding this comment.
| To achieve this, an `ITP <: AbstractInterpolation` should define it's own `Adapt.adapt_structure(to, itp::ITP)`, which constructs a new `ITP` with the adapted | |
| fields (`adapt(to, itp.fieldname)`) of `itp`. The field adaption could be skipped | |
| if we know that it has been GPU-compatable, e.g. a bit range. | |
| To achieve this, an `ITP <: AbstractInterpolation` should define its own `Adapt.adapt_structure(to, itp::ITP)`. | |
| `adapt_structure` constructs a new `ITP` with the adapted fields of `itp` as defined by `adapt(to, itp.fieldname)`. The field adaption could be skipped if we know that it is been GPU-compatable, e.g. a bit range. |
Also what do you mean by "bit range" here. Do you mean bit type where isbitstype is true?
There was a problem hiding this comment.
Yes, has been replaced with "a isbit range"
src/Interpolations.jl
Outdated
| Base.@propagate_inbounds Base.getindex(A::InterpGetindex{N}, I::Vararg{Union{Int,WeightedIndex},N}) where {N} = | ||
| interp_getindex(A.coeffs, I, ntuple(d->0, Val(N))...) | ||
|
|
||
| # TODO: should we drop the following injection? |
There was a problem hiding this comment.
Can you elaborate? Is this fully obsolete now?
There was a problem hiding this comment.
Base._getindex is an internal api anyway. Since Interpolations.jl itself no-longer use it. It makes sense to me to dop it.
Co-Authored-By: Mark Kittisopikul <[email protected]>
This should omit the `isempty` check if inference give good result. fix test on 1.6, `range(2, 19.0, 101)` was introduced in 1.7
and remove unneeded `@propagate_inbounds`.
And fix the inference problem.
|
Is this ready to merge? |
|
Yes, CI passed. The test also passed locally with 1.6.7. |
This is kind of late, but was it OK to merge this without tests for 3D? And I thought that Cl's tests don't cover GPU code? |
|
Feel free to submit another pull request. As far as I know there are no GPUs to run CI on here. |
Only 1d and 2dBSplineInterpolations are tested though.Test has been added for
GriddedandLanczos.Since this overload needsGPUArraysCore.jl, we need to drop < 1.6 compatibility if we don't want to add more@require.Base.getindexis overloaded with aInterpGetIndexwrapper in the latest design. Thus there's no need to depend onGPUArraysCore.jl. (We can keep < 1.6 compatibility now)