Files
ConformalLabpp/doc/reviewer/java-port-audit.md
Tarik Moussa a3ee9576d4
All checks were successful
C++ Tests / test-fast (pull_request) Successful in 1m57s
API Docs / doc-build (pull_request) Successful in 1m3s
Markdown link check / check (pull_request) Successful in 44s
C++ Tests / test-cgal (pull_request) Has been skipped
C++ Tests / quality-gates (pull_request) Successful in 2m11s
fix+test: Euclidean holonomy/τ end-to-end + spherical edge-DOF oracle (2026-05-29 audit)
Bundles the 2026-05-29 Java↔C++ math-correctness audit (doc/reviewer/
java-port-audit.md, 11 findings) with two follow-up fixes.

Audit code changes:
- Finding 3 (spherical_functional): edge-DOF replacement parameterization via
  spher_eff_lambda; edge gradient α_opp⁺+α_opp⁻−θ_e (drops additive −(S⁺+S⁻)/2)
- Finding 4 (spherical_hessian): always-compiled edge-DOF throw guard
- Finding 6 (period_matrix): faithful normalizeModulus (0≤Re≤½, Im≥0, |τ|≥1)
- Finding 9 (inversive_distance): degenerate-face limiting angles, no skip
- Findings 1/2 (euclidean): degenerate gradient limiting angles + Hessian guard

Euclidean holonomy/τ fix: develop the cut surface across the dual spanning tree
only (cut_graph now exposes is_dual_tree), so genus-1 cut edges yield
non-degenerate lattice generators. Previously τ came out 0 / NaN / 1e13 on the
bundled tori; now matches the analytic revolution modulus i·√(R²−r²)/r. Re-enabled
τ reporting in the Euclidean CLI; rewrote validation.md §3/§4 accordingly.

Tests (240 CGAL, 0 skipped):
- HolonomyEndToEnd ×3 — tori of revolution (4×4, hex 6×6, 8×8) vs analytic modulus
- SphericalFunctional.EdgeGradient_RegularTetClosedForm — independent closed-form
  π/3 oracle locking the Finding-3 edge formula (the path-integral FD check cannot
  detect a wrong-but-conservative gradient)

Also documents the latent spherical/hyperbolic holonomy-extraction bug (same
single-development pattern, dead code today) in research-track.md (Phase 9c/10),
and adds favour/normalisations to the codespell ignore list.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2026-05-29 12:50:16 +02:00

245 lines
15 KiB
Markdown
Raw Blame History

This file contains invisible Unicode characters

