Skip to content

Replace the one-page docs with a full manual - #7

Merged
michakraus merged 6 commits into
mainfrom
docs/comprehensive-manual
Sep 7, 2026
Merged

michakraus merged 6 commits into
mainfrom
docs/comprehensive-manual

Conversation

@michakraus

@michakraus michakraus commented Sep 7, 2026 •

Copy link
Copy Markdown
Member

Replaces the single-page manual with a fourteen-page one. The old docs/src/index.md was 131
lines ending in an @autodocs dump, and its opening still described the package as
periodic-only with "one basis" — untrue since the boundary conditions and tensor products
landed. Nothing in docs/src/ mentioned recombination, tensor products, Spline,
evaluate_all!, basis_integrals, weighted_matrix, local_indices or mass_solve!.

What is in it

page
Home what the package is and is not, a feature matrix, a fifteen-line worked example, a pipeline diagram
Tutorial one narrative from a bare using to a solved boundary-value problem
Theory the Cox-de Boor recursion and its derivative recursion, support, smoothness and the three dimension counts; recombination, what it costs and which polynomials each condition keeps; the tensor-product space, its flattening convention and what does not separate
Usage one page per object — meshes, bases, boundary conditions, assembly, tensor products, splines — with exact constructors, accessor tables, what is rejected, and the traps
Gallery eight solved problems, each printing what it measured
Library 81 of the 85 exports in explicit @docs blocks, the other four being re-exports it names
References a bibliography via DocumenterCitations

Everything computational is one of the 201 @example blocks, so the numbers, the rates and the
sixteen CairoMakie figures are produced by the build: a claim that stops being true stops the
build. Five gallery problems are re-derived from assertions the suite already makes; three are
new — Poisson in one dimension on a Dirichlet basis and its variable-coefficient variant
(measured order p+1), the Dirichlet eigenvalues against (kπ)², and Poisson on a box through
the separable Laplacian.

docs/make.jl gains the page tree, checkdocs = :exports, DocumenterCitations, and
prettyurls gated on CI as the sibling packages have it. size_threshold_warn/
size_threshold are raised because the library page builds to 160 KB, above Documenter's
100 KiB default warning threshold.

Two documentation defects fixed

  • findcell was undocumented. Its @doc raw block sat immediately above the internal
    _findcell and documented that, so ?findcell showed nothing, @docs findcell would have
    failed this build, and @autodocs rendered the text under SimpleSplines._findcell. Same
    class as a comment between a docstring and its definition, and equally silent.
  • A wrong complexity. evaluate and evaluate_all! gave the single-function path as
    O(p^2). _bspline is the recursion written out with no memoisation, so it splits into two
    subproblems at every level and one value costs O(2^p). Corrected in both docstrings and in
    the manual's cost table, and now archived: scripts/evaluate_cost_scaling.jl measures both
    paths at p = 3, 6, 8, 12 and fails if the exponents stop being distinguishable. From p = 6
    to p = 12 the single value costs 64× more and the block 2.6× — 2^6 against (12/6)^2.

Docstrings were added for the exports that had none: quadratures,
PeriodicBSplineDerivative, nnodes, grid, basis on a SplineQuadrature, and basis,
mass_operator, mass_matrix, nnodes on the tensor-product types. No executable line
changed anywhere in src/.

Pre-PR verification

Fixed during the pass

  • src/bspline.jl — the new findcell docstring implied that calling on a mesh allocates. It
    does not: the three non-uniform meshes return their stored breakpoint vector and
    UniformMesh computes the cell index by division. Measured 0 bytes on basis/uniform,
    basis/graded, mesh/uniform, mesh/graded and mesh/general.
  • docs/src/usage/boundary.md — "four capitalised spellings that used to mean something
    different" mis-describes _BC_RENAMED: :nothing is not capitalised, and
    :Dirichlet/:Periodic never meant anything different. Now names the four symbols and the
    generic branch's message, matching the @example below it.
  • CHANGELOG.md — "each reports its own convergence order" was false for two of the three new
    gallery problems; now says what each actually prints.
  • docs/src/gallery.md — using SparseArrays was dead on that page (0 of its 28 exports
    appear); the docs dependency itself is used by two other pages, usage/boundary.md and
    usage/quadrature.md.
  • Three gallery figures whose legends covered the curves now place the legend outside the axes;
    the home-page pipeline diagram was misaligned and is redrawn.
  • PeriodicBSplineDerivative's new docstring carried a historical note into the rendered
    manual; it now says what the alias is. One comment the new findcell docstring had made
    redundant is gone.

Checked and clean

  • All ten new or moved docstring signatures match their real methods — no drift. @doc findcell
    now resolves on the exported binding.
  • The O(2^p)/O(p^2) correction is consistent in all nine places it appears across src/ and
    docs/src/, and the figures were reproduced independently within ~10% before being replaced
    by the script's own.
  • Countable CHANGELOG claims all verified: fourteen pages, eight gallery problems, sixteen
    figures, 201 @example blocks, 81/85 exports in @docs, all eight bibliography keys cited.
  • Type piracy: the diff defines no method; Aqua.test_all runs in the suite.
  • fatou lint clean on the changed .jl files; JuliaFormatter --check clean; load test passes.

Verification run

check result
Pkg.test() 64,098 pass / 0 fail
julia --project=docs docs/make.jl exit 0
doctest(SimpleSplines) pass
scripts/evaluate_cost_scaling.jl pass
JuliaFormatter --check clean

