Modernise CI, split out the docs build, and make the per-step paths allocation-free - #5
Conversation
`l2_projection!` was in-place only in its output: it built the `f ⊙ w` product and the load vector as fresh arrays on every call, 6.3 kB per projection at N = 128. The quadrature now holds an `f ⊙ w` buffer and the load vector is formed with `mul!` straight into `û`, which takes a uniform-mesh projection to zero allocations end to end -- the `CirculantMass` solve was already allocation-free. On a non-uniform mesh the CHOLMOD temporary remains, CHOLMOD having no in-place `ldiv!`. The shared buffer makes the method non-reentrant across threads, so the docstring says so and points at the allocating `l2_projection` as the way out. It also gains the sample-length check the allocating method already had; the buffer would otherwise have turned a wrong-length `f` into a broadcast error rather than a named one. `basis_integrals` recomputed `Φ₀ * w` on every call, 1.1 kB a time, although ∫ φ_i dx is as much a constant of the discretisation as the mass matrix. It is now assembled with the quadrature and returned by reference, like `mass_matrix`, and the docstring records that it must not be mutated. Both properties are pinned by tests, in the style of the existing `mass_solve!` allocation test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Split the documentation build out of `CI.yml` into its own `Documenter.yml`, so
that a docs failure and a test failure are separate signals and the docs job is
not queued behind the test matrix. The README gains a badge for it.
Bring the actions up to current versions -- `actions/checkout@v7`,
`julia-actions/setup-julia@v3`, `julia-actions/cache@v3`,
`codecov/codecov-action@v7`, and `RegisterAction` pinned so Dependabot can bump
it, which `@latest` prevented. Dependabot now groups action bumps into one pull
request.
The test matrix names Julia versions by alias rather than by number: `min`
resolves the lower bound of the `julia` compat entry, so the matrix tracks the
declared support window instead of having to be edited alongside it; `lts` and
`1` cover the long-term-support and current stable releases; `pre` and
`nightly` run on Linux only under `continue-on-error`, an upcoming-release
failure being information rather than a broken build. `arch` becomes `default`,
which tests Julia natively on the ARM64 macOS runners instead of under Rosetta.
Two workflow bugs fixed on the way:
- `docs/Project.toml` pinned `CompactBasisFunctions` to an absolute path on a
developer's machine through a `[sources]` entry. That path does not exist on
a runner, so the documentation build could only ever have succeeded locally.
0.3.1 is registered, so the entry goes and the dependency resolves from the
registry.
- `CompatHelper.yml` invoked `julia` without installing it. The runner images
no longer ship a Julia, so the workflow failed before CompatHelper started.
Add `AUTHORS.md` and rename `LICENSE` to `LICENSE.md`, whose copyright line now
names "The SimpleSplines Authors" and points at it -- otherwise AUTHORS.md's
reference to what the LICENSE mentions would not be true.
Add the `.githooks/pre-push` hook the README already told the reader to enable.
`.githooks/` did not exist, so `git config core.hooksPath .githooks` was
silently a no-op. It runs the test suite and refuses the push if it fails, with
`SIMPLESPLINES_SKIP_TESTS=1` as the documented override.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`weighted_matrix` is the one assembly that cannot be memoised, since it depends on the field, so a time integrator asks for a different one inside every Newton iteration -- the call pattern under which allocation matters most. It costs 260 kB a call at N = 128, p = 3, and only 5 kB of that is a temporary; the rest is a freshly built SparseMatrixCSC whose structure is recomputed each time even though the sparsity pattern of Phi_a diag(f w) Phi_b' does not depend on f. Recorded rather than fixed: the fix needs a `weighted_matrix!` entry point, because the present signature has nowhere to write, and the sparse triple product has to be written out by hand against a cached pattern instead of delegating to SparseArrays. That widens the API, so it is the caller's call. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`l2_projection!` took the quadrature's `f ⊙ w` buffer unconditionally, which narrowed the
element types the method accepts: a complex sample threw an `InexactError` where the
allocating `l2_projection` -- the very method the docstring offers as the interchangeable
alternative -- returns a `ComplexF64` result, and a `BigFloat` sample was silently computed
through `Float64`. The buffer is now taken only when the product lands in its element type,
which is a test on types alone and so resolved at compile time: the ordinary path still
allocates nothing (0 B at N = 128 on a uniform mesh), and a wider sample gets a product of
its own. Verified equal to `l2_projection` to the last bit on a graded mesh.
The in-place and allocating methods are separate implementations, and only the uniform mesh
compared them, so the suite now checks them against each other across all three mesh
families and degrees 1 to 4, and pins the complex case.
The non-reentrancy warning moves from `l2_projection!` to `SplineQuadrature`, where it
belongs: the shared buffer is the second piece of mutable state on that struct, not the
first, the memoising `cache` behind `mixed_matrix` being equally unsynchronised.
- `CompatHelper.yml` gained `julia-actions/cache` without the `actions: write` permission
the action needs to prune the caches it created, which `CI.yml` grants with a comment
saying so; the daily run would have logged a permission failure.
- `Documenter.yml` instantiated the docs environment in a step of its own, which
`julia-actions/julia-docdeploy` then did again -- it runs `Pkg.develop` and
`Pkg.instantiate` on `docs/` itself. The step goes, and with it a comment describing
work it was not doing.
- `LICENSE.md` had lost its copyright year in the rename.
- `AUTHORS.md` introduced contributors as being "in addition" to the principal developers
and then listed a principal developer among them.
- `.githooks/pre-push` never read the ref lines git feeds it, so it left git writing into
a closed pipe and ran the whole suite for a push that only deletes a branch.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
michakraus
left a comment
michakraus
left a comment
There was a problem hiding this comment.
Review
Verified rather than assumed, where a claim could be checked: every action tag this PR
references resolves (actions/checkout@v7.0.1, julia-actions/setup-julia@v3.0.2,
julia-actions/cache@v3.3.0, codecov/codecov-action@v7.0.0,
julia-actions/RegisterAction@v0.3.2), the include: rows do create new matrix combinations
rather than overwriting version, both Manifest.toml and docs/Manifest.toml are
gitignored so removing the [sources] entry is sufficient, basis_integrals returning by
reference is safe because nothing in src/ or test/ mutates it, mass_solve! really is
alias-safe in both representations, and no branch protection depended on the old
Julia 1.10 - … - x64 job names. All ten CI jobs plus the docs build pass on the head
commit. The two workflow bugs it fixes are real ones.
Eight things came out of it, one of them a behavioural regression. All are fixed in
4527bd1 on this branch; the findings are recorded here so the reasoning is on the PR and not
only in a commit message.
1. l2_projection! no longer accepts what l2_projection accepts (major)
q.scratch .= q.w .* f writes into a Vector{T} owned by the quadrature, so f must now
be convertible to the quadrature's element type. Measured on GradedMesh(16, 2π), p = 3
— non-uniform, so FactorizedMass, on which the complex path is otherwise supported:
| call | on 80866d0 |
|---|---|
q.mass \ (Φ * (q.w .* fc)), fc::Vector{ComplexF64} — the pre-PR expression |
ok, ComplexF64 |
l2_projection(q, fc) — allocating, untouched by the PR |
ok, ComplexF64 |
l2_projection!(ûc, q, fc) |
InexactError |
l2_projection!(ûb, q, fb), fb::Vector{BigFloat} |
ok, but computed through Float64 |
So the two methods disagree about their domain, and the docstring points at the allocating
one as the interchangeable alternative. The BigFloat row is the worse half of it: the
narrowing is silent.
Fixed by taking the buffer only when the product lands in its element type:
S = promote_type(eltype(q.w), eltype(f))
fw = S === eltype(q.scratch) ? q.scratch : similar(f, S)A test on types alone, hence resolved when the method is compiled. Re-measured after the
fix: 0 bytes for Float64 at N = 128 on a uniform mesh (unchanged), 1296 bytes on a
graded one (the CHOLMOD temporary, as documented), and the complex result now equals
l2_projection's exactly — max difference 0.0.
Out of scope, worth knowing: a complex f on a UniformMesh still fails inside
mass_solve!, whose rfft plan cannot take a complex argument. That predates this PR and
is in mass.jl.
2. The non-reentrancy warning is on the wrong docstring (minor)
l2_projection!'s docstring says the shared buffer is "what makes this method
non-reentrant". But SplineQuadrature was already non-reentrant before the buffer existed:
mixed_matrix memoises through get!(q.cache, …) on a plain Dict, which concurrent
callers can corrupt. Warning about one and not the other implies the rest of the struct is
shareable. The warning now sits on SplineQuadrature and names both.
3. l2_projection! had no coverage off the uniform mesh (minor)
The two methods are separate implementations now, and the testset only compared them on
UniformMesh(32, 2π) — nothing would have caught the FactorizedMass path drifting. They
are now compared across all three mesh families and degrees 1 to 4, and the complex case
from finding 1 is pinned.
4. CompatHelper.yml adds julia-actions/cache without actions: write (minor)
The cache action needs that permission to prune the caches it created — CI.yml grants it
with a comment saying exactly this, and the action's README documents it. The new top-level
permissions: block granted only contents: write and pull-requests: write, so every
nightly run would have logged a permission failure on the way out. Permission added; the
cache itself is worth keeping, since it holds CompatHelper.jl and its dependencies.
5. Documenter.yml's instantiate step is redundant, and its comment describes work it is not doing (minor)
julia-actions/julia-docdeploy@v1 runs Pkg.develop(PackageSpec(path=pwd())) and
Pkg.instantiate() against docs/ itself — from its action.yml, not from memory. The
explicit step duplicated the resolve, and the comment ("so instantiating the docs
environment is all that is needed to resolve it against the checkout") credited the resolve
to the step that was not performing it. Dropped, which leaves exactly the standard template.
6. LICENSE.md lost its copyright year in the rename (minor)
Copyright (c) The SimpleSplines Authors — the file it replaced said 2025, and Trixi.jl,
whose AUTHORS.md wording this follows, uses Copyright (c) 2020-present The Trixi.jl Authors. Now 2025-present.
7. AUTHORS.md contradicts itself (nit)
The preamble says "In addition, there are contributors", and the Contributors section
then lists the sole principal developer. Reworded so the list reads as including them.
8. .githooks/pre-push never reads its stdin (nit)
git push feeds the hook one <local ref> <local oid> <remote ref> <remote oid> line per
ref; exiting without draining leaves git writing into a closed pipe. Reading them also gives
the hook something it did not have: an all-zero local oid means a deletion, and a push that
only deletes branches has nothing to test. It now drains stdin, skips the suite for a
deletion-only push, and still runs it when stdin is empty (the hook invoked by hand). All
four paths exercised. The worktree comment above git rev-parse --show-toplevel is reworded
too — it was arguing against something it then did.
Not changed
minandltsoverlap in the 1.10 series while the compat floor is1.10. Harmless, and
it resolves itself when the floor moves —minpins the declared floor exactly,lts
tracks the head of the LTS series, so the rows are not the same test even today.- The
## Open Issuessection in the changelog stays as it is, per the author's call. It is
not a Keep-a-Changelog section and the byte counts in it will age, but the alternative
(the issue tracker) loses the reasoning that goes with them.
Verification of the fixes: full suite passes (5,130 tests, up 14), the docs build and
doctests are clean with no cross-reference warnings from the new @refs, both @allocated
tests still hold, and the hook's four paths were run by hand.
Verifies the package, modernises the GitHub workflows, splits the documentation build into its own workflow, and adds the repository metadata that was missing. Three commits, reviewable independently.
Verification
Pkg.test()on Julia 1.13.0-rc2Allocations
The Cox-de Boor kernel was already exactly allocation-free and type-stable — 0 B for
evaluateatd = 0…3,p = 1,3,5, on uniform, graded and random meshes, 0 B for spline reconstruction and for a 1000-point sweep, and 0 B for aCirculantMasssolve, somass.jl's claim about that path holds. Two things on per-step paths were not:l2_projection!was in-place only in its output, building thef ⊙ wproduct and the load vector fresh on every call — 6.3 kB per projection atN = 128. The quadrature now holds thef ⊙ wbuffer and the load vector is formed withmul!straight intoû. 0 B now atN = 64/128/256on a uniform mesh; on a non-uniform mesh what remains is entirely CHOLMOD's temporary, which has no in-placeldiv!.basis_integralsrecomputedΦ₀ * wevery call, 1.1 kB a time, although∫ φᵢ dxis as much a constant of the discretisation as the mass matrix. Assembled once at construction and returned by reference now. 0 B.Both pinned by tests, in the style of the existing
mass_solve!allocation test. Results verified unchanged:l2_projection(q, f) ≈ ûon both mesh families,basis_integralsstillM * 1summing toL.weighted_matrixstill allocates 260 kB a call and is not fixed here — it needs aweighted_matrix!entry point and a hand-written sparse triple product against a cached pattern, which widens the API. Recorded under## Open Issuesin the changelog with the measured breakdown and the shape of the fix.Workflows
The docs job moves out of
CI.ymlintoDocumenter.yml, so a docs failure and a test failure are separate signals and the docs job is not queued behind the test matrix. README gains a badge for it.Actions go to
checkout@v7,setup-julia@v3,cache@v3,codecov-action@v7;RegisterActionis pinned so Dependabot can bump it, which@latestprevented. Dependabot groups action bumps into one PR. Every referenced tag was checked to resolve rather than assumed.The matrix names Julia versions by alias instead of by number:
minresolves the lower bound of thejuliacompat entry, so it tracks the declared support window instead of needing to be edited alongside it;ltsand1cover long-term-support and current stable;preandnightlyrun on Linux only undercontinue-on-error.arch: defaultreplacesx64, which tests natively on the ARM64 macOS runners rather than under Rosetta.Two live workflow bugs fixed on the way:
docs/Project.tomlpinnedCompactBasisFunctionsto an absolute path on a developer's machine via[sources]. That path does not exist on a runner, so the documentation build could only ever have succeeded locally. 0.3.1 is registered, so the entry goes; verified by deletingdocs/Manifest.toml, re-resolving from the registry and rebuilding the docs clean.CompatHelper.ymlinvokedjuliawithout installing it. The runner images no longer ship a Julia, so it failed before CompatHelper started.Repository metadata
AUTHORS.mdadded, following the NeuralNetworkParameters.jl wording.LICENSErenamed toLICENSE.md, with its copyright line now naming "The SimpleSplines Authors" and pointing at it — otherwise AUTHORS.md's reference to what the LICENSE mentions would not be true. README gains a License section..githooks/pre-pushadded. The README already told the reader to rungit config core.hooksPath .githooks, but.githooks/did not exist, so the instruction was silently a no-op. It runs the suite and refuses the push on failure, withSIMPLESPLINES_SKIP_TESTS=1as a documented override; both paths exercised.Note on the branch
Committed to a branch rather than to
maindirectly, though the previous commit went straight tomain. Fast-forward and drop the PR if that is the convention you want.🤖 Generated with Claude Code