Adapt.jl icon indicating copy to clipboard operation
Adapt.jl copied to clipboard

Implement missing LinearAlgebra wrappers and add support for uplo parameter

Open danielwe opened this issue 2 years ago • 7 comments

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.

danielwe avatar Mar 18 '22 22:03 danielwe

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.

danielwe avatar Mar 19 '22 20:03 danielwe

Thanks for the PR!

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).

It seems that way, yes. Can you come up with a test to make sure this doesn't regress?

maleadt avatar Mar 21 '22 07:03 maleadt

I think that should do it. Testing that all wrapped arrays are subtypes of the appropriate dimension-specific AnyCustomArray{T,N} union.

danielwe avatar Mar 24 '22 07:03 danielwe

Thanks. Did you validate these changes with any of the packages that rely on Adapt.jl (like CUDA.jl)?

maleadt avatar Mar 24 '22 07:03 maleadt

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.

codecov[bot] avatar Mar 24 '22 07:03 codecov[bot]

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?

danielwe avatar Mar 24 '22 08:03 danielwe

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).

maleadt avatar Mar 24 '22 08:03 maleadt

Can this be merged? I just hit a scalar indexing issue caused by missing adapt rules for Hermitian.

mtfishman avatar Oct 23 '23 12:10 mtfishman

Thanks!

mtfishman avatar Oct 23 '23 15:10 mtfishman

Sorry, reverting this due to CUDA.jl breakage: https://github.com/JuliaGPU/Adapt.jl/pull/70#issue-1964143921

maleadt avatar Oct 26 '23 18:10 maleadt