Files
ConformalLabpp/doc/reviewer/java-port-audit.md
Tarik Moussa ba83974525
All checks were successful
C++ Tests / test-fast (pull_request) Successful in 1m57s
API Docs / doc-build (pull_request) Successful in 59s
Markdown link check / check (pull_request) Successful in 51s
C++ Tests / test-cgal (pull_request) Has been skipped
C++ Tests / quality-gates (pull_request) Successful in 2m19s
test: Java golden-value oracles for the five DCE math cores + P1-2/P1-3 fixes
Add bit-for-bit (1e-12) golden-value oracle tests pinning the C++ pure-math
and functional cores against the compiled upstream Java library (openjdk 17):

- HyperIdealGoldenJava: Clausen/Л/ImLi2, ζ13/14/15/ζ, both tetrahedron-volume
  formulas (real de.varylab…Clausen / HyperIdealUtility).
- EuclideanGoldenJava / SphericalGoldenJava: angle formulas + β relations + Л
  energy terms, plus FULL-MESH oracles driving the real EuclideanCyclicFunctional
  / SphericalFunctional on a shared tetrahedron — per-vertex gradient (Θ−Σα) and
  ΔE = E(x)−E(0) (C++ Gauss-Legendre path integral vs Java closed form).
- SphericalGoldenJava.FullMeshEdgeDofGradient: edge-DOF gradient (vertex + edge
  components, α_opp⁺+α_opp⁻−θ_e) vs raw conformalEnergyAndGradient — locks
  Finding 3 at the solution level (audit items 4 & 5).
- PeriodMatrix.NormalizeModulus_GoldenJava: τ-reduction fold convention vs the
  real DiscreteEllipticUtility.normalizeModulus (audit items 7 & 8).