This file contains invisible Unicode characters that are indistinguishable to humans but may be processed differently by a computer. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# Java → C++ Port Audit — Anomalies & Missing Test Cases
Systematic comparison of the C++ port (`code/include/*.hpp`) against the original
Java reference (`~/Desktop/conformallab`,
`de.varylab.discreteconformal.functional.*`). Focus: math correctness, logic, and
faithfulness to the reference.
Status legend: ✅ fixed · ⚠️ open / needs decision · note (no action)
---
## Files compared (math-critical)
| Java | C++ | Verdict |
|------|-----|---------|
| `Clausen.java` | `clausen.hpp` | ✅ faithful (incl. `inits` return, `clausen2`, `Л`, `ImLi2`) |
| `EuclideanCyclicFunctional.java` | `euclidean_functional.hpp`, `euclidean_geometry.hpp` | ✅ angles + gradient assembly match; degenerate handling fixed |
| — Hessian (`triangleHessian`/`conformalHessian`) | `euclidean_hessian.hpp` | cotangent + vertex block match; edge block **not implemented** (now guarded) |
| `SphericalFunctional.java` | `spherical_functional.hpp`, `spherical_geometry.hpp` | ✅ vertex-mode + edge-DOF replacement parameterization now match (Finding 3) |
| `HyperIdealFunctional.java` | `hyper_ideal_functional.hpp`, `hyper_ideal_geometry.hpp` | ✅ faithful (incl. degenerate (π,0,0) angles) |
| `HyperIdealUtility.java` | `hyper_ideal_utility.hpp` | ✅ ζ, ζ₁₃, ζ₁₄, ζ₁₅, both tetrahedron-volume formulas match |
| `CPEuclideanFunctional.java` | `cp_euclidean_functional.hpp` | ✅ `p()`, gradient, energy match |
| `DiscreteEllipticUtility.java` | `discrete_elliptic_utility.hpp`, `period_matrix.hpp` | ✅ `normalizeModulus` faithful and now wired into `compute_period_matrix` (Finding 6) |
| `HomologyUtility.java`, `CanonicalBasisUtility.java`, `SpanningTreeUtility.java` | `cut_graph.hpp` | tree-cotree only; no canonical/symplectic basis (Finding 7) |
| `EuclideanLayout.java` (holonomy) | `layout.hpp` (`detail::euclidean_holonomy`) | translation via midpoint displacement; ignores residual rotation (Finding 8) |
---
## Findings
### 1. ✅ FIXED — Degenerate triangle handling (Euclidean + Spherical gradient)
The Java reference assigns the **limiting** corner angles to a degenerate
(triangle-inequality-violating) face — the corner opposite the over-long edge is
**π**, the other two **0**. This is the convex C¹ extension that keeps the BPS
energy well-defined on the infeasible region, so Newton can pass through a flip
without stalling.
The C++ port returned `{0,0,0}` and **skipped the whole face** in the gradient.
Tell-tale: the hyper-ideal port *already* mirrors Java's (π,0,0) fallback —
euclidean/spherical were inconsistent oversights.
**Fix:** `euclidean_geometry.hpp` / `spherical_geometry.hpp` degenerate branches
now return the limiting angles (still `valid=false`, so the cotangent Hessian
keeps skipping — matching Java, whose `triangleHessian` zeroes cotangents on
degenerate faces). `euclidean_functional.hpp` / `spherical_functional.hpp`
gradients no longer skip. All 237 tests pass; solution unchanged (only affects
evaluations that previously contributed zero).
### 2. ✅ FIXED — `euclidean_hessian` missing edge-DOF guard
Doc comment claimed "the function asserts that no edge DOF is variable" — but
**no such guard existed**. With edge DOFs assigned
(`assign_euclidean_all_dof_indices`), the Java `conformalHessian`'s edge-edge and
vertex-edge blocks are absent, so the C++ would silently build a Hessian with
all-zero edge rows/cols (singular → LDLT fails → QR least-squares garbage).
**Fix:** added an always-compiled `throw std::logic_error` guard.
### 3. ✅ FIXED — Spherical edge-DOF parameterization now matches Java
Java's spherical functional **drops** the `u_i + u_j` vertex terms when an edge is
a variable (replacement parameterization, `SphericalFunctional.java:398400`),
whereas the C++ previously used additive `Λ = λ⁰ + u_i + u_j + λ_e` always. That
made the C++ spherical edge gradient carry an extra `(S_f⁺+S_f⁻)/2` term absent
from Java's `G.add(i, αk + αl π)` (`SphericalFunctional.java:283292`).
**Fix:** added helper `spher_eff_lambda` implementing the replacement convention
(`Λ_ij = λ_e` when the edge carries a DOF, else `λ⁰ + u_i + u_j`); `spherical_gradient`
Pass 1 now uses it, and the edge gradient is `α_opp⁺ + α_opp⁻ θ_e` (θ_e default
π), exactly matching Java. The energy is the path integral of this gradient, so it
follows automatically. The **vertex-only path is bit-for-bit unchanged** (no edge
DOFs → identical to before). All 237 tests pass.
### 4. ✅ FIXED — Spherical Hessian edge-DOF guard added
`spherical_hessian.hpp` builds only the vertex block; Java's spherical
`conformalHessian` includes edge terms. Same limitation as the Euclidean Hessian
but previously **without** the edge-DOF guard added in Finding 2.
**Fix:** added an always-compiled `throw std::logic_error` guard at the top of
`spherical_hessian` (mirrors Finding 2), so any attempt to use edge DOFs with the
spherical Hessian fails loudly instead of silently building a singular matrix.
All 237 tests pass.
### 5. NOTE — CP-Euclidean energy uses `clausen2`, Java uses `clausen`
`CPEuclideanFunctional.java:226` calls `Clausen.clausen` (the BORDERLINE-2.0944
polynomial); the C++ energy calls `clausen2` (the Chebyshev variant). Both compute
the same Cl₂ integral to ~machine precision, and the term appears only in the
energy (not the Newton-driving gradient). No functional impact. The C++ never
ported the `clausen()` variant at all — harmless.
### 6. ✅ FIXED — Period-matrix reduction now uses the faithful Java port
Two reductions coexist:
- `discrete_elliptic_utility.hpp::normalizeModulus` is a **faithful** line-by-line
port of Java `DiscreteEllipticUtility.normalizeModulus` — it folds τ into
**`0 ≤ Re(τ) ≤ ½`, `Im(τ) ≥ 0`, `|τ| ≥ 1`** (the extra `Re ≥ 0` fold uses the
mirror symmetry τ ≅ −τ̄).
- `period_matrix.hpp::reduce_to_fundamental_domain` reduces to the **standard**
SL(2,) domain **`−½ ≤ Re(τ) < ½`, `|τ| ≥ 1`** (no `Re ≥ 0` fold).
`compute_period_matrix` (the production path, and the one the `torus_8x8` τ test
exercises) calls **`reduce_to_fundamental_domain`**, so `normalizeModulus` is
**never used outside its own unit test** (verified by grep). Consequence: the C++
production τ can land with `Re(τ) < 0` where Java would report the mirrored
`+|Re(τ)|`. Same conformal type up to orientation, but **not equal to the Java
oracle value** — this will bite any golden-value cross-check (missing-test item 5).
**Fix (option a):** `compute_period_matrix` now calls `normalizeModulus`, so the
production τ matches the Java oracle exactly (folds into `0 ≤ Re ≤ ½`, `Im ≥ 0`,
`|τ| ≥ 1` via the mirror symmetry). `normalizeModulus` is no longer dead code. The
existing `torus_8x8` τ test still passes (`Re ∈ [0,½] ⊂ [−½,½]` so it satisfies
`is_in_fundamental_domain` too). `reduce_to_fundamental_domain` is retained for
callers that explicitly want the canonical (non-mirror-folded) SL(2,) domain.
### 7. NOTE — Cut graph is tree-cotree, not a canonical homology basis
`cut_graph.hpp` (`compute_cut_graph`) runs the EricksonWhittlesey tree-cotree
algorithm: primal BFS tree, dual BFS cotree, the remaining `2g` edges generate
H₁. Java's `CanonicalBasisUtility.getCanonicalHomologyBasis` goes further — it
builds the intersection form on the generators and solves for a **canonical
symplectic basis** `{a₁..a_g, b₁..b_g}` (and uses *weighted* shortest-path
cycles, not unweighted BFS).
- **Genus 1:** harmless — there is only one generator pair, and
`compute_period_matrix` reduces τ to the fundamental domain, so the basis choice
washes out. The common case matches.
- **Genus > 1:** the C++ `omega[]` ordering is arbitrary (edge-iteration order),
so `τ = ω₂/ω₁` is **not** the canonical Riemann period matrix. This is already
flagged in the `period_matrix.hpp` header ("Computing Ω from holonomy data
requires integration of holomorphic differentials — not implemented") so it is
consistent with the documented contract, not a regression.
### 8. NOTE — Holonomy translation assumes zero residual rotation
`detail::euclidean_holonomy` (`layout.hpp`) develops each face along a dual tree
that never crosses a cut edge, then reads each generator's translation as the
**midpoint displacement** `ω = midA midB` of the shared cut edge between its two
independent developments. This equals the true deck-translation **only when the
two developments differ by a pure translation** (linear part = identity), i.e. for
a perfectly flat cone metric (Θ ≡ 2π). When Newton has not fully converged (small
residual cone-angle defect), there is a residual rotation and the midpoint
displacement is a first-order approximation rather than the exact ω. A fully
robust version would fit the rigid motion mapping edge `(Bs,Bt)↦(At,As)` and read
its translation part. Acceptable for converged flat tori (the `torus_8x8` test
passes); worth tightening if higher-genus or under-converged inputs are used.
---
## Findings in non-ported (conformallab++-only) math modules
These modules have **no Java reference** — they were ported from the literature
(Luo 2004, Glickenstein 2011, BowersStephenson 2004) or are original additions.
They were audited against the cited formulas / first principles rather than Java.
### 9. ✅ FIXED — Inversive-distance gradient now uses limiting angles (consistent with Finding 1)
`inversive_distance_functional.hpp::inversive_distance_gradient` previously detected
a triangle-inequality-violating face two ways and **skipped it** in both:
- `if (l12sq <= 0 || l23sq <= 0 || l31sq <= 0) continue;` (non-real circle config)
- `if (!fa.valid) continue;` (degenerate-but-real triangle)
The second skip was the *same* situation Finding 1 fixed for Euclidean/Spherical:
`euclidean_angles` returns the **limiting angles** (π opposite the over-long edge,
0/0 for the others) with `valid=false`, but this code threw them away.
**Fix:** removed the `if (!fa.valid) continue;` skip so the limiting angles flow
into the gradient (the BPS-style convex C¹ extension), mirroring Finding 1; added a
comment explaining the rationale. The first skip (`l*sq <= 0`, a genuinely non-real
circle configuration with no limiting angle) is **kept**. All 237 tests pass.
### 10. NOTE — Inversive-distance edge length has no overflow centring
`id_detail::edge_length_squared` computes `ℓ² = rᵢ² + rⱼ² + 2·I·rᵢ·rⱼ` with
`r = exp(u)` directly — no log-centring like `euclidean_angles`'s `μ`. The *angle*
computation is still safe (it re-centres internally on `log ℓ²`), but the raw `ℓ²`
can overflow for extreme `|u|`. Cosmetic robustness only; not hit in practice.
### 11. NOTE — Verified correct (no action)
Audited against the literature and found faithful:
- **`trilaterate_2d / _sph / _hyp`** (`layout.hpp`) — circle-circle intersection
(ℝ²), spherical law of cosines via `α·pa+β·pb+γ·(pa×pb)`, and hyperbolic law of
cosines + Poincaré-disk Möbius placement. All three exact; left/CCW side correct.
- **`newton_solver.hpp`** — merit `f = ½‖G‖²`; Newton dir is always a descent dir
(`∇f·dx = ‖G‖²`), Armijo tests algebraically correct in both phases; spherical
solver factorises `H` (PSD) and `d_sd = H·G = ∇f` uses the un-negated H — all
consistent. SparseQR fallback handles the gauge null space (valid because
GaussBonnet makes `G ⟂ 𝟙`).
- **`gauss_bonnet.hpp`** — `χ=VE+F`, `g=(2χ)/2`, `enforce` shift
`δ=(lhsrhs)/V` makes the new sum equal `2π·χ` exactly.
- **Normalisations** — Euclidean PCA, spherical Rodrigues (mean→north),
hyperbolic weighted Fréchet-mean Möbius centring: all correct (spherical has a
benign antipodal-mean edge case that no-ops).
---
## Missing test cases (gaps observed during the audit)
1. **Degenerate / flipped-triangle gradient** — no test drives a face through the
triangle-inequality boundary to lock in Finding 1. Add: build a triangle with
one edge length > sum of the others, assert the gradient picks up the π corner
(not 0). Cover both Euclidean and Spherical.
2. **`euclidean_hessian` edge-DOF guard** — no test asserts the new `throw` fires
when `assign_euclidean_all_dof_indices` is used. Add an `EXPECT_THROW`.
3.**DONE (2026-05-29)** — Holonomy / period matrix end-to-end now covers
three tori of revolution (`HolonomyEndToEnd.{Torus4x4,TorusHex6x6,Torus8x8}`),
each asserting τ against the analytic modulus `i·√(R²r²)/r` and `Re(τ) ≈ 0`.
(This also fixed the Euclidean holonomy extraction — develop across the dual
tree only — see `layout.hpp` / `cut_graph.hpp`.)
4. ⚠️ **PARTLY DONE (2026-05-29)** — added `EdgeGradient_RegularTetClosedForm`:
an *independent closed-form* oracle that pins each edge-DOF gradient to
`π/3` (= 2·(2π/3) π) on the regular spherical tetrahedron, where the corner
angle 2π/3 follows from the spherical law of cosines. This locks the
Finding-3 formula `α_opp⁺ + α_opp⁻ θ_e` against a value the path-integral FD
check cannot detect (the energy is the integral of G, so FD only proves
curl-freeness). **Still open:** (i) Newton-to-convergence *with* edge DOFs is
blocked by the Finding-4 spherical-Hessian guard (`throw` on edge DOFs), so a
converged-metric test needs an FD-Hessian or guard relaxation first; (ii) a
live-Java golden-value oracle still requires running the upstream library.
5. **Cross-check vs Java numeric oracles** — there is no test that pins C++
functional/gradient values against recorded Java outputs on a fixed mesh. A
handful of golden-value tests (energy + gradient at a known `x`) would catch
any future silent divergence from the reference far more directly than the
self-consistent FD checks (which only verify curl-freeness, since the C++
energy is itself the path-integral of its own gradient).
6. **CP-Euclidean `clausen` vs `clausen2`** — a single test asserting
`|clausen(x) clausen2(x)| < 1e-12` across a sweep would document Finding 5
and guard against an approximation regression.
7. **Period-matrix domain convention (Finding 6)** — production now uses
`normalizeModulus` (folds `Re ≥ 0`). Add a torus whose true τ has `Re(τ) < 0`
before reduction and assert the resulting `Re(τ) ≥ 0` convention, so any future
switch back to `reduce_to_fundamental_domain` is caught. Pair with a golden τ
recorded from the Java `DiscreteEllipticUtility` on the same mesh.
8. **`normalizeModulus` vs Java oracle (Finding 6)** — the faithful
`normalizeModulus` is only checked against ad-hoc values. Add a few recorded
Java `normalizeModulus` outputs as golden values to lock the `Re ≥ 0` fold.
9. **Higher-genus cut graph (Finding 7)** — only genus-1 is exercised end-to-end.
Add a genus-2 mesh and assert `compute_cut_graph` returns exactly `2g = 4` cut
edges and `genus == 2`, documenting that the basis is *not* canonical (so no τ
correctness is claimed there).
10. **Inversive-distance degenerate face (Finding 9)** — no test drives an
inversive-distance triangle through the triangle-inequality boundary. Now that
Finding 9 mirrors Finding 1 (limiting angles used, not skipped), add the
analogue of missing-test item 1: assert the corner opposite the over-long edge
is picked up as π rather than silently skipped.