Skip to content

GH-40: Centralize and document epsilon/tolerance constants#145

Merged
ehkropf merged 3 commits into
mainfrom
gh-40-epsilon-tolerances
Jun 14, 2026
Merged

GH-40: Centralize and document epsilon/tolerance constants#145
ehkropf merged 3 commits into
mainfrom
gh-40-epsilon-tolerances

Conversation

@ehkropf

@ehkropf ehkropf commented Jun 14, 2026

Copy link
Copy Markdown
Owner

Closes #40 (centralization scope; hazard guards split to #144).

Context

Issue #40 asked for a codebase-wide review of epsilon selection and scaling. An audit found its original trigger — a rho_val / (z_0 - c_val) division — was already guarded, and that the real problem was consistency: the codebase had exactly one centralized tolerance (BOUNDARY_TOLERANCE in Types.h) and 15+ scattered, undocumented magic numbers (1e-14, 1e-15, 1e-10, 1e-6, 100·ε).

Per discussion, the work was split along its risk boundary into two issues. This PR is the value-preserving half; the genuine behavior-changing hazard guards are tracked in #144.

What this PR does

  1. Adds src/core/Tolerances.h — named constexpr constants grouped by meaning (not value), with a rationale comment block: domains are unit-disk-normalized, so O(1) operands make absolute machine-epsilon-scaled tolerances appropriate; SPLINE_CLOSURE_EPS is intentionally relative because it compares un-normalized user input. Includes a 'do not retune without re-running thesis regression tests' warning.
    • GEOMETRIC_COINCIDENCE_EPS (1e-14), NEWTON_UPDATE_SCALE_EPS (1e-14), PIVOT_EPS (1e-15), SPLINE_CLOSURE_EPS (100·ε, relative), GRID_GEOMETRY_EPS (1e-10), FINITE_DIFFERENCE_STEP (1e-6).
  2. Replaces the scattered literals across FornbergMC, PMatrixBuilder, Domain, SplineBoundaryComponent, RootFinder, and Grid with these constants. Every value is byte-identical → no behavior change.
  3. The PMatrixBuilder coincident-center error message no longer hardcodes 1e-14 (which would drift from the constant); it reports the actual distance and references the coincidence tolerance. Its test assertion is updated to match.

BOUNDARY_TOLERANCE stays in Types.h (domain-level), cross-referenced from the new header. FornbergMCConfiguration tolerances are left as-is (already named and configurable).

Verification

  • Release build clean (2 pre-existing unused-parameter warnings only).
  • ctest: 306/306 pass, including the thesis examples (thesis2/thesis4/thesis5) — the regression guardrail for the value-preserving claim.

Out of scope (tracked elsewhere)

🤖 Generated with Claude Code

ehkropf and others added 3 commits June 14, 2026 20:30
Introduce a central header for the previously scattered near-zero / division-guard
tolerances, grouped by meaning with a rationale comment block explaining the
absolute-vs-relative choice (domains are unit-disk normalized, so O(1) operands make
absolute machine-epsilon-scaled tolerances appropriate). SPLINE_CLOSURE_EPS is kept
relative because it compares un-normalized user input.

No call sites changed yet; behavior unchanged.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the duplicated/scattered near-zero and division-guard literals across
FornbergMC, PMatrixBuilder, Domain, SplineBoundaryComponent, RootFinder, and Grid
with the named constants from Tolerances.h. Each substitution preserves the exact
value, so behavior is unchanged (full suite incl. thesis examples stays green).

The PMatrixBuilder coincident-center error message no longer hardcodes the literal
'1e-14' (which would drift from the constant); it now reports the actual distance and
references the coincidence tolerance. The corresponding test assertion is updated to
match.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Blind-review follow-ups:
- Convert the second central-difference step (DiscreteBoundaryComponent::
  evaluateDerivative, BoundaryComponent.cpp) to FINITE_DIFFERENCE_STEP; it was
  missed in the initial sweep while its Domain.cpp sibling was converted. Value
  identical, behavior unchanged.
- Reword the NEWTON_UPDATE_SCALE_EPS comment: it guards a tangent-magnitude
  divisor, so the honest distinction from GEOMETRIC_COINCIDENCE_EPS is that it
  guards a different quantity (tune independently), not that it 'does not test
  coincidence'.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@ehkropf

ehkropf commented Jun 14, 2026

Copy link
Copy Markdown
Owner Author

Blind review (4 specialized agents)

Fanned out four reviewers against the diff: code-reviewer, comment-analyzer, silent-failure-hunter, pr-test-analyzer. No blocking findings — the value-preservation claim was verified site-by-site from every angle.

Verified

  • Value preservation (code-reviewer): every literal→constant substitution is byte-identical; no off-by-magnitude swaps (no 1e-15 site got a 1e-14 constant or vice versa); semantic grouping correct; include hygiene correct; no control-flow changes.
  • Error handling (silent-failure-hunter): all throw sites keep their operator, direction, threshold, and exception type. The PMatrixBuilder message reword still reports the actual distance + actionable guidance; no errors silently swallowed.
  • Tests (pr-test-analyzer): the passing 306-test suite incl. thesis2/4/5 convergence is the right regression net for a value-preserving refactor. The updated assertion (coincidence tolerance vs 1e-14) is correctly de-coupled from the implementation detail, not weakened.

Acted on

  • Missed FD step (comment-analyzer, medium): a second const double h = 1e-6; in DiscreteBoundaryComponent::evaluateDerivative (BoundaryComponent.cpp) was not converted while its Domain.cpp sibling was. Fixed in b471f90 — converted to FINITE_DIFFERENCE_STEP, completing the centralization. Suite still 306/306.
  • Comment wording (low): reworded the NEWTON_UPDATE_SCALE_EPS doc — it guards a tangent-magnitude divisor, so the honest distinction from GEOMETRIC_COINCIDENCE_EPS is 'different quantity, tune independently', not 'does not test coincidence'.

Noted, not acted on (out of scope / pre-existing)

  • FornbergMC::map() hole-center guard is untested (severity 6) — pre-existing gap, not introduced here; the shared constant is regression-guarded at other call sites. Worth a small targeted throw-test in future.
  • Optional: interpolate GEOMETRIC_COINCIDENCE_EPS into the PMatrixBuilder message to restore the printed numeric threshold without reintroducing a drift-prone literal. The silent-failure-hunter explicitly did not consider this a blocker.
  • Grid single-line division + RootFinder NaN/inf guards remain deferred to Guard genuine division/NaN hazards surfaced by the #40 epsilon audit #144 as planned.

@ehkropf ehkropf merged commit 0785318 into main Jun 14, 2026
@ehkropf ehkropf deleted the gh-40-epsilon-tolerances branch June 14, 2026 20:03
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.

Review codebase for proper epsilon selection and scaling

1 participant