The two remaining build warnings are benign and unavoidable: Documenter reports that it used
the image/png fallback for the 16 Makie figures — the intended behaviour, since raising
example_size_threshold would inline the large HTML instead — and, locally only, that it could
not auto-detect the building environment.

Not checked

  • Type instability and allocations have no executable line to attach to: the src/ diff is
    docstrings only. The one allocation claim written in the pass was measured.

Bibliography

All eight entries were checked against Crossref, and against the publisher's own page wherever
one is served to a fetch. Six carry a DOI:

entry DOI
Schumaker, Spline Functions 10.1017/CBO9780511618994 — Cambridge Core confirms the 3rd edition, the series and both ISBNs
Piegl & Tiller, The NURBS Book 10.1007/978-3-642-59223-2 — the 1997 second edition; the 1995 first has a DOI of its own
Höllig, FEM with B-Splines 10.1137/1.9780898717532
Cottrell, Hughes & Bazilevs 10.1002/9780470749081
Cox 1972 10.1093/imamat/10.2.134 — Oxford Academic confirms vol. 10, issue 2, pp. 134-149
de Boor 1972 10.1016/0021-9045(72)90080-9

Two carry none because none is registered for the edition cited, and references.md now says
so rather than merely noting the absence:

  • de Boor, A Practical Guide to Splines, revised edition 2001 — Crossref has no record for
    ISBN 9780387953663; the only record for the title is the 1978 first edition's eBook
    (10.1007/978-1-4612-6333-3), whose SpringerLink page returns 404 where the NURBS book's
    returns a live 303. So that page is gone, not merely blocked to a fetch.
  • Boyd, Chebyshev and Fourier Spectral Methods, Dover 2nd edition — only the 1989 Springer
    first edition is registered.

ISBNs were added alongside, and the issue numbers of the two articles. SIAM and Wiley bot-block
automated fetches (403), so those two rest on their Crossref records, both of which were
retrieved by DOI and match on title, author, publisher, year and ISBNs.

The documentation was a single 131-line index.md ending in an `@autodocs`
dump, and its opening still described the package as periodic-only with
"one basis" -- untrue since the boundary conditions and tensor products
landed. It is now fourteen pages: a tutorial, three theory pages (the
Cox-de Boor recursion and its derivative recursion; recombination; the
tensor-product space and its flattening convention), six usage pages with
the exact constructors, accessor tables and traps, a gallery of eight
solved problems, an explicit library page, and a bibliography.

Everything computational is an `@example` block, so the numbers, the rates
and the sixteen CairoMakie figures are produced by the build and a claim
that stops being true stops the build. Three gallery problems are new
rather than lifted from the suite: Poisson in one dimension on a Dirichlet
basis and its variable-coefficient variant, the Dirichlet eigenvalues
against (k*pi)^2, and Poisson on a box through the separable Laplacian.

Two documentation defects fixed along the way. The `findcell` docstring was
attached to the internal `_findcell` that follows it, so `?findcell` showed
nothing and `@docs findcell` would have failed the build. And `evaluate`'s
cost was given as O(p^2) when the recursion is unmemoised and one value
costs O(2^p) -- 15.8 microseconds against 103 nanoseconds for the whole
block at p = 12. Docstrings were added for the exports that had none.
The `findcell` docstring implied that calling on a mesh allocates, which
none of the paths does: the three non-uniform meshes hand over a stored
breakpoint vector and `UniformMesh` computes the index by division. The
"four capitalised spellings" claim in usage/boundary.md mis-described
`_BC_RENAMED` -- `:nothing` is not capitalised, and `:Dirichlet` and
`:Periodic` never meant anything else. Both reworded.

The CHANGELOG said each of the three new gallery problems "reports its own
convergence order"; only the Poisson one does. Now says what each actually
prints. `using SparseArrays` was dead on the gallery page.

The eight timings quoted for `evaluate` against `evaluate_all` had no
archived check, which the tree rule requires. They are now read off
`scripts/evaluate_cost_scaling.jl`, which measures both paths across
p = 3, 6, 8, 12 and fails if the two exponents stop being
distinguishable -- p = 6 to 12 costs the single value 64x more and the
block 2.6x, i.e. 2^6 against (12/6)^2. The quoted figures are the
script's own, so the two cannot drift apart.

Also: the three gallery figures whose legends covered the curves now put
the legend outside the axes, the home-page pipeline diagram was redrawn
aligned, the `PeriodicBSplineDerivative` docstring no longer carries a
historical note into the rendered manual, and one comment the new
`findcell` docstring had made redundant is gone.
Copilot AI lite review requested due to automatic review settings September 7, 2026 02:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Changes recommended

A few newly introduced manual/index statements are internally inconsistent or misleading (and there is at least one docstring that should be generalized to local_width(b)), so the documentation set needs small corrections before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR replaces the previous single-page documentation with a multi-page manual that more accurately reflects the current SimpleSplines feature set (boundary conditions, recombination, tensor products, splines, and key assembly/evaluation APIs), while also tightening docstring coverage and docs-build validation.

Changes:

  • Reworks docs/src/ into a structured manual (Home, Tutorial, Theory, Usage, Gallery, Library, References) with extensive executable @example blocks.
  • Adds/adjusts docstrings for several exported accessors and clarifies evaluate/evaluate_all! complexity claims, including an archived scaling script.
  • Updates the docs build (page tree, checkdocs = :exports, DocumenterCitations, CI-gated prettyurls) plus README/CHANGELOG to match.
