Redundant dimension wastes space and allows disagreement (#178)

* Redundant dimension wastes space and allows disagreement * Bump version for a non-breaking patch release * A few fixes * Add deprecations to `dim` to constructors * Revert 1-element vector ldiv returning vectors * Move `@deprecate`s to `deprecates.jl` * Add missing deprecation and tests --------- Co-authored-by: David Widmann <[email protected]>
JuliaStats · Oct 4, 2023 · 57cdd4d · 57cdd4d
1 parent 6c91f8b
commit 57cdd4d
Show file tree

Hide file tree

Showing 5 changed files with 106 additions and 12 deletions.
diff --git a/src/deprecates.jl b/src/deprecates.jl
@@ -16,3 +16,9 @@ using Base: @deprecate
 @deprecate PDiagMat(v::AbstractVector, inv_v::AbstractVector) PDiagMat(v)
 
 @deprecate dim(a::AbstractMatrix) LinearAlgebra.checksquare(a)
+
+@deprecate PDMat{T,S}(d::Int, m::AbstractMatrix{T}, c::Cholesky{T,S}) where {T,S} PDMat{T,S}(m, c)
+
+@deprecate PDiagMat(dim::Int, diag::AbstractVector{<:Real}) PDiagMat(diag)
+@deprecate PDiagMat{T,V}(dim, diag) where {T<:Real, V<:AbstractVector{T}} PDiagMat{T,V}(diag)
+
diff --git a/src/pdiagmat.jl b/src/pdiagmat.jl
@@ -2,11 +2,16 @@
 Positive definite diagonal matrix.
 """
 struct PDiagMat{T<:Real,V<:AbstractVector{T}} <: AbstractPDMat{T}
-    dim::Int
     diag::V
 end
 
-PDiagMat(v::AbstractVector{<:Real}) = PDiagMat{eltype(v),typeof(v)}(length(v), v)
+function Base.getproperty(a::PDiagMat, s::Symbol)
+    if s === :dim
+        return length(getfield(a, :diag))
+    end
+    return getfield(a, s)
+end
+Base.propertynames(::PDiagMat) = (:diag, :dim)
 
 AbstractPDMat(A::Diagonal{<:Real}) = PDiagMat(A.diag)
 AbstractPDMat(A::Symmetric{<:Real,<:Diagonal{<:Real}}) = PDiagMat(A.data.diag)

diff --git a/src/pdmat.jl b/src/pdmat.jl
@@ -2,23 +2,31 @@
 Full positive definite matrix together with a Cholesky factorization object.
 """
 struct PDMat{T<:Real,S<:AbstractMatrix} <: AbstractPDMat{T}
-    dim::Int
     mat::S
     chol::Cholesky{T,S}
 
-    PDMat{T,S}(d::Int,m::AbstractMatrix{T},c::Cholesky{T,S}) where {T,S} = new{T,S}(d,m,c)
+    PDMat{T,S}(m::AbstractMatrix{T},c::Cholesky{T,S}) where {T,S} = new{T,S}(m,c)
 end
 
 function PDMat(mat::AbstractMatrix,chol::Cholesky{T,S}) where {T,S}
-    d = size(mat, 1)
-    size(chol, 1) == d ||
+    d = LinearAlgebra.checksquare(mat)
+    if size(chol, 1) != d
         throw(DimensionMismatch("Dimensions of mat and chol are inconsistent."))
-    PDMat{T,S}(d, convert(S, mat), chol)
+    end
+    PDMat{T,S}(convert(S, mat), chol)
 end
 
 PDMat(mat::AbstractMatrix) = PDMat(mat, cholesky(mat))
 PDMat(fac::Cholesky) = PDMat(AbstractMatrix(fac), fac)
 
+function Base.getproperty(a::PDMat, s::Symbol)
+    if s === :dim
+        return size(getfield(a, :mat), 1)
+    end
+    return getfield(a, s)
+end
+Base.propertynames(::PDMat) = (:mat, :chol, :dim)
+
 AbstractPDMat(A::Cholesky) = PDMat(A)
 
 ### Conversion

diff --git a/src/pdsparsemat.jl b/src/pdsparsemat.jl
@@ -2,24 +2,32 @@
 Sparse positive definite matrix together with a Cholesky factorization object.
 """
 struct PDSparseMat{T<:Real,S<:AbstractSparseMatrix} <: AbstractPDMat{T}
-    dim::Int
     mat::S
     chol::CholTypeSparse
 
-    PDSparseMat{T,S}(d::Int,m::AbstractSparseMatrix{T},c::CholTypeSparse) where {T,S} =
-        new{T,S}(d,m,c) #add {T} to CholTypeSparse argument once #14076 is implemented
+    PDSparseMat{T,S}(m::AbstractSparseMatrix{T},c::CholTypeSparse) where {T,S} =
+        new{T,S}(m,c) #add {T} to CholTypeSparse argument once #14076 is implemented
 end
+@deprecate PDSparseMat{T,S}(d::Int, m::AbstractSparseMatrix{T}, c::CholTypeSparse) where {T,S} PDSparseMat{T,S}(m, c)
 
 function PDSparseMat(mat::AbstractSparseMatrix,chol::CholTypeSparse)
-    d = size(mat, 1)
+    d = LinearAlgebra.checksquare(mat)
     size(chol, 1) == d ||
         throw(DimensionMismatch("Dimensions of mat and chol are inconsistent."))
-    PDSparseMat{eltype(mat),typeof(mat)}(d, mat, chol)
+    PDSparseMat{eltype(mat),typeof(mat)}(mat, chol)
 end
 
 PDSparseMat(mat::SparseMatrixCSC) = PDSparseMat(mat, cholesky(mat))
 PDSparseMat(fac::CholTypeSparse) = PDSparseMat(sparse(fac), fac)
 
+function Base.getproperty(a::PDSparseMat, s::Symbol)
+    if s === :dim
+        return size(getfield(a, :mat), 1)
+    end
+    return getfield(a, s)
+end
+Base.propertynames(::PDSparseMat) = (:mat, :chol, :dim)
+
 AbstractPDMat(A::SparseMatrixCSC) = PDSparseMat(A)
 AbstractPDMat(A::CholTypeSparse) = PDSparseMat(A)
 

diff --git a/test/pdmtypes.jl b/test/pdmtypes.jl
@@ -36,6 +36,18 @@ using Test
                 test_pdmat(PDSparseMat(sparse(M)), M,          cmat_eq=true, verbose=1, t_eig=false)
             end
         end
+
+        @testset "test deprecated internal constructors" begin
+            m = Matrix{T}(I, 2, 2)
+            C = cholesky(m)
+            @test @test_deprecated(PDMat{T,typeof(m)}(2, m, C)) == PDMat(m)
+            d = ones(T,2)
+            @test @test_deprecated(PDiagMat(2, d)) == @test_deprecated(PDiagMat{T,Vector{T}}(2, d)) == PDiagMat(d)
+            if HAVE_CHOLMOD
+                s = SparseMatrixCSC{T}(I, 2, 2)
+                @test @test_deprecated(PDSparseMat{T, typeof(s)}(2, s, cholesky(s))) == PDSparseMat(s)
+            end
+        end
     end
 
     @testset "zero-dimensional matrices" begin
@@ -189,4 +201,59 @@ using Test
         @test M isa PDSparseMat
         @test Matrix(M) ≈ A
     end
+
+    @testset "properties and fields" begin
+        for dim in (1, 5, 10)
+            x = rand(dim, dim)
+            M = PDMat(x' * x + I)
+            @test fieldnames(typeof(M)) == (:mat, :chol)
+            @test propertynames(M) == (fieldnames(typeof(M))..., :dim)
+            @test getproperty(M, :dim) === dim
+            for p in fieldnames(typeof(M))
+                @test getproperty(M, p) === getfield(M, p)
+            end
+
+            M = PDiagMat(rand(dim))
+            @test fieldnames(typeof(M)) == (:diag,)
+            @test propertynames(M) == (fieldnames(typeof(M))..., :dim)
+            @test getproperty(M, :dim) === dim
+            for p in fieldnames(typeof(M))
+                @test getproperty(M, p) === getfield(M, p)
+            end
+
+            M = ScalMat(dim, rand())
+            @test fieldnames(typeof(M)) == (:dim, :value)
+            @test propertynames(M) == fieldnames(typeof(M))
+            for p in fieldnames(typeof(M))
+                @test getproperty(M, p) === getfield(M, p)
+            end
+
+            if HAVE_CHOLMOD
+                x = sprand(dim, dim, 0.2)
+                M = PDSparseMat(x' * x + I)
+                @test fieldnames(typeof(M)) == (:mat, :chol)
+                @test propertynames(M) == (fieldnames(typeof(M))..., :dim)
+                @test getproperty(M, :dim) === dim
+                for p in fieldnames(typeof(M))
+                    @test getproperty(M, p) === getfield(M, p)
+                end
+            end
+        end
+    end
+
+    @testset "Incorrect dimensions" begin
+        x = rand(10, 10)
+        A = x * x' + I
+        C = cholesky(A)
+        @test_throws DimensionMismatch PDMat(A[:, 1:(end - 1)], C)
+        @test_throws DimensionMismatch PDMat(A[1:(end - 1), 1:(end - 1)], C)
+
+        if HAVE_CHOLMOD
+            x = sprand(10, 10, 0.2)
+            A = x * x' + I
+            C = cholesky(A)
+            @test_throws DimensionMismatch PDSparseMat(A[:, 1:(end - 1)], C)
+            @test_throws DimensionMismatch PDSparseMat(A[1:(end - 1), 1:(end - 1)], C)
+        end
+    end
 end