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}iagonalwrappers use theSrctype 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