File summaries
File Description
src/tensorproduct.jl Adds docstrings for tensor-product accessors (nnodes, basis, quadratures, mass_operator, mass_matrix).
src/quadrature.jl Adds basis(::SplineQuadrature) docstring.
src/bspline.jl Fixes/extends docstrings (complexity clarifications, findcell doc attachment, shared accessor docs, compatibility alias docs).
scripts/evaluate_cost_scaling.jl New script asserting exponential vs polynomial scaling to keep complexity claims honest.
README.md Updates package summary to reflect non-periodic bases, boundary conditions, tensor products, and points to the manual.
docs/src/index.md New/rewritten home page with feature matrix, quickstart, and navigation map.
docs/src/tutorial.md Adds a runnable end-to-end tutorial (mesh → basis → spline → projection → Poisson solve → 2D example).
docs/src/theory/bsplines.md Adds/expands theory and cost discussion for Cox–de Boor vs de Boor triangular scheme.
docs/src/theory/boundary.md Adds/expands recombination theory and implications (dimension counts, reproduction, caveats).
docs/src/theory/tensorproduct.md Adds tensor-product theory, flattening convention, Kronecker structure, and separability limits.
docs/src/usage/meshes.md Adds meshes usage guide, invariants, and traps (including breakpoints mutability warning).
docs/src/usage/bases.md Adds bases usage guide, accessors, evaluation paths, and key traps.
docs/src/usage/boundary.md Adds boundary-condition usage guide, symbol sugar, and recombined-basis specifics/traps.
docs/src/usage/quadrature.md Adds assembly/quad usage guide, tabulation semantics, memoization notes, and traps (thread safety, by-reference returns).
docs/src/usage/tensorproduct.md Adds tensor-product usage guide (construction, evaluation, assembly, Kronecker mass operator).
docs/src/usage/splines.md Adds spline usage guide (construction, evaluation, derivative objects, aliasing behavior).
docs/src/gallery.md Adds a runnable gallery of solved/measured problems with figures generated during docs build.
docs/src/library.md Replaces @autodocs with explicit @docs blocks grouped by topic; documents re-exports/owned generics.
docs/src/references.md Adds references page and explains DOI omission policy for now.
docs/src/references.bib Adds bibliography entries consumed by DocumenterCitations.
docs/Project.toml Adds docs-only deps/compat (Makie, citations, stdlibs) required by executable examples.
docs/make.jl Adds page tree, checkdocs = :exports, DocumenterCitations plugin, CI-gated pretty URLs, and size thresholds.
CHANGELOG.md Documents the manual overhaul and the docstring/complexity fixes.
Review details
  • Files reviewed: 23/23 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread docs/src/index.md Outdated
Comment thread docs/src/index.md Outdated
Comment thread src/bspline.jl
Six of the eight entries now carry a DOI, each checked against Crossref
and, where the publisher serves a page to a fetch, against that page too:
Schumaker 10.1017/CBO9780511618994 (Cambridge Core confirms 3rd edition,
Cambridge Mathematical Library, both ISBNs), Piegl-Tiller
10.1007/978-3-642-59223-2 (the 1997 second edition, not the 1995 first,
which has a DOI of its own), Hoellig 10.1137/1.9780898717532, Cottrell
10.1002/9780470749081, Cox 10.1093/imamat/10.2.134 (Oxford Academic
confirms volume 10, issue 2, pages 134-149) and de Boor
10.1016/0021-9045(72)90080-9. ISBNs added alongside, and the issue
numbers of the two articles.

Two entries still carry none, and the references page now says why rather
than merely noting the absence. No DOI is registered for de Boor's revised
edition of 2001: the only Crossref record for the title is the 1978 first
edition's eBook, and its SpringerLink page returns 404 while the NURBS
book's returns a live 303, so that is the page being gone rather than a
blocked fetch. For Boyd only the 1989 Springer first edition is
registered, not the Dover second.

Cox's journal is given under the name Crossref registers, IMA Journal of
Applied Mathematics, rather than the Journal of the Institute of
Mathematics and its Applications it appeared in: the DOI is the anchor,
and it resolves to the former.

@michakraus michakraus left a comment

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.

Review — replace the one-page docs with a full manual

Verdict: request changes. Posted as a comment rather than a formal request: GitHub refuses --request-changes on a PR from the same account ("Can not request changes on your own pull request"), so treat the verdict as stated here. Nothing here is a code defect — the src/ diff is docstrings
only, and I confirmed no executable line changed. But this is a documentation PR whose stated
premise is that "a claim that stops being true stops the build", and the six items below are
exactly the claims the build cannot check: prose assertions, and one docstring contract.
Three of them are contradicted by another page of this same PR. All are cheap to fix.

CI: green, and read from the jobs, not the conclusions. All 10 checks pass:
Julia 1 / min / pre / nightly across ubuntu, macOS and windows, Doctests - ubuntu-latest,
and Documentation (13m15s — it built, it did not skip). Nightly is green here rather than
the expected red. The Documentation log carries exactly one warning,

##[warning]For 16 @example blocks, the 'text/html' representation of the resulting

which is the image/png fallback the PR body names and defends. No failed @ref, no
checkdocs miss, no doctest failure. The second warning the PR body mentions
("could not auto-detect the building environment") does not appear on CI, consistent with its
being local-only as claimed.

Findings

