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

Fix QR based fitting #559

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions .github/workflows/Documenter.yml
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,11 @@ jobs:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@v4
- uses: julia-actions/julia-buildpkg@latest
- uses: julia-actions/julia-docdeploy@latest
- uses: julia-actions/setup-julia@v2
with:
version: 'lts'
show-versioninfo: true
- uses: julia-actions/julia-docdeploy@v1
env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
DOCUMENTER_KEY: ${{ secrets.DOCUMENTER_KEY }}
3 changes: 2 additions & 1 deletion Project.toml
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,10 @@ julia = "1.6"
CSV = "336ed68f-0bac-5ca0-87d4-7b16caf5d00b"
CategoricalArrays = "324d7699-5711-5eae-9e2f-1d82baa6b597"
DataFrames = "a93c6f00-e57d-5684-b7b6-d8193f3e46c0"
Downloads = "f43a241f-c20a-4ad4-852c-f6b1247861c6"
RDatasets = "ce6b1742-4840-55fa-b093-852dadbb1d8b"
StableRNGs = "860ef19b-820b-49d6-a774-d7a799459cd3"
Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40"

[targets]
test = ["CategoricalArrays", "CSV", "DataFrames", "RDatasets", "StableRNGs", "Test"]
test = ["CategoricalArrays", "CSV", "DataFrames", "Downloads", "RDatasets", "StableRNGs", "Test"]
2 changes: 1 addition & 1 deletion src/ftest.jl
Original file line number Diff line number Diff line change
Expand Up @@ -189,7 +189,7 @@ function show(io::IO, ftr::FTestResult{N}) where N

# get rid of negative zero -- doesn't matter mathematically,
# but messes up doctests and various other things
# cf. Issue #461
# cf. Issue #461
r2vals = [replace(@sprintf("%.4f", val), "-0.0000" => "0.0000") for val in ftr.r2]

