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

Add and fix some aqua tests #1965

Merged
merged 7 commits into from
Jun 16, 2023
Merged

Conversation

charleskawczynski
Copy link
Contributor

This PR adds and fixes some aqua tests. Closes #1964.

@maleadt
Copy link
Member

maleadt commented Jun 15, 2023

Can you explain why some of these changes are necessary? They only seem to add duplicate definitions with manually-split type signatures, which doesn't seem great.

@charleskawczynski
Copy link
Contributor Author

charleskawczynski commented Jun 15, 2023

Can you explain why some of these changes are necessary? They only seem to add duplicate definitions with manually-split type signatures, which doesn't seem great.

Sure, here's an explanation based on my best understanding. Let's take this fix as an example:

master

LinearAlgebra.triu(A::Union{$SparseMatrixType{T,M}, Transpose{T,<:$SparseMatrixType}, Adjoint{T,<:$SparseMatrixType}}) where {T,M} = 
    $SparseMatrixType( triu(CuSparseMatrixCOO(A), 0) )

fix

LinearAlgebra.triu(A::$SparseMatrixType{T,M}) where {T,M} = 
    $SparseMatrixType( triu(CuSparseMatrixCOO(A), 0) )
LinearAlgebra.triu(A::Union{Transpose{T,<:$SparseMatrixType}, Adjoint{T,<:$SparseMatrixType}}) where {T} = 
    $SparseMatrixType( triu(CuSparseMatrixCOO(A), 0) )

In the main commit, Julia creates methods for each element of the Union. In the case of the second element, the created method is:

LinearAlgebra.triu(A::Transpose{T,<:$SparseMatrixType) where {T,M} = 
    $SparseMatrixType( triu(CuSparseMatrixCOO(A), 0) )

In this method definition, type M is unused and may suffer from JuliaLang/julia#29393. In the fix, the first method was separated into a separate method, but only with the type parameters that are used, so it has no unbound arguments. The second method definition omits the type parameter M, so it too excludes unbound type arguments.

I apologize about the duplication, I tried my best to keep duplication minimal (e.g., creating a shared _unsafe_wrapped_buff from unsafe_wrap). I'm not sure there's alternatives to fix these issues, unfortunately. Perhaps we could ask for JuliaLang/julia#29393 to be reopened, since the issue was closed but technically remains an issue?

@maleadt maleadt added enhancement New feature or request tests Adds or changes tests. labels Jun 16, 2023
@maleadt
Copy link
Member

maleadt commented Jun 16, 2023

OK thanks, that clears it up. Pushed some additional things, this should be good to go if CI agrees.

@charleskawczynski
Copy link
Contributor Author

Thanks!

@maleadt maleadt merged commit 3c09d19 into JuliaGPU:master Jun 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request tests Adds or changes tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add aqua tests
2 participants