Skip to content

Fix a few bugs#236

Merged
devmotion merged 3 commits into
masterfrom
dmw/fixes
Jun 25, 2026
Merged

Fix a few bugs#236
devmotion merged 3 commits into
masterfrom
dmw/fixes

Conversation

@devmotion

@devmotion devmotion commented Jun 24, 2026

Copy link
Copy Markdown
Member

I noticed these (unrelated) bugs while updating #188:

  • The quad/invquad specializations for PDMat were dispatched on PDMat{<:Real, <:Vector}. But the second type parameter of PDMat is the type of the wrapped matrix (<:AbstractMatrix), so this signature can never be satisfied and the specializations were never used — instead the generic fallbacks, which materialize chol_upper(a) * x, were called. Changing it to <:Matrix restores the intended implementation: for an n×n matrix and an n×k input the specialization reuses a single length-n buffer, so it allocates O(n + k) instead of the O(n*k) temporary of the fallback.
  • \ and / for PDSparseMat unnecessarily required the element types of the matrix and the right-hand side to match. Since CHOLMOD only supports Float64/ComplexF64 anyway, this excluded e.g. integer or Float32 inputs that the other types accept. I relaxed the signatures to AbstractVecOrMat{<:Real} and promote the element type of the result.
  • The right-division method /(::AbstractVecOrMat, ::PDSparseMat) was in fact dead: CHOLMOD provides no right division, so x / a.chol always threw a MethodError, and every test skipped the sparse right-division path (so it was also uncovered). Since a is symmetric, x / a == (a \ xᵀ)ᵀ, so I route through the (working, covered) left-division method and enable the previously-skipped right-division tests for PDSparseMat.
  • I removed the self-recursive definition *(a::AbstractPDMat, c::Real) = a * c in generics.jl. It is shadowed by the concrete methods of the built-in types but leads to a StackOverflowError for other AbstractPDMat subtypes.
  • For consistency with the other types I defined Matrix{T}(::ScalMat) instead of Matrix(::ScalMat) (the untyped constructor is provided by Base).

Regression tests are added in test/typeparams.jl.

The allocation test in test/chol.jl was marked broken on Apple since Julia 1.11.5. The bound turns out to be unreliable there in a machine-dependent way: it is met on my Apple silicon machine (Julia 1.12.6) but exceeded on the CI Apple runner for the same version, so neither value of the broken marker works for both (it errors as "unexpectedly passing" where it passes, and fails where it doesn't). I therefore skip the allocation check on Apple (Julia >= 1.11.5) instead. The marker only ever applied to Apple, so other CI configurations are unaffected.

🤖 Generated with Claude Code

devmotion and others added 2 commits June 24, 2026 21:43
- pdmat.jl: `quad`/`invquad` were specialized on `PDMat{<:Real,<:Vector}`,
  but the second type parameter of `PDMat` is the type of the wrapped matrix
  (`<:AbstractMatrix`), so this signature is never satisfied and the
  specializations were never used. The generic fallbacks ran instead,
  materializing the full `chol_upper(a) * x` product. Use `<:Matrix`, restoring
  the reduced-allocation implementation.
- pdsparsemat.jl: `\` and `/` unnecessarily required the operand element type to
  match the matrix's. Relax to `AbstractVecOrMat{<:Real}` and promote the result.
- generics.jl: remove the self-recursive `*(a::AbstractPDMat, c::Real) = a * c`,
  a stack-overflow trap for custom `AbstractPDMat` subtypes.
- scalmat.jl: define `Matrix{T}` (like the other types) instead of the untyped
  `Matrix`; the untyped form is provided by Base.
- test/typeparams.jl: regression tests. test/chol.jl: bound the `broken`
  allocation marker to `< v"1.12"` (passes on Apple silicon with Julia 1.12).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@codecov

codecov Bot commented Jun 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.78%. Comparing base (473f638) to head (c814d22).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #236      +/-   ##
==========================================
+ Coverage   93.37%   94.78%   +1.40%     
==========================================
  Files          11       11              
  Lines         800      805       +5     
==========================================
+ Hits          747      763      +16     
+ Misses         53       42      -11     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/pdsparsemat.jl
T = promote_type(eltype(a), eltype(x))
return convert(Array{T}, a.chol \ convert(Array{Float64}, x))
end
function /(x::AbstractVecOrMat{<:Real}, a::PDSparseMat)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Coverage suggests that this one is not hit

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

AFAICT this method has never worked and is explicitly disabled in the tests.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I added a workaround for the non-existing / in SparseArrays in c814d22

The `chol.jl` allocation bound is unreliable on Apple: it is met on some
machines but exceeded on CI for the same Julia version, so a `broken`
marker either fails CI (exceeded) or errors locally (unexpectedly
passing). Skip the check on Apple (Julia >= 1.11.5) instead.

`/(::AbstractVecOrMat{<:Real}, ::PDSparseMat)` could never run: CHOLMOD
has no right division, so `x / a.chol` threw a MethodError, and every
test skipped the path (leaving it uncovered). Since `a` is symmetric,
`x / a == (a \ xᵀ)ᵀ`, so route through the existing left-division method
and enable the previously-skipped right-division tests for PDSparseMat.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@devmotion devmotion marked this pull request as ready for review June 24, 2026 21:32
@devmotion devmotion requested a review from andreasnoack June 24, 2026 21:45
@devmotion devmotion merged commit 6371e0c into master Jun 25, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants