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 StructuredArray abstract type #6906

Closed
wants to merge 1 commit into from

Conversation

simonbyrne
Copy link
Contributor

At the risk of spilling yet more bits over the type hiearchy (#6212, etc.), I would like to propose an abstract StructuredArray type for "specially structured" arrays. The main purpose is to make it easier to reason about the type hierarchy, which is becoming difficult when dealing with ambiguous dispatch for all the Ax_foo_Bx methods.

julia> subtypes(StructuredArray)
10-element Array{Any,1}:
 Bidiagonal{T}                                                             
 Diagonal{T}                                                               
 Givens{T}                                                                 
 Hermitian{T}                                                              
 SymTridiagonal{T}                                                         
 SymmetricRFP{T<:Union(Complex{Float64},Float32,Complex{Float32},Float64)} 
 Symmetric{T}                                                              
 TriangularRFP{T<:Union(Complex{Float64},Float32,Complex{Float32},Float64)}
 Triangular{T<:Number}                                                     
 Tridiagonal{T}                                                            

The remaining subtypes of AbstractArray are:

julia> subtypes(AbstractArray)
10-element Array{Any,1}:
 AbstractSparseArray{Tv,Ti,N}                                          
 DArray{T,N,A}                                                         
 DenseArray{T,N}                                                       
 HessenbergQ{T}                                                        
 QRCompactWYQ{S}                                                       
 QRPackedQ{T}                                                          
 Range{T}                                                              
 StructuredArray{T,N}                                                  
 SubArray{T,N,A<:AbstractArray{T,N},I<:(Union(Range{Int64},Int64)...,)}
 Woodbury{T}                                                           

Of these, HessenbergQ{T}, QRCompactWYQ{S} and QRPackedQ{T} could also be moved, as they are also structured in some sense.

@stevengj
Copy link
Member

Can you give an example of a method that would dispatch on StructuredArray?

@simonbyrne
Copy link
Contributor Author

Honestly, no, it is purely for organisational convenience. That said, DenseArray is only used once for dispatch, and AbstractSparseArray twice (and one of those is issparse).

@simonbyrne
Copy link
Contributor Author

Perhaps I should expand on the point above. At the moment, when implementing new Ax_foo_Bx methods, you have to be very careful not to introduce ambiguities. Say I have a new type Bar <: AbstractMatrix, and I want to define

Ax_foo_Bx(A::Bar, B::AbstractMatrix) = ...
Ax_foo_Bx(A::AbstractMatrix, B::Bar) = ...

Then this will introduce a bunch of ambiguities for calls like Ax_foo_Bx(A::Bar, B::Baz), where Baz <: AbstractMatrix has its own methods like these (QRPackedQ is a current example).

One possible solution is to have a convention that AbstractMatrix should only appear in the second argument. If we want to define Ax_foo_Bx(A::AbstractMatrix, B::Bar), then we have to be more specific with the typing of A. If all matrices are subtypes of DenseMatrix, AbstractSparseMatrix or AbstractStructuredMatrix, this would be considerably easier to handle, and keep track of which ones will fall through the gaps.

@jakebolewski
Copy link
Member

Closing as improvements to Array / Matrix types are discussed elsewhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs decision A decision on this change is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants