Finding-E from doc/reviewer/external-audit-2026-05-30.md.
Scan also uncovered the same issue in inversive_distance_functional.hpp.
Three functions used absolute error `|analytic - fd| > tol` while the
rest of the library (euclidean_functional, spherical_functional,
euclidean_hessian, hyper_ideal_functional) all use relative error
`|analytic - fd| / max(1, |analytic|) > tol`.
Absolute error is too strict for large gradients (false failures) and
too lenient for small gradients.
Fixed:
cp_euclidean_functional.hpp gradient_check_cp_euclidean()
cp_euclidean_functional.hpp hessian_check_cp_euclidean()
inversive_distance_functional.hpp gradient_check_inversive_distance()
All three now use the relative criterion and accumulate all failures
before returning (ok=false instead of early return on first mismatch).
Default tol updated from 1e-6/1e-5 to 1e-4, matching the Java
FunctionalTest convention used by all other checks in the library.
Error message updated to print rel-err instead of raw diff.
266/266 CGAL tests pass, 0 failed.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Finding-D from doc/reviewer/external-audit-2026-05-30.md.
All three DOF-assignment functions iterated over every vertex
unconditionally, overwriting any v_idx set before the call. The
documentation said "pin before OR after" — the "before" option was
silently wrong (the pin would be overwritten). For the
inversive-distance variant the doc explicitly said the pre-call pin
would make the function "a no-op for that vertex", which was false.
Changes (three headers):
- euclidean_functional.hpp assign_euclidean_vertex_dof_indices()
- spherical_functional.hpp assign_vertex_dof_indices()
- inversive_distance_functional.hpp assign_inversive_distance_vertex_dof_indices()
For each:
1. Single-arg overload: doc corrected to "pin AFTER, not before;
pre-call pins are overwritten"
2. New two-arg overload accepting a Vertex_index gauge: pins the
requested vertex (v_idx=-1) in a single pass, preventing the
user error entirely
Three new GTests in test_euclidean_functional.cpp:
SingleArg_PinBeforeHasNoEffect — documents the old pitfall
TwoArg_GaugeIsPinnedOthersAreSequential — verifies the new overload
TwoArg_NewtonConvergesWithGaugeOverload — end-to-end correctness
266/266 CGAL tests pass, 0 failed.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Finding-C from doc/reviewer/external-audit-2026-05-30.md.
The box comment at the top and the function-level comment above
euclidean_cot_weights() both stated:
cot_k = (t_adj1·l123 − t_adj2·t_opp) / denom2 ← WRONG
The correct formula (verified numerically on a 3-4-5 right triangle,
expected cot1=4/3, cot2=3/4, cot3=0) is:
cot_k = (t_opp · l123 − t_a · t_b) / denom2
where t_opp is the t-value of the edge OPPOSITE vertex k, and t_a/t_b
are the t-values of the two edges ADJACENT to vertex k.
The implementation in euclidean_cot_weights() was already correct;
only the documentation was wrong.
Changes (documentation only, zero code changes):
- Box comment: rewritten with correct formula and explicit per-vertex
assignment (cot1: t_opp=t23, t_a=t12, t_b=t31; etc.)
- Box comment: added missing ½ factor to Hessian contribution lines
- Function-level comment: corrected to (t_opp·l123 − t_a·t_b)/(8·Area)
with a pointer to the box comment for the full assignment
- Inline return comment in euclidean_cot_weights(): now shows the
mapping (cot1: t_opp=t23, t_a=t12, t_b=t31) directly at the formula
263/263 CGAL tests pass, 0 failed.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Finding-B from doc/reviewer/external-audit-2026-05-30.md.
The Euclidean Gauss–Bonnet identity Σ(2π−Θ_v) = 2π·χ is ONLY valid for
flat/Euclidean and spherical metrics. For hyperbolic metrics (HyperIdeal)
the correct identity is Σ(2π−Θ_v) − Area(M) = 2π·χ. The previously
provided gauss_bonnet_sum(HyperIdealMaps) overload would silently pass the
wrong LHS to check_gauss_bonnet, which would always throw "deficit = ±Area"
for valid hyperbolic targets.
Fix:
- gauss_bonnet_sum(mesh, HyperIdealMaps) → = delete + explanation comment
- enforce_gauss_bonnet(mesh, HyperIdealMaps&) → = delete + explanation comment
- Header box comment rewritten with the correct hyperbolic Gauss–Bonnet
identity and a clear "HyperIdeal: NOT SUPPORTED" section
New test in test_phase6.cpp:
- HyperIdeal_EuclideanSumDiscrepancy_DocumentsWhyCheckIsDeleted
verifies numerically that the Euclidean sum = 0 but 2π·χ = 4π for a
regular tetrahedron, documenting the −4π discrepancy that motivated
the deletion
- Three compile-time static_asserts (SFINAE) confirm the overload is not
invocable with HyperIdealMaps but remains so with Euclidean/SphericalMaps
263/263 CGAL tests pass, 0 failed.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Follow-up to the Finding-A fix (face_energy guard). The throw is the
correct safe behaviour for now; the mathematically complete solution
requires formulas for semi-ideal and fully-ideal tetrahedra that are
absent from the Java reference.
Adds a structured research entry to research-track.md (Phase 9b+):
- primary references: Kolpakov-Mednykh arXiv:math/0603097, Milnor 1982,
Vinberg 1985
- acceptance criteria: two new volume functions + gradient-check tests +
limiting-behaviour continuity witness
- explicit note that the throw in face_energy() must not be removed
without implementing and testing the replacement formulas
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Finding-A from doc/reviewer/external-audit-2026-05-30.md.
Root cause: both the Java reference (HyperIdealFunctional.java:222-231)
and the C++ port silently applied the one-ideal-vertex volume formula
to the first ideal vertex found in a face, ignoring any additional ideal
vertices. For two or three ideal vertices this produces a wrong energy
value with no diagnostic.
Fix: add an ideal_count guard at the top of face_energy() that throws
std::logic_error for ideal_count >= 2. The one-ideal (Kolpakov-Mednykh)
and zero-ideal (Meyerhoff/Ushijima) paths are unchanged and correct.
Three new GTests cover the three guard cases:
MultiIdealGuard_TwoIdealVertices_Throws (two ideal → throw)
MultiIdealGuard_AllThreeIdealVertices_Throws (all ideal → throw)
MultiIdealGuard_ExactlyOneIdeal_DoesNotThrow (one ideal → no throw)
262/262 CGAL tests pass, 0 failed.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Full external code review of v0.10.0 against all math-critical headers
(direct source read, not automated scan). Documents 5 open bugs/API errors,
3 test gaps inherited from java-port-audit.md, 1 architectural CI risk,
and 5 minor findings. Each finding is self-contained with file+line
references, a concrete fix proposal, and acceptance criteria so a new
session can pick up any single finding without prior context.
Key findings:
- FINDING-A (critical): face_energy() wrong for mixed ideal/hyper-ideal configs
- FINDING-B (medium): Gauss–Bonnet API conceptually wrong for HyperIdeal
- FINDING-C (medium): cotangent formula in euclidean_hessian.hpp header is wrong
- FINDING-D (medium): DOF-assignment doc falsely claims "pin before" works
- FINDING-E (medium): cp_euclidean gradient_check uses absolute not relative error
- FINDING-F/G/H: three open test gaps from java-port-audit.md
- FINDING-I (arch): 246/272 CGAL tests not gated in CI
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
