Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing support from AbstractArrays to AnyArrays for tril! and triu! functions in LinAlg + support for printarray and getproperty of QR #458

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

evelyne-ringoot
Copy link
Contributor

@evelyne-ringoot evelyne-ringoot commented Mar 7, 2023

Changing this for triu and tril, adding support for QRQ for getproperty and printarray

@maleadt
Copy link
Member

maleadt commented Mar 8, 2023

Those mul! definitions are there to avoid ambiguities, so I sadly think we need them. I'm also not sure introducing AnyGPUArray everywhere is wanted... That introduces large unions, and in the case of mul! it's already the slowest method by far to insert when loading the package (JuliaLang/julia#48092).

Until we have something like JuliaLang/julia#48861 for mul! I'm not sure we can reasonably do this.

Thoughts, @vchuravy?

@evelyne-ringoot evelyne-ringoot force-pushed the QR_views branch 2 times, most recently from a1d4388 to fba39d8 Compare March 8, 2023 16:22
@maleadt
Copy link
Member

maleadt commented Mar 8, 2023

[Reverting 2 commits](/JuliaGPU/GPUArrays.jl/pull/458/commits/fba39d88f31215619c4b1691525750e44f2ed8b6)

I don't think you've reverted anything. Can you rebase on top of latest master too, when you're at it?

@evelyne-ringoot
Copy link
Contributor Author

@maleadt : I removed the mul functionality for this pull request. I only need it for tril! and triu! functions for JuliaGPU/CUDA.jl#1764. I added the getproperty and printarray function for QR as well.

@evelyne-ringoot evelyne-ringoot changed the title Changing support from AbstractArrays to AnyArrays for certain functions in LinAlg Changing support from AbstractArrays to AnyArrays for tril! and triu! functions in LinAlg + support for printarray and getproperty of QR Mar 9, 2023
@evelyne-ringoot evelyne-ringoot marked this pull request as ready for review March 10, 2023 03:41
@maleadt
Copy link
Member

maleadt commented Mar 10, 2023

Yeah that's much less invasive and should be fine

The CUDA.jl CI failure as part of the CUSOLVER qr tests looks related though?

@maleadt
Copy link
Member

maleadt commented Sep 6, 2023

The error persist. Did you expect it to disappear (given the CI restarts)?

It would also be good to have some tests, for the getproperty method at least.

@maleadt
Copy link
Member

maleadt commented Jul 5, 2024

Squashed and rebased.

I'm confused how this is supposed to work though. You're adding functionality for getproperty on a QR factorization, however, GPUArrays doesn't have any functionality for constructing a QR factorization in the first place. Base uses geqrt, CUDA.jl uses geqrf, neither of which are available here generically, and it's also not part of the 'contract' of APIs that are supposed to be provided by back-ends.

Which back-ends are you interested in, specifically? Maybe it's better to focus first on adding the change there (i.e., in CUDA.jl) before making it generic?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants