Skip to content

Commit

Permalink
remove unsupported use of chained type parameters
Browse files Browse the repository at this point in the history
  • Loading branch information
@JeffBezanson
JeffBezanson committed Oct 12, 2014
1 parent 79e006b commit 2e46b1b
Show file tree
Hide file tree
Showing 5 changed files with 6 additions and 6 deletions.
2 changes: 1 addition & 1 deletion base/linalg/cholesky.jl
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
##########################
# Cholesky Factorization #
##########################
immutable Cholesky{T,S<:AbstractMatrix{T},UpLo} <: Factorization{T}
immutable Cholesky{T,S<:AbstractMatrix,UpLo} <: Factorization{T}
UL::S
end
immutable CholeskyPivoted{T} <: Factorization{T}
Expand Down
2 changes: 1 addition & 1 deletion base/linalg/ldlt.jl
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
immutable LDLt{T,S<:AbstractMatrix{T}} <: Factorization{T}
immutable LDLt{T,S<:AbstractMatrix} <: Factorization{T}
data::S
end

Expand Down
2 changes: 1 addition & 1 deletion base/linalg/lu.jl
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
####################
# LU Factorization #
####################
immutable LU{T,S<:AbstractMatrix{T}} <: Factorization{T}
immutable LU{T,S<:AbstractMatrix} <: Factorization{T}
factors::S
ipiv::Vector{BlasInt}
info::BlasInt
Expand Down
4 changes: 2 additions & 2 deletions base/linalg/symmetric.jl
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
#Symmetric and Hermitian matrices
immutable Symmetric{T,S<:AbstractMatrix{T}} <: AbstractMatrix{T}
immutable Symmetric{T,S<:AbstractMatrix} <: AbstractMatrix{T}
data::S
uplo::Char
end
Symmetric(A::AbstractMatrix, uplo::Symbol=:U) = (chksquare(A);Symmetric{eltype(A),typeof(A)}(A, char_uplo(uplo)))
immutable Hermitian{T,S<:AbstractMatrix{T}} <: AbstractMatrix{T}
immutable Hermitian{T,S<:AbstractMatrix} <: AbstractMatrix{T}
data::S
uplo::Char
end
Expand Down
2 changes: 1 addition & 1 deletion base/linalg/triangular.jl
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
## Triangular
immutable Triangular{T,S<:AbstractMatrix{T},UpLo,IsUnit} <: AbstractMatrix{T}
immutable Triangular{T,S<:AbstractMatrix,UpLo,IsUnit} <: AbstractMatrix{T}
data::S
end
function Triangular{T}(A::AbstractMatrix{T}, uplo::Symbol, isunit::Bool=false)
Expand Down

6 comments on commit 2e46b1b

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm disturbed by how much code there seems to be in linalg that appears to be entirely untested.

@jiahao
Copy link
Member

@jiahao jiahao commented on 2e46b1b Oct 13, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was our attempt to mimic triangular dispatch. A lot of the code for special matrix types needs to reason about the type of the container and the type of the matrix element. The signature doesn't make the compiler complain (yet), but it doesn't quite work as expected. @andreasnoack has encountered issues where the type parameter T of the container would silently promote (e.g. from Matrix{Float64} to Matrix{BigFloat}), but the declared eltype of the container (S) doesn't change, and so the kernels had to be rewritten to manually update S at runtime.

This is a problem that needs to be thought about more carefully. Ref: #8240

@timholy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While the syntax is still a little ugly for the moment, you can get triangular dispatch with the trick in #2345. I've had to do a lot of that with Images ever since the transition to incorporating Color.

@JeffBezanson
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The behavior that falls out of an implementation detail of the compiler here is that the T in AbstractMatrix{T} basically has no effect, so on master I don't think this commit changes any functionality. However on my jb/call_constructors branch the implementation changed, and this gives a "T not defined" error, which is why I did this.

@andreasnoack
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JeffBezanson Thanks for the explanation. Ironically, as @jiahao mentioned, I encountered the problem this Friday without realising the root cause. I have been living in delusion. Probably, I'll have to write inner constructors for all the types to ensure that T actually carries information about the element type.

@StefanKarpinski There is room for improvement, but I think that the coverage of exported functions in LinAlg is decent. However, promotion is a challenge. We try, but it is difficult to cover all corners of possible promotions. Regarding this commit, the wrong assumption that T had an effect in AbstractMatrix{T} has probably not been discovered before because the problematic constructors are rarely used, e.g. when constructing a triangular matrix it is more common to write Triangular(A, :L) than Triangular{Float64,SubArray{Float64,2,Array{Float64,2},(UnitRange{Int64},UnitRange{Int64})},:L,false}(A).

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is less bad than I thought it was since it used to accidentally work – I thought it was fundamentally broken and that brokenness simply hadn't been discovered. There's been a fair amount of that sort of thing in the linalg code in the past but it does seem to be much better since you guys have been putting so much work into it.

Please sign in to comment.