Subtlety documented: the spherical oracles call Java's raw
conformalEnergyAndGradient, not evaluate() (which pre-runs a Brent gauge
maximization that C++ factors into the Newton solver's spherical_gauge_shift).

Also:
- P1-2 (layout.hpp): Euclidean holonomy now uses a per-cut-edge rigid-motion fit
  g(z)=a·z+b, exposing residual_rotation = |arg(a)| as a diagnostic; non-
  regressive (flat case a=1 reduces to the old midpoint formula).
- P1-3 (period_matrix.hpp): is_in_fundamental_domain fixed to the correct
  half-open SL(2,ℤ) domain (−½ ≤ Re < ½). Updated the now-exposed
  ComputePeriodMatrix_ReducedTau_InFD to assert the normalizeModulus domain
  (closed +½ edge) instead.

Test counts (single source of truth = doc/api/tests.md): 272/272 pass, 0
skipped (26 non-CGAL + 246 CGAL).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-05-29 19:08:37 +02:00

291 lines
18 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. ✅ FIXED — Holonomy now fits the full deck isometry (was: midpoint only)
`detail::euclidean_holonomy` (`layout.hpp`) develops each face along a dual tree
that never crosses a cut edge. It **previously** read each generator's
translation as the **midpoint displacement** `ω = midA midB` of the shared cut
edge between its two independent developments — exact **only** when the two
developments differ by a pure translation (linear part = identity), i.e. a
perfectly flat cone metric (Θ ≡ 2π). Under residual cone-angle defect that was
a first-order approximation.
**Fix (2026-05-29):** it now fits the full orientation-preserving deck isometry
`g(z) = a·z + b` to the correspondence `(Bs,Bt) ↦ (As,At)` (`a = (AtAs)/(BtBs)`,
`b = As a·Bs`). Because both faces use the same edge length, `|a| = 1` exactly,
so the fit is an exact isometry; `ω = b` is its translation part and `|arg(a)|`
is exposed per cut edge as `HolonomyData::residual_rotation` (a convergence
diagnostic). For a flat cone metric `a = 1` and `b` reduces to `midA midB`, so
converged flat tori are **bit-for-bit unchanged** — all 240 CGAL tests still pass,
including `HolonomyEndToEnd.{Torus4x4,TorusHex6x6,Torus8x8}`. Note: when
`|arg(a)|` is non-negligible the holonomy is a genuine rotation and *no* lattice
translation is well-defined; the diagnostic now surfaces that instead of silently
averaging.
---
## 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.**DONE (2026-05-29)** — the Finding-3 spherical edge-DOF gradient is now
locked at two levels: (a) `EdgeGradient_RegularTetClosedForm` — an independent
*closed-form* oracle pinning each edge-DOF gradient to `π/3` (= 2·(2π/3) π)
on the regular spherical tetrahedron; and (b)
`SphericalGoldenJava.FullMeshEdgeDofGradient_Tetrahedron` — a *live-Java*
full-mesh oracle (item 5) on a generic asymmetric configuration that pins
BOTH the vertex gradient AND the edge gradient `α_opp⁺ + α_opp⁻ θ_e` to
1e-12 vs the real raw `conformalEnergyAndGradient`. The vertex-DOF spherical
path is likewise locked by `SphericalGoldenJava.FullMeshGradientAndEnergy_…`.
**Still open (solver feature, not a correctness gap):** 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.
5.**DONE (2026-05-29)** — golden-value oracles now span both levels:
*pure-math / per-triangle* (1e-12 vs compiled upstream, openjdk 17):
`HyperIdealGoldenJava` (Clausen/Л/ImLi₂, ζ₁₃/₁₄/₁₅/ζ, both tetrahedron-volume
formulas), `EuclideanGoldenJava` (angle formula + 2·Л energy),
`SphericalGoldenJava` (law-of-cosines angles + β relations + Л energy),
`PeriodMatrix.NormalizeModulus_GoldenJava` (item 8).
*full-mesh energy + gradient at a known `x`* — the originally-fragile part,
now solved via a shared OBJ tetrahedron loaded identically in both languages:
`EuclideanGoldenJava.FullMeshGradientAndEnergy_Tetrahedron` and
`SphericalGoldenJava.FullMeshGradientAndEnergy_Tetrahedron` drive the REAL
`EuclideanCyclicFunctional` / `SphericalFunctional` and pin every per-vertex
gradient `Θ−Σα` (matched by vertex position) AND `ΔE = E(x)E(0)` to 1e-12.
The ΔE check is doubly independent: C++ computes energy as a Gauss-Legendre
PATH INTEGRAL of its gradient, Java as a CLOSED-FORM sum — the initialEnergy
constant and the φ·λ⁰ term cancel in the difference (no edge DOFs here).
*full-mesh EDGE-DOF gradient*
`SphericalGoldenJava.FullMeshEdgeDofGradient_Tetrahedron` makes two opposite
tetrahedron edges variable (replacement parameterization λ_e = x[e_idx]) and
pins BOTH the vertex gradient AND the edge gradient `α_opp⁺+α_opp⁻θ_e` to
1e-12 vs the real raw `conformalEnergyAndGradient`, closing the Finding-3
solution-level gap (item 4). It is a pure gradient oracle (no ΔE): with an
edge DOF, x=0 ⇒ λ_e=0 ⇒ arc length π is degenerate, so the path-integral-from-
origin energy reference is ill-defined; the fixed-x gradient is the
unambiguous quantity. It never touches the Hessian, so the Finding-4 edge-DOF
Hessian guard does not block it. Harness: `/tmp/oracle/SphereEdgeOracle.java`.
**Subtlety surfaced:** the spherical oracle must call Java's raw
`conformalEnergyAndGradient`, not `evaluate()`; `evaluate()` first runs a 1-D
Brent maximization over the global-scale gauge (`maximizeInNegativeDirection`),
which the C++ `spherical_gradient` deliberately omits (C++ factors that gauge
into the Newton solver's `spherical_gauge_shift`). Harnesses:
`/tmp/oracle/{tet.obj,EucMeshOracle.java,SphereMeshOracle.java,SphereEdgeOracle.java}`.
The *edge-DOF* full-mesh gradient oracle is now also done (item 4,
`SphereEdgeOracle.java`).
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. ⚠️ **PARTLY DONE (2026-05-29)** — the `Re ≥ 0` fold convention is now locked at
the function level by item 8 (`NormalizeModulus_GoldenJava`, including `Re < 0`
inputs). **Still open:** an *end-to-end* torus whose true τ has `Re(τ) < 0`
before reduction, asserting the production pipeline yields `Re(τ) ≥ 0` (so any
future switch back to `reduce_to_fundamental_domain` is caught at the mesh
level, not just the function level).
8.**DONE (2026-05-29)**`PeriodMatrix.NormalizeModulus_GoldenJava` pins the
C++ `normalizeModulus` against five recorded outputs of the real Java
`DiscreteEllipticUtility.normalizeModulus` (1e-12), including `Re < 0` mirror
folds, large-`Re` `T`-steps, and `|τ| < 1` `S`-inversions — locking the
`0 ≤ Re ≤ ½` fold convention. (Generated via `/tmp/oracle/TauOracle.java`.)
This also surfaced a real interaction: `compute_period_matrix` can land
exactly on `Re = +½` (the closed right edge of `normalizeModulus`'s mirror-
folded domain), which the half-open SL(2,) `is_in_fundamental_domain`
(`−½ ≤ Re < ½`, P1-3) correctly *excludes*; `ComputePeriodMatrix_ReducedTau_InFD`
was updated to assert the `normalizeModulus` domain rather than the SL(2,) one.
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.