From 57cdd4de02729939515ec4ce6bcc5713221a2e39 Mon Sep 17 00:00:00 2001 From: Chris Elrod Date: Wed, 4 Oct 2023 09:12:47 -0400 Subject: [PATCH] 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 --- src/deprecates.jl | 6 +++++ src/pdiagmat.jl | 9 +++++-- src/pdmat.jl | 18 +++++++++---- src/pdsparsemat.jl | 18 +++++++++---- test/pdmtypes.jl | 67 ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 106 insertions(+), 12 deletions(-) diff --git a/src/deprecates.jl b/src/deprecates.jl index 0bfdb15..385206a 100644 --- 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 index df6f74f..3831285 100644 --- 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 index b8fd077..4149df9 100644 --- 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 index 2899b59..baedad6 100644 --- 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 index 37c2e83..471b826 100644 --- 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