Skip to content

Address the review findings on the polar spline space - #17

Merged
michakraus merged 1 commit into
mainfrom
polar/review-fixes
Sep 17, 2026
Merged

michakraus merged 1 commit into
mainfrom
polar/review-fixes

Conversation

@michakraus

Copy link
Copy Markdown
Member

Follow-up to the review of #15. That PR merged before these landed on it, so they come as their own change. Seven findings, none of them a bug; two are worth a measurement.

pseudo_cartesian was O(Nθ) per call

It rebuilt the chart coefficients cos.(θ) and sin.(θ) over all Nθ Greville abscissae on every call. They are fixed for the basis, so PolarSplineBasis caches them in the constructor instead. The routine is called once per quadrature point — some 65 000 calls at 64×128 — by both the test suite and the theory page.

Two cold processes, same script, 64×128 cubic:

before after
time 1389 ns 63 ns
allocation 3552 B 192 B

For scale, an ordinary evaluate on this basis is 368 B / 125 ns.

A single point written as a vector reached the wrong method

evaluate(B, û, X::AbstractVector) is the vector-of-points method, so evaluate(B, û, [0.3, 1.0]) reached it and failed with a bare BoundsError rather than evaluating at the point (0.3, 1.0). It now dispatches on the element type, AbstractVector{<:Union{Tuple, AbstractVector}}, and a vector point reaches the single-point method.

This was filed in #15 as pre-existing. It is not: TensorProductBasis has four evaluate methods and none of them takes a vector of points, and the one-dimensional form cannot hit it because a one-dimensional point is a scalar. The method is new with the polar space.

The rest

  • vec rather than [:] in the two grid comprehensions, which drops a copy of the whole quadrature grid from the per-Newton-iteration path of weighted_matrix.
  • The documented signature of PolarSplineQuadrature said quadrature_order.(degree(B)) where the source says map(quadrature_order, degree(B)). Equivalent, but the rendered signature should match.
  • A stray second blank line before the 0.2.0 heading in the CHANGELOG.
  • Tests for the meshwidth, mesh and domain forwards, for show, and for the new dispatch. Seven assertions, so the polar testset goes 391 → 398.

One finding from the review is not in this diff: scripts/polar_mass_cost.jl has no assertion and no exit, against #15's claim that all four scripts exit non-zero on failure. A timing script needs no threshold, so the description was what was wrong, and #15's body is corrected in place.

Verification

Full suite green on this base: 16 testsets, 0 failures, the polar testset at 398. Aqua passes all eight checks. JuliaFormatter, fatou lint and the NFC check are clean on the three changed files.

One gap worth naming: the local manifest resolved ContinuumArrays 0.20.10, not the 0.21 that main has just admitted, so the 0.21 path is tested here only by CI.

No CHANGELOG entry: the polar space is unreleased, the existing entry describes the end state, and none of its figures move.

🤖 Generated with Claude Code

`pseudo_cartesian` rebuilt the chart coefficients `cos.(θ)` and `sin.(θ)`
over all `Nθ` Greville abscissae on every call, so a routine called once
per quadrature point was `O(Nθ)`. They are fixed for the basis, so
`PolarSplineBasis` now caches them: at 64x128 cubic, one call goes from
3552 B / 1389 ns to 192 B / 63 ns, measured in two cold processes with
the same script.

The vector-of-points `evaluate` dispatched on a bare `AbstractVector`, so
a single point written as `[s, θ]` reached it and failed with a bare
`BoundsError`. It now dispatches on the element type.

Also: `vec` rather than `[:]` in the two grid comprehensions, which drops
a copy from the per-Newton-iteration path of `weighted_matrix`; the
documented signature of `PolarSplineQuadrature` matches the source again;
and tests for the `meshwidth`, `mesh` and `domain` forwards, for `show`,
and for the new dispatch.

Co-Authored-By: Claude Opus 5 <[email protected]>
Copilot AI lite review requested due to automatic review settings September 17, 2026 21:26

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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@michakraus
michakraus enabled auto-merge (squash) September 17, 2026 21:29
@michakraus
michakraus merged commit 100af68 into main Sep 17, 2026
10 checks passed
@michakraus
michakraus deleted the polar/review-fixes branch September 17, 2026 21:30
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