outrows[2, :] = ["[1]", @sprintf("%.0d", ftr.dof[1]), " ",
Expand Down
2 changes: 1 addition & 1 deletion src/glmfit.jl
Original file line number Diff line number Diff line change
Expand Up @@ -622,7 +622,7 @@ function fit(::Type{M},
off = offset === nothing ? similar(y, 0) : offset
wts = wts === nothing ? similar(y, 0) : wts
rr = GlmResp(y, d, l, off, wts)

if method === :cholesky
res = M(rr, cholpred(X, dropcollinear), f, false)
elseif method === :qr
Expand Down
4 changes: 2 additions & 2 deletions src/glmtools.jl
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,7 @@ dispersion_parameter(::Union{Bernoulli, Binomial, Poisson}) = false

"""
_safe_int(x::T)

Convert to Int, when `x` is within 1 eps of an integer.
"""
function _safe_int(x::T) where {T<:AbstractFloat}
Expand All @@ -527,7 +527,7 @@ function loglik_obs end
loglik_obs(::Bernoulli, y, μ, wt, ϕ) = wt*logpdf(Bernoulli(μ), y)
loglik_obs(::Binomial, y, μ, wt, ϕ) = logpdf(Binomial(Int(wt), μ), _safe_int(y*wt))
loglik_obs(::Gamma, y, μ, wt, ϕ) = wt*logpdf(Gamma(inv(ϕ), μ*ϕ), y)
# In Distributions.jl, a Geometric distribution characterizes the number of failures before
# In Distributions.jl, a Geometric distribution characterizes the number of failures before
# the first success in a sequence of independent Bernoulli trials with success rate p.
# The mean of Geometric distribution is (1 - p) / p.
# Hence, p = 1 / (1 + μ).
Expand Down
89 changes: 35 additions & 54 deletions src/linpred.jl
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ mutable struct DensePredQR{T<:BlasReal,Q<:Union{QRCompactWY, QRPivoted}} <: Dens
scratchbeta::Vector{T}
qr::Q
scratchm1::Matrix{T}

function DensePredQR(X::AbstractMatrix, pivot::Bool=false)
n, p = size(X)
T = typeof(float(zero(eltype(X))))
Expand All @@ -62,70 +62,53 @@ Evaluate and return `p.delbeta` the increment to the coefficient vector from res
function delbeta! end

function delbeta!(p::DensePredQR{T,<:QRCompactWY}, r::Vector{T}) where T<:BlasReal
rnk = rank(p.qr.R)
rnk == length(p.delbeta) || throw(RankDeficientException(rnk))
p.delbeta = p.qr\r
mul!(p.scratchm1, Diagonal(ones(size(r))), p.X)
andreasnoack marked this conversation as resolved.
Show resolved Hide resolved
p.delbeta = p.qr \ r
return p
end

function delbeta!(p::DensePredQR{T,<:QRCompactWY}, r::Vector{T}, wt::Vector{T}) where T<:BlasReal
rnk = rank(p.qr.R)
rnk == length(p.delbeta) || throw(RankDeficientException(rnk))
X = p.X
W = Diagonal(wt)
sqrtW = Diagonal(sqrt.(wt))
wtsqrt = sqrt.(wt)
sqrtW = Diagonal(wtsqrt)
mul!(p.scratchm1, sqrtW, X)
mul!(p.delbeta, X'W, r)
qnr = qr(p.scratchm1)
Rinv = inv(qnr.R)
p.delbeta = Rinv * Rinv' * p.delbeta
ỹ = (wtsqrt .*= r) # to reuse wtsqrt's memory
p.qr = qr!(p.scratchm1)
p.delbeta = p.qr \ ỹ
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be done in-place?

Suggested change
p.delbeta = p.qr \
ldiv!(p.delbeta, p.qr, ỹ)

Copy link
Member Author

Choose a reason for hiding this comment

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

return p
end

function delbeta!(p::DensePredQR{T,<:QRPivoted}, r::Vector{T}) where T<:BlasReal
rnk = rank(p.qr.R)
if rnk == length(p.delbeta)
p.delbeta = p.qr\r
p.delbeta = p.qr \ r
else
R = @view p.qr.R[:, 1:rnk]
Q = @view p.qr.Q[:, 1:size(R, 1)]
R = UpperTriangular(view(parent(p.qr.R), 1:rnk, 1:rnk))
piv = p.qr.p
p.delbeta = zeros(size(p.delbeta))
p.delbeta[1:rnk] = R \ Q'r
fill!(p.delbeta, 0)
p.delbeta[1:rnk] = R \ view(p.qr.Q'r, 1:rnk)
Comment on lines +87 to +88
Copy link
Member

Choose a reason for hiding this comment

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

Maybe one could save a few allocations here?

Suggested change
fill!(p.delbeta, 0)
p.delbeta[1:rnk] = R \ view(p.qr.Q'r, 1:rnk)
mul!(view(p.delbeta, 1:rnk), view(p.qr.Q, :, 1:rnk)', r)
ldiv!(R, view(p.delbeta, 1:rnk))
fill!(@view(p.delbeta[(rnk + 1):end]), 0)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think a view of a Householder matrix would end up hitting a fallback multiplicatino which would be very costly.

invpermute!(p.delbeta, piv)
end
mul!(p.scratchm1, Diagonal(ones(size(r))), p.X)
return p
end

function delbeta!(p::DensePredQR{T,<:QRPivoted}, r::Vector{T}, wt::Vector{T}) where T<:BlasReal
rnk = rank(p.qr.R)
X = p.X
W = Diagonal(wt)
sqrtW = Diagonal(sqrt.(wt))
delbeta = p.delbeta
scratchm2 = similar(X, T)
wtsqrt = sqrt.(wt)
sqrtW = Diagonal(wtsqrt)
mul!(p.scratchm1, sqrtW, X)
mul!(scratchm2, W, X)
mul!(delbeta, transpose(scratchm2), r)

if rnk == length(p.delbeta)
qnr = qr(p.scratchm1)
Rinv = inv(qnr.R)
p.delbeta = Rinv * Rinv' * delbeta
else
qnr = pivoted_qr!(copy(p.scratchm1))
R = @view qnr.R[1:rnk, 1:rnk]
Rinv = inv(R)
piv = qnr.p
permute!(delbeta, piv)
for k=(rnk+1):length(delbeta)
delbeta[k] = -zero(T)
end
p.delbeta[1:rnk] = Rinv * Rinv' * view(delbeta, 1:rnk)
invpermute!(delbeta, piv)
r̃ = (wtsqrt .*= r) # to reuse wtsqrt's memory

p.qr = pivoted_qr!(p.scratchm1)
rnk = rank(p.qr.R) # FIXME! Don't use svd for this
R = UpperTriangular(view(parent(p.qr.R), 1:rnk, 1:rnk))
permute!(p.delbeta, p.qr.p)
for k = (rnk + 1):length(p.delbeta)
p.delbeta[k] = zero(T)
end
p.delbeta[1:rnk] = R \ view(p.qr.Q'*r̃, 1:rnk)
invpermute!(p.delbeta, p.qr.p)

return p
end

Expand Down Expand Up @@ -279,27 +262,25 @@ end
LinearAlgebra.cholesky(p::SparsePredChol{T}) where {T} = copy(p.chol)
LinearAlgebra.cholesky!(p::SparsePredChol{T}) where {T} = p.chol

function invqr(x::DensePredQR{T,<: QRCompactWY}) where T
Q,R = qr(x.scratchm1)
Rinv = inv(R)
function invqr(p::DensePredQR{T,<: QRCompactWY}) where T
Rinv = inv(p.qr.R)
Rinv*Rinv'
end

function invqr(x::DensePredQR{T,<: QRPivoted}) where T
Q,R,pv = pivoted_qr!(copy(x.scratchm1))
rnk = rank(R)
p = length(x.delbeta)
if rnk == p
Rinv = inv(R)
function invqr(p::DensePredQR{T,<: QRPivoted}) where T
rnk = rank(p.qr.R)
k = length(p.delbeta)
if rnk == k
Rinv = inv(p.qr.R)
xinv = Rinv*Rinv'
ipiv = invperm(pv)
ipiv = invperm(p.qr.p)
return xinv[ipiv, ipiv]
else
Rsub = R[1:rnk, 1:rnk]
Rsub = UpperTriangular(view(p.qr.R, 1:rnk, 1:rnk))
RsubInv = inv(Rsub)
xinv = fill(convert(T, NaN), (p,p))
xinv = fill(convert(T, NaN), (k, k))
xinv[1:rnk, 1:rnk] = RsubInv*RsubInv'
ipiv = invperm(pv)
ipiv = invperm(p.qr.p)
return xinv[ipiv, ipiv]
end
end
Expand Down
4 changes: 2 additions & 2 deletions src/lm.jl
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,7 @@ function fit(::Type{LinearModel}, X::AbstractMatrix{<:Real}, y::AbstractVector{<
"argument `dropcollinear` instead. Proceeding with positional argument value: $allowrankdeficient_dep"
dropcollinear = allowrankdeficient_dep
end

if method === :cholesky
fit!(LinearModel(LmResp(y, wts), cholpred(X, dropcollinear), nothing))
elseif method === :qr
Expand Down Expand Up @@ -299,7 +299,7 @@ function StatsModels.predict!(res::Union{AbstractVector,
length(res) == size(newx, 1) ||
throw(DimensionMismatch("length of `res` must equal the number of rows in `newx`"))
res .= newx * coef(mm)
elseif mm.pp isa DensePredChol &&
elseif mm.pp isa DensePredChol &&
mm.pp.chol isa CholeskyPivoted &&
mm.pp.chol.rank < size(mm.pp.chol, 2)
throw(ArgumentError("prediction intervals are currently not implemented " *
Expand Down
Loading
Loading