search

JuliaGPU / Adapt.jl

Other
86 stars 24 forks source link

Implement missing LinearAlgebra wrappers and add support for uplo parameter #51

Closed danielwe closed 8 months ago

danielwe commented 2 years ago

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 commented 2 years ago

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.

maleadt commented 2 years ago

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?

danielwe commented 2 years ago

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

maleadt commented 2 years ago

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

codecov[bot] commented 2 years ago

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 ```diff @@ 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](https://app.codecov.io/gh/JuliaGPU/Adapt.jl/pull/51?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGPU) | Coverage Δ | | |---|---|---| | [src/wrappers.jl](https://app.codecov.io/gh/JuliaGPU/Adapt.jl/pull/51?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=JuliaGPU#diff-c3JjL3dyYXBwZXJzLmps) | `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.

danielwe commented 2 years ago

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?

maleadt commented 2 years ago

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

mtfishman commented 8 months ago

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

mtfishman commented 8 months ago

Thanks!

maleadt commented 8 months ago

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