Repository navigation
Build a CirculantMass from its first column - #11
michakraus wants to merge 1 commit into
Conversation
A caller that could describe a circulant matrix in O(n) still had to materialise O(n^2) entries, and CirculantMass then kept them for the life of the operator. mass_operator(c, b) and CirculantMass(c, n) now take that first column. It is wrapped in a new internal Circulant, which stores nothing else. Circulance holds by construction there, so _check_circulant has nothing left to verify -- which is what keeps the path O(n) in construction as well as in storage. mass_matrix returns the Circulant, and size, Matrix and every solve are unchanged. Only a periodic basis on a UniformMesh has a circulant mass matrix, so that is the only combination the vector form accepts. Fixes #10 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
There was a problem hiding this comment.
🟡 Changes recommended
Circulant mishandles valid offset-indexed vectors during indexing or materialization.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds an O(n)-storage and construction path for circulant mass operators from their first column.
Changes:
- Introduces internal
Circulantstorage and vector-based constructors. - Adds uniform periodic
mass_operatordispatch. - Updates tests, documentation, and changelog coverage.
File summaries
| File | Summary |
|---|---|
test/mass_tests.jl |
Tests construction, solving, storage, and rejection cases. |
src/mass.jl |
Implements circulant storage and construction; offset-indexed vectors require correction. |
docs/src/usage/quadrature.md |
Documents first-column usage. |
docs/src/library.md |
Documents Circulant. |
CHANGELOG.md |
Records the new API and limitation. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # rather than throw. The bounds check is what keeps that from happening. | ||
| function Base.getindex(C::Circulant, k::Int, l::Int) | ||
| @boundscheck checkbounds(C, k, l) | ||
| return @inbounds C.c[mod1(k - l + 1, length(C.c))] |
|
Closing this unmerged, in favour of #12, which removes the problem rather than making it cheaper WhyThis branch exists because a caller had to materialise
The With that in place the caller has no shift to build. It hands over the sparse assembly itself, and What that leavesThe first-column entry point is still a fair API for a caller that describes a circulant matrix in #10 stays open for its own deferred item — making For anyone who revives thisTwo things the review turned up, both worth carrying over:
The branch is kept. Nothing here was wrong — it answered the question as asked, and the |
Closes #10.
A caller that can describe a circulant matrix in
O(n)no longer has to materialiseO(n²)entries to get its mass operator. The four points of the issue, additively — no existing
signature and no existing result changes.
What it adds
SimpleSplines.Circulant{T, CT}AbstractMatrixholding only the first column:C[k, l] = c[mod1(k - l + 1, n)], plussizeand the genericMatrix, which materialises on demand_check_circulant(::Circulant, …)nothing— circulance holds by construction, so the quadratic verification has nothing left to verify. This is what keeps the pathO(n)in construction as well as in storageCirculantMass(c::AbstractVector, n)cand runs the existing constructor.mass_matrix(op)returns theCirculant;size,Matrixand every solve are unchangedmass_operator(c::AbstractVector, b::PeriodicBSplineBasis)The case it comes from
The periodic B-spline Poisson solve in PoissonSolvers.jl shifts the periodic stiffness matrix
by the rank-one mean projector before factorising.
Sis sparse and banded,S + 𝟙𝟙ᵀ/nisstructurally full, and both are circulant.
At order 5 on a
UniformMeshof 1024 cells, measured on Julia 1.13.0:summarysizeS— for referenceS .+ inv(n)— aSparseMatrixCSC, structurally fullS[:, 1] .+ inv(n)— aSparseVectorVector(S[:, 1]) .+ inv(n)— a dense columnThree orders of magnitude, and the solves are identical to the last bit. The caller now writes
mass_operator(S[:, 1] .+ inv(n), b).Circulantis parametrised on the vector type, so asparse column is stored as one rather than being copied;
Vector(…)first is half the sizeagain, if the caller cares.
What raises
Only a periodic basis on a
UniformMeshhas a circulant mass matrix, so that is the onlycombination the vector form accepts. A periodic basis on a
GradedMeshor aRandomMeshraises an
ArgumentErrorrather than falling to theFactorizedMassbranch, which wouldfactorise a matrix the caller never described. A bounded basis matches no method at all.
Two decisions worth a reviewer's eye
Circulantis not exported. The issue calls it an internal representation, andToeplitzMatrices.jlexports the same name —using SimpleSplines, ToeplitzMatriceswouldthen be an ambiguity error for a type nobody needs to name. It is documented on the Library
page, and its docstring says how to reach it. Say the word if you would rather export it.
CirculantMass(c, n)takes noatol. There is nothing on this path for a tolerance togovern, so the keyword is absent rather than accepted and ignored.
Verification
Pkg.test()on this branch: 64 206 passed, 0 failed,Aqua.test_allincluded(ambiguities, piracy, stale deps, persistent tasks, compat bounds all green).
julia --project=docs -e 'include("docs/make.jl")'): exit 0, doctests ran,no missing-docstring and no broken-
@refwarnings.--check,fatou lintand the NFC check: clean on every changed file.julia-branch-verifierran the seven-point pass against the diff. What it found:Fixed
test/mass_tests.jl:93— a comment narrated the change instead of the state(
circulance is no longer checked). Rewritten in the present tense.src/mass.jl:189—# Verify rather than assume.was made incomplete by this branch, sinceone constructor path now assumes, justified by the type. The comment says so.
src/mass.jl:216—atolwas accepted and ignored on the vector form. Removed, so it nowraises a
MethodError.Raised and not accepted
16.8 MBfigure in the changelog as unreproducible, measuring8.39 MB. Ithad measured
Matrix(S) .+ inv(n)— a denseMatrix{Float64}, 8 388 656 B. The expressionthe changelog names is
S .+ inv(n), which stays sparse and so carriesrowvalbesidenzval:SparseMatrixCSC, 1 048 576 stored entries, 16 785 576 B. The figure stands, andthe three others in that sentence reproduce exactly.
Checked and clean
mod1indexing agrees with the documentedC_{kl}formula andC[:, 1] == c. Therfftsolve is correct for a general real circulant, not only asymmetric one — checked against a non-symmetric case to
1.4e-17. Out-of-range indices inall five corners raise
BoundsError; linear and cartesian indexing agree.mass_matrixreturn type:*,mul!(including into a view),kron,copyand a two-factorKroneckerMassall work with aCirculantfactor —km \ (km * B)round-trips to6.4e-15.matrix type.
code_typedon every changed symbol returns concrete types; noAnyoriginates in a changed hunk.
--check-bounds=auto: a fullgetindexloop allocates 0 bytes;the vector construction path costs 32 B more than the matrix path at
n = 1024— thewrapper — the rest being the FFTW plans both paths share.
Not fixed
CirculantMassbuilt from a materialised matrix still stores the whole matrix. DroppingMthere would make every periodic operator
O(n), but it changes whatmass_matrixreturns forexisting callers, so it belongs in a separate decision, as the issue says.
🤖 Generated with Claude Code