# severity file:line claim evidence
1 bug src/bspline.jl:741 The evaluate_all/evaluate_all! docstring says "values must have p+1 entries". That is the contract for two of the three bases; the third throws. local_width(BSplineBasis(UniformMesh(8, 0 .. 1), 3, Natural())) == 5 against degree + 1 == 4, and src/recombined.jl:344-350 enforces local_width with a DimensionMismatch. This PR's own manual states the rule correctly twice — docs/src/usage/bases.md:187 and docs/src/usage/boundary.md:174 ("So local_width(b), never degree(b) + 1") — so the docstring is now the only text in the repo that disagrees. The line is not itself in a hunk, but the paragraph twelve lines below it is edited by this branch. Copilot raised this; agreed.
2 quality docs/src/tutorial.md:179 "recombination replaces two functions by their difference, so the constants have left the space" — said of the Dirichlet basis, where nothing is combined. recombination_matrix(BSplineBasis(UniformMesh(8, 0 .. 1), 3, Dirichlet())) is 11×9 with nnz == 9 and [length(nzrange(R, j)) for j in axes(R, 2)] == [1,1,1,1,1,1,1,1,1] — one nonzero per column, i.e. φ₁ and φ₁₁ dropped, nothing differenced. (For contrast, Neumann gives [2,1,1,1,1,1,1,1,2].) docs/src/theory/boundary.md:90 says it right: "there are no combinations at all and \varphi_1 is simply dropped — the textbook elimination". The constants leave because every ψ vanishes at the ends, not because of a difference.
3 quality docs/src/theory/boundary.md:335-337 "the recombination coefficients -a_t/a_{m+1} overflow" and "cholesky(…; check = false) reports success on a matrix containing Inf". Neither holds, and the sentence contradicts its own opening clause "the mass matrix comes out finite". For Robin(1.0, 1e-20) on UniformMesh(8, 0 .. 1), p = 3: maximum(abs, R) = 4.17e18, any(isinf, R) = false, any(isnan, R) = false; maximum(abs, M) = 4.81e35, any(isinf, M) = false; cond(M) = Inf; issuccess(cholesky(Symmetric(M); check = false)) = true. So the mechanism is catastrophic scaling, not overflow. docs/src/usage/boundary.md:212-215 states it correctly and should be the model.
4 quality docs/src/index.md:22 The feature row lists Periodic among the conditions available "per end". src/boundary.jl:331-333 throws ArgumentError("Periodic identifies the two ends of the domain, so it cannot be given for one end only…"). docs/src/theory/boundary.md:30 and docs/src/usage/boundary.md:78 both say so, the latter with an @example demonstrating the rejection. Copilot raised this; agreed.
5 quality docs/src/index.md:105 "Gallery — seven solved problems". docs/src/gallery.md:7 opens "Eight problems, solved and measured" and carries eight numbered ## headings (grep -n '^## ' docs/src/gallery.md → 8 lines, 1.–8.). Copilot raised this; agreed.
6 quality CHANGELOG.md (the "One complexity claim corrected" paragraph) Quotes 27 ns / 15.8 µs for evaluate and 17 ns / 103 ns for the block. src/bspline.jl:38-44 and docs/src/theory/bsplines.md:333-338 quote 26 / 15.5 µs and 21 / 104 ns. Both sets cannot be "the script's own". I ran the archived script on this branch (Julia 1.13.0-rc4, --check-bounds=auto, fresh process): p=3 → 26.3 / 22.5, p=6 → 238.5 / 38.8, p=8 → 959.9 / 57.5, p=12 → 15438.0 / 106.2; p = 6 → 12: evaluate ×64.7, block ×2.7. The docstring and the theory table reproduce; the CHANGELOG's 17 ns block figure at p = 3 does not (22.5 ns measured). The PR body says these figures "are now read off scripts/evaluate_cost_scaling.jl, so the two cannot drift apart" — the CHANGELOG was not brought along in 8ec08d9.
7 nit docs/src/gallery.md:148 "At nq = 2 on a random mesh the defect is 10^{-3}, not 10^{-15}." Measured 9.26e-3 — and RandomMesh defaults to seed = 1 (src/mesh.jl:290), so this is deterministic, not a draw: the @example eleven lines above prints 0.0093 while the prose says 10^-3. Either say 10^{-2}, or drop the exponent and let the block speak.
8 nit scripts/evaluate_cost_scaling.jl:4,10 "The docstrings used to give the single-function path as O(p^2)" and "which is why the wrong figure survived" are historical notes in a comment. This is the rule the same PR applied when it stripped a historical note out of PeriodicBSplineDerivative's docstring; CHANGELOG.md is where this belongs, and it is already there. The script reads fine stating only what it measures and why.
9 nit docs/src/library.md:11-19 "Five accessors are shared with the rest of the ecosystem rather than defined here" is followed by an enumeration of eight names — basis, degree, nodes, nnodes, order, nbasis, Basis, grid. Five is the GeometricBase subset only. Also, nnodes and grid are the only two of the linkable ones left as bare code spans while basis/degree/nodes/order/nbasis are @ref'd, though both are in @docs blocks at lines 76-77 — if that asymmetry is deliberate (an ambiguous-@ref dodge) it is invisible.
10 nit docs/Project.toml:3 CompactBasisFunctions is a docs dependency with no using anywhere under docs/ — it appears only in prose (index.md:110, library.md:14, usage/bases.md:99). Pre-existing rather than introduced here, so flagging rather than blocking — but this PR removed using SparseArrays from gallery.md on exactly this rationale and it edits docs/Project.toml, so it is the natural place. Separately, the PR body says the SparseArrays docs dependency "is used by three other pages"; grep -rn "using SparseArrays" docs/src finds two — usage/boundary.md:14 and usage/quadrature.md:24.

Verified and fine

Everything below was re-measured on origin/pr/7 in a detached worktree, Julia 1.13.0-rc4,
--check-bounds=auto, fresh process per script.

  • The complexity correction itself is right, and the archived script earns its keep. The
    exponents separate cleanly and the assertion bounds (>20× / <8×) are neither vacuous nor
    brittle at the measured 64.7× / 2.7×. O(2^p) for _bspline and O(p^2) for the block
    is the correct reading, and the correction is consistent across src/bspline.jl:38, :576,
    :759 and docs/src/theory/bsplines.md:320-346.
  • findcell allocates nothing on every path, as the new docstring claims. @allocated = 0
    for all ten of mesh/basis × uniform, graded, random, general, periodic-graded,
    recombined-graded. breakpoints(UniformMesh) does hand back a fresh vector
    (breakpoints(m) === breakpoints(m) is false) while GradedMesh hands back its own —
    both exactly as usage/meshes.md:101-108 warns.
  • usage/quadrature.md's allocation claims all hold. weighted_matrix at N = 128, p = 3,
    nq = 5: 265536 bytes = 259.3 kB, against the page's "about 260 kB". l2_projection! is 0
    bytes on the circulant (periodic uniform) and banded (Dirichlet) paths and 1296 bytes on
    the FactorizedMass path — precisely the "every representation but one" the page names.
    mass_solve! is 0. mass_matrix, basis_integrals, basis_values and mixed_matrix are
    all ===-identical across calls, so the "returned by reference" warnings are accurate, and
    stiffness_matrix(q) === mixed_matrix(q, 1, 1) holds.
  • The theory pages' arithmetic. n(p+1) - (n-1)p = n + p; clamped knot vector length
    n + 2p + 1 (10 for n=5, p=2, measured); five periodic images stored (length(knotvector) == 25 for n=5, matching the src/bspline.jl:497 comment); (3+1)^3 = 64; 41² = 1681,
    41³ = 68921; 9 + 4 - 2 = 11; Greville formula; ∂₁(x₁²x₂) = 0.84 and ∂₂ = 0.09 at
    (0.3, 1.4); √(32/20000) ≈ 4 %; the 26 MB kron estimate at 64² cubic cells.
  • usage/bases.md end to end. local_indices(periodic, 1) == [6, 7, 8, 1] as the prose
    says; local_indices(clamped, 3) == [3, 4, 5, 6];
    PeriodicBSplineBasis(16, 3) == BSplineBasis(UniformMesh(16, 2π), 3, Periodic()); the
    deposition sums to 3.5 with the outside particle contributing nothing; nnodes == nbasis.
  • usage/boundary.md's traps. constraint_order(Constraint(0,0,0,1)) == 3,
    constraint_order(Constraint(1,0)) == 0 (trailing zero does not count); n + p less one per
    constrained end gives 10 at n=8, p=4 for all four conditions; the Dirichlet
    basis_integrals / mass_matrix * ones discrepancy is 0.0109, i.e. real.
  • theory/boundary.md's partition-of-unity table. Measured on cubics over six cells:
    Free 3.3e-16, Dirichlet 1.0, Neumann 3.3e-16, Natural 6.7e-16, Robin(1.5,-0.7)
    0.0712 — so "Neumann and Natural keep it, Dirichlet loses it outright, Robin with α ≠ 0 loses
    it partially" is right. And non-negativity: Natural gives min ψ = -1.193 while the other
    four give 0.0, matching "it is Natural that loses it".
  • Gallery rates. Poisson 1D measured rates [3.01, 3.00, 3.00], [4.05, 4.01, 4.00],
    [4.91, 4.97, 4.99] for p = 2, 3, 4 — order p+1 as claimed. Dirichlet eigenvalues from
    above holds at the p = 3 the @example checks. The deposition sweep over 20 000 particles is
    96 bytes, i.e. the one scratch buffer and nothing per particle, and sums to exactly 1.0.
  • Structure and countables. 85 exports, 81 in explicit @docs blocks, the four omitted
    being exactly the re-exports library.md names (Basis, .., leftendpoint,
    rightendpoint) — so the 81/85 claim is right and checkdocs = :exports passes, as the docs
    job confirms. 14 pages in the make.jl tree, 201 @example blocks, 16 Figure( calls.
  • fatou lint / formatter. JuliaFormatter with SciMLStyle reports clean, no rewrite, on
    all five changed .jl files including the new script.
  • Removing using SparseArrays from gallery.md was right — none of its exports appears on
    that page.
  • The docstring-attachment fix is real. The @doc raw block did sit above _findcell; it
    now sits above findcell(m::Mesh, x), and _findcell is moved up with a comment saying why
    it carries no @doc. Exactly the failure mode the project memory records as invisible to
    formatter, lint, load and Pkg.test().

Not checked

  • The bibliography DOIs. api.crossref.org is outside this session's network allowlist —
    the fetch was denied — so all eight entries rest on the PR body's own verification. The
    metadata is internally consistent and the DOI prefixes match the stated publishers
    (10.1017 Cambridge, 10.1007 Springer, 10.1137 SIAM, 10.1002 Wiley, 10.1093 OUP,
    10.1016 Elsevier), but that is a plausibility check, not a confirmation. The two
    deliberate omissions are now explained on the page rather than merely present, which is the
    right shape either way.
  • The 64,098-pass figure. Not re-run locally; the ten green CI jobs across four Julia
    versions and three platforms are the evidence I am relying on, and Pkg.test() uses
    --check-bounds=yes, which skips guarded @allocated assertions — so the allocation numbers
    above are mine, measured at auto, not CI's.
  • Type instability and allocation regressions. No executable line changed in src/, so
    there is nothing for @code_warntype or a benchmark to attach to. The allocation claims the
    diff makes were all measured, above.
  • Rendered output. I read the sources and the build log, not the deployed HTML — figure
    legends, the redrawn pipeline diagram and the 16 PNGs are taken on the author's report.

Corrects several factual errors flagged in PR #7 review: the
evaluate_all! docstring's entry count for a recombined (Natural)
basis, the Dirichlet recombination description, contradictory
overflow language in the Robin boundary-condition writeup (theory
page and CHANGELOG), Periodic's per-end listing, the gallery problem
count, stale benchmark numbers in the CHANGELOG, an imprecise defect
magnitude in the gallery, historical language in a script comment,
and an undercounted accessor list in the library page.
@michakraus

Copy link
Copy Markdown
Member Author

Pushed bff6958 addressing every finding from the review, both mine and Copilot's:

  1. evaluate_all/evaluate_all! docstring now says local_width(b) entries, not p+1 — the Natural basis has width p+2.
  2. Tutorial: the Dirichlet recombination paragraph no longer claims a combination happens where none does — Dirichlet just drops the offending function at each end.
  3. Theory/boundary.md and the CHANGELOG's Open Issues section: dropped the wrong "coefficients overflow" / "matrix containing Inf" claims — the matrix stays finite (measured max|M| ≈ 4.8e35), only its condition number goes to Inf.
  4. index.md: Periodic is no longer listed among the per-end conditions.
  5. index.md: "seven" solved problems → "eight", matching the gallery.
  6. CHANGELOG benchmark numbers now match the docstring/theory-page figures instead of disagreeing with them.
  7. gallery.md: the quadrature-defect magnitude corrected to ≈10^{-2} (measured 9.26e-3).
  8. scripts/evaluate_cost_scaling.jl: dropped the "used to"/"survived" historical framing in the comment.
  9. library.md: "Five accessors are shared" → "Eight names", matching the enumeration in the same sentence.

Left docs/Project.toml's unused CompactBasisFunctions dep (finding 10) alone — it predates this PR and isn't part of this diff, so it's out of scope here.

@michakraus michakraus left a comment

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.

Re-review after bff6958 — seven of nine findings closed, two still open

Reviewed at bff6958 ("Fix review findings on the manual"), which landed ten minutes after the
previous review. It is a genuine fix commit: 21 insertions, 23 deletions across eight files, and
no executable line changed — the src/ half is still docstrings only.

Verdict: two items worth a follow-up commit, both one-line prose fixes. Neither is a code
defect. One is the previous finding #1 surviving one paragraph above where it was fixed; the
other was introduced by the fix to finding #9.

CI: green, 10/10, read from the check runs on bff6958 itself — not from the PR
conclusion. Julia 1 / min / pre / nightly across ubuntu, macOS and windows all success,
Documentation success, and Doctests - ubuntu-latest success (it was in_progress at the
start of this review and has since completed). Nightly is green again rather than the expected
red.

Closed, verified

# was now
2 tutorial claimed Dirichlet recombination "replaces two functions by their difference" docs/src/tutorial.md:178 now says it "just drops the one function at each end that fails to vanish there" — which is what the 11×9 matrix with one nonzero per column actually does
3 -a_t/a_{m+1} "overflow", cholesky "on a matrix containing Inf" "become huge" and "reports success on it anyway", in both docs/src/theory/boundary.md:335 and the parallel CHANGELOG paragraph. Matches the measurements: `max
4 index.md:22 listed Periodic among the per-end conditions now "per end, plus Periodic for the whole basis"
5 index.md:105 "seven solved problems" "eight"
6 CHANGELOG quoted 27 / 15.8 µs / 17 / 103 ns against the docstring's 26 / 15.5 / 21 / 104 CHANGELOG now carries 26 / 15.5 µs / 21 / 104 ns, so all three places agree and the "read off the script" claim holds
7 gallery called the nq = 2 defect 10^{-3} where the block prints 0.0093 \approx 10^{-2}
8 historical note ("used to give", "why the wrong figure survived") in the script header removed; the comment now states only what it measures

Still open

1. src/bspline.jl:737 — finding #1 is fixed in the second paragraph and left standing in the
first.
The corrected line reads values must have local_width(b) entries — p+1 for a
clamped or periodic basis, more for a recombined one. Four lines above it, the docstring's
opening sentence still says:

The d-th derivatives at x of the p+1 basis functions that do not vanish there,
returned as (j₀, values)

Measured on this branch (Julia 1.13.0-rc4, fresh process):

basis          p+1   local_width   length(evaluate_all)   nonzero at x=0.03
(clamped)       4         4                4                    4
Natural()       4         5                5                    3
Dirichlet()     4         4                4                    3
Neumann()       4         4                4                    3

and the in-place form on a p+1 buffer:

DimensionMismatch: evaluate_all! needs a buffer of 5 entries for this basis, got 4;
                   see `local_width`

So for Natural() the returned block is five wide, evaluate_all allocates
local_width(b) (src/bspline.jl:662), and the summary line is now the one sentence in the
docstring that disagrees with the paragraph below it. Same fix as before: local_width(b), or
"of the basis functions that do not vanish there" with the count left to the next paragraph.

And the corrected sentence over-generalises in the other direction. It reads "p+1 for a
clamped or periodic basis, more for a recombined one" — but three of the four recombined
conditions give exactly p+1:

Dirichlet()       RecombinedBSplineBasis   local_width = 4   p+1 = 4
Neumann()         RecombinedBSplineBasis   local_width = 4   p+1 = 4
Natural()         RecombinedBSplineBasis   local_width = 5   p+1 = 4
Robin(1.0, 1.0)   RecombinedBSplineBasis   local_width = 4   p+1 = 4

Natural is the outlier because width is maximum(lastcol[c] - firstcol[c] + 1 …) over the
cells (src/recombined.jl:129) — the widest column span of R — so it exceeds p+1 only where
the condition's stencil does, which for a cubic is the second-derivative condition and not the
first-derivative ones.

local_width's own docstring already says this correctly, and it is pre-existing rather
than part of this branch (src/recombined.jl:313-314, from c77d387):

p+1 for a BSplineBasis or a PeriodicBSplineBasis. For a
RecombinedBSplineBasis it can be larger, because a recombined function near an
end spans the union of two parent supports.

So the new line is a strictly stronger claim than the package's own statement of the same fact,
four hundred lines away. "can be more for a recombined one" matches it; simply pointing at
local_width and stopping matches it too.

2. docs/src/library.md:11 — "Eight names" over-corrects, and double-counts Basis against
the sentence that follows it.
Finding #9 said five was the GeometricBase subset only. Eight
is the count of every non-local name in the enumeration, but one of them belongs to the next
sentence:

Eight names are shared with the rest of the ecosystem rather than defined here.
basis, degree, nodes, nnodes and order belong to GeometricBase, nbasis and
Basis to CompactBasisFunctions.jl, and grid to ContinuumArrays, so that one generic
function per accessor is extended across the packages instead of one being defined in each.
Four further names are re-exported unchanged … .., leftendpoint and
rightendpoint come from IntervalSets and DomainSets, and Basis from
CompactBasisFunctions.

Basis appears in both lists, and it is not an extended generic in either case — it is a type
used as a supertype, abstract type AbstractBSplineBasis{T} <: Basis{T} at
src/bspline.jl:112, and grep finds no method defined on it anywhere in src/. So the
rationale clause ("one generic function per accessor is extended") does not cover it.

The arithmetic confirms which sentence owns it. names(SimpleSplines) is 85; the @docs
blocks in this page hold 81 unique entries; the difference is exactly the four re-exports
.., leftendpoint, rightendpoint, Basis. The other seven shared names are all in @docs
blocks here — nnodes at library.md:76, grid at :77. Seven is the number for the
first sentence.

Minor, in the same sentence: Basis is attributed to CompactBasisFunctions while grid,
which parentmodule also reports as ContinuumArrays, is attributed to ContinuumArrays. Both
match how the package obtains them (import CompactBasisFunctions: Basis, nbasis at
src/SimpleSplines.jl:28, using ContinuumArrays at :4), so this is defensible as the import
path rather than the owner — worth a word only if the sentence is being retyped anyway.

Nits, unchanged from the previous review

  • docs/src/gallery.md:148 is now the longest line in the file at 104 characters, where
    every other line caps at 96. Introduced by this fix — \approx added four characters to a
    line that was already at the margin.
  • The @ref asymmetry in library.md:12,14 is still there. nnodes and grid are bare
    code spans while basis, degree, nodes, order and nbasis are @ref'd, although
    nnodes and grid both have @docs entries at :76-77. If it dodges an ambiguous @ref it
    is still invisible as such.
  • CompactBasisFunctions remains a docs dependency with no using under docs/ —
    docs/Project.toml:3, prose-only at index.md:110, library.md:14,18 and
    usage/bases.md:99. Pre-existing, so not blocking, and unchanged by this commit.
  • The PR body's "the docs dependency itself is used by three other pages" (of
    SparseArrays) still reads three; grep -rn "using SparseArrays" docs/src finds two,
    usage/boundary.md:14 and usage/quadrature.md:24. A PR-body claim only, nothing in the
    tree.

Method

Everything above was measured on bff6958 in the checked-out branch, Julia 1.13.0-rc4, one
fresh process; export and ownership counts from names(SimpleSplines) and parentmodule rather
than from grep; CI read per-check-run on the head SHA rather than from the rollup conclusion.
The complexity correction, the archived script's assertion bounds and the findcell allocation
claim were verified in the previous review and are unchanged by this commit, so they were not
re-measured.

`evaluate_all`'s docstring asserted `p+1` in its opening sentence while
the paragraph below it — corrected in the previous commit — said
`local_width(b)`. The summary line was then the only text in the
docstring that disagreed with the rest, and it is wrong for a `Natural`
basis, where `local_width` is 5 against `degree + 1 == 4` and a `p+1`
buffer raises `DimensionMismatch`. The count is now stated once, in the
paragraph that owns the buffer contract.

That paragraph in turn claimed "more for a recombined one". Only
`Natural` widens the block: `Dirichlet`, `Neumann` and `Robin(1, 1)` are
all recombined and all give exactly `p+1`, because `width` is the widest
column span of `R` and so exceeds `p+1` only where the condition's
stencil does. Now "possibly more", matching `local_width`'s own
docstring.

The library page's "Eight names ... shared with the rest of the
ecosystem" counted `Basis` twice, the second time in the very next
sentence listing it among the four re-exports. `Basis` is a type used as
a supertype, never a generic extended here, so it does not belong in a
sentence about accessor generics — the distinction the CHANGELOG already
draws between joining the `Basis` hierarchy and extending the accessor
generics. Seven accessors, with `Basis` left to the re-export sentence.

No executable line changed.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@michakraus

Copy link
Copy Markdown
Member Author

Both open findings are fixed in 31c31d5. 12 insertions, 11 deletions, two files, no executable line changed.

1. src/bspline.jl — the opening sentence no longer carries a count, so the buffer contract
is stated once, in the paragraph that owns it. That paragraph's "more for a recombined one" is
now "possibly more", which is what local_width's own docstring has said all along.

2. docs/src/library.md — "Seven accessors", with Basis removed from that list and left
only in the re-export sentence. Confirming the categorisation after the fact: CHANGELOG.md:281
already draws exactly this distinction — "The bases join the Basis hierarchy of
CompactBasisFunctions and extend the accessor generics of GeometricBase" — so the page
now matches the changelog rather than contradicting it. @docs entries still 81 against 85
exports; the paragraph's longest line went 95 → 94.

Swept the tree afterwards: no remaining text anywhere in src/, docs/ or CHANGELOG.md
requires a p+1 buffer or miscounts the shared accessors.

Checks

check result
julia --project=docs docs/make.jl exit 0 — checkdocs = :exports clean, all @refs resolved, doctests ran inside the build
doctests, CI's own invocation (DocMeta.setdocmeta! then doctest(m)) Pass 1 / Total 1
JuliaFormatter --check on the staged .jl clean
pre-commit hook formatting ok · fatou lint clean · loads ok

The docs build's only two warnings are the same benign pair as before — the image/png
fallback for the 16 Makie figures, and the local-only "could not auto-detect the building
environment".

No CHANGELOG entry: these are corrections to text this branch itself introduced, and the
changelog's own description of the change was already right on both points — :140 states the
local_width(b) contract correctly, and :281 the hierarchy/accessor split.

Not done — the four nits from the re-review are untouched and still open: the 104-character
line at gallery.md:148, the @ref asymmetry on nnodes/grid in library.md:12-13, the
prose-only CompactBasisFunctions docs dependency, and the PR body's "three other pages" for
SparseArrays where the tree has two.

`gallery.md`'s note on the quadrature defect had grown to 104 characters
when `\approx` was added, making it the one line in the file past the
96-column fill; rewrapped.

`nnodes` and `grid` were the only two linkable accessors in the library
page's opening paragraph left as bare code spans, although both have
`@docs` entries on that same page. They are now `@ref`s like the other
five. Both names are also exported by a dependency — `nnodes` by
GeometricBase, `grid` by ContinuumArrays — so the asymmetry could have
been an ambiguous-`@ref` dodge; it was not. The docs build resolves
both, with no ambiguity and no unresolved reference.

`CompactBasisFunctions` was a docs dependency with no `using` anywhere
under `docs/`, appearing only in prose. It had no `[compat]` entry, and
the docs environment still reaches it transitively through
SimpleSplines, so `Pkg.resolve()` reports no manifest change and the
build is unaffected. Removed, on the same rationale that took
`using SparseArrays` off the gallery page.

Verified by a full `docs/make.jl` build: exit 0, `checkdocs = :exports`
satisfied, no new warning attributable to the source.

Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
@michakraus

Copy link
Copy Markdown
Member Author

All four remaining nits are now closed, in 869e146 (three files, 5 insertions, 5 deletions) plus one edit to this PR's description.

nit done
gallery.md:148 at 104 characters rewrapped; the file is back to its own 96-column fill, and that line is no longer the outlier
nnodes / grid bare code spans in library.md now @refs, like the other five accessors in the paragraph
CompactBasisFunctions a prose-only docs dependency removed from docs/Project.toml
"used by three other pages" in the PR body corrected to two, and both are now named — usage/boundary.md and usage/quadrature.md

The @ref asymmetry was an oversight, not a dodge. That was the open question — nnodes is
exported by GeometricBase and grid by ContinuumArrays, so an unqualified @ref to either
could have been ambiguous in the docs' Main, which is a hard build error. It is not: both
resolve, and CrossReferences and CheckDocument pass silently.

Removing the dependency changes nothing but the file. CompactBasisFunctions had no
[compat] entry, no using anywhere under docs/, and the docs environment still reaches it
transitively through SimpleSplines — Pkg.resolve() reports no manifest change at all. Same
rationale that took using SparseArrays off the gallery page earlier in this branch.

Verification — full docs/make.jl, the CI invocation:

check result
exit status 0
checkdocs = :exports satisfied — no undocumented export
cross-references no ambiguous, unresolved or duplicate @ref
removed dependency no UndefVarError, LoadError or missing-package error
doctests (run inside the build) pass

One note on the warnings, so it is not mistaken for a regression later: this local run emitted a
third warning shape beyond the usual two — 8× overwriting 'build/assets/…'. That is Documenter
reusing a docs/build/ left behind by an earlier local build, not anything attributable to the
source; CI checks out clean and will not show it. The two real warnings are unchanged.

No CHANGELOG entry, for the same reason as 31c31d5: three of the four are corrections to text
this branch itself introduced, and the fourth removes a dependency the changelog never claimed
was used.

@michakraus
michakraus merged commit 9f78e53 into main Sep 7, 2026
10 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