Adapt.jl
Adapt.jl copied to clipboard
Implement missing LinearAlgebra wrappers and add support for uplo parameter
Finally got around to #46 (long since closed but never fixed)
Bonus: I noticed that the existing support for Symmetric
was buggy as it didn't support the uplo
parameter, i.e., adapt_structure
would do the wrong thing with an instance of Symmetric
with non-default uplo
. So fixed that too for Symmetric and Hermitian.
Reading the docstring for WrappedArray
, shouldn't the {D,Bid,Trid,SymTrid}iagonal
wrappers use the Src
type parameter rather than Dst
? They wrap 1-3 vectors and turn them into a matrix, so the wrapper has different dimensionality from the parent(s).
In this PR I just followed the pattern already in use for {D,Trid}iagonal
, so I used Dst
for {Bi,SymTri}diagonal
.
Thanks for the PR!
Reading the docstring for
WrappedArray
, shouldn't the{D,Bid,Trid,SymTrid}iagonal
wrappers use theSrc
type parameter rather thanDst
? They wrap 1-3 vectors and turn them into a matrix, so the wrapper has different dimensionality from the parent(s).
It seems that way, yes. Can you come up with a test to make sure this doesn't regress?
I think that should do it. Testing that all wrapped arrays are subtypes of the appropriate dimension-specific AnyCustomArray{T,N} union.
Thanks. Did you validate these changes with any of the packages that rely on Adapt.jl (like CUDA.jl)?
Codecov Report
Attention: 12 lines
in your changes are missing coverage. Please review.
Comparison is base (
5063b0f
) 83.33% compared to head (d29a1e8
) 77.77%. Report is 6 commits behind head on master.
Additional details and impacted files
@@ Coverage Diff @@
## master #51 +/- ##
==========================================
- Coverage 83.33% 77.77% -5.56%
==========================================
Files 6 6
Lines 72 81 +9
==========================================
+ Hits 60 63 +3
- Misses 12 18 +6
Files | Coverage Δ | |
---|---|---|
src/wrappers.jl | 64.44% <53.84%> (-7.78%) |
:arrow_down: |
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Thanks for the reminder. The entire CUDA.jl test suite passes on my end.
Looks like UpperHessenberg only exists from 1.3 onwards. Is it important to maintain support for 1.2?
No. We can bump the requirement to the latest LTS, 1.6, and then also include the other recent versions that are missing from the CI roster (1.6, 1.7, 1.8-beta1).
Can this be merged? I just hit a scalar indexing issue caused by missing adapt rules for Hermitian
.
Thanks!
Sorry, reverting this due to CUDA.jl breakage: https://github.com/JuliaGPU/Adapt.jl/pull/70#issue-1964143921