diff --git a/doc/reviewer/api-performance-audit-2026-05-31.md b/doc/reviewer/api-performance-audit-2026-05-31.md new file mode 100644 index 0000000..b788d37 --- /dev/null +++ b/doc/reviewer/api-performance-audit-2026-05-31.md @@ -0,0 +1,420 @@ +# API-Consistency & Performance Audit β€” ConformalLabpp + +**Date:** 2026-05-31 +**Auditor:** External reviewer (Claude Opus 4.8) +**Scope:** Low-level API (`code/include/*.hpp`), high-level CGAL API (`code/include/CGAL/`), Newton-solver performance +**Focus:** Public-API naming consistency and runtime performance β€” NOT port-faithfulness or test coverage. + +This document is self-contained. A new session can pick up any finding below and +act on it without prior context. Each finding includes: +- exact file path + line numbers (verified by direct file read) +- a minimal reproduction of the problematic code +- the recommended fix +- acceptance criteria for "done" + +Status legend: πŸ”΄ Critical Β· 🟑 Important Β· πŸ”΅ Hint / nice-to-have + +> **Companion documents (no overlap in findings):** +> - `external-audit-2026-05-30.md` β€” port-faithfulness bugs +> - `test-coverage-error-handling-audit-2026-05-31.md` β€” test gaps + error handling +> - **this file** β€” API consistency + performance +> +> Cross-reference: performance findings B2/B3/B5 below share root cause with +> `FINDING-H2` ("five near-identical Newton loops") in the test-coverage audit. +> A single `newton_core` refactor can resolve all of them together. + +--- + +## How to read this document in a new session + +```bash +# Build the CGAL suite (the API + solvers live behind WITH_CGAL_TESTS) +cmake -S code -B build-cgal -DWITH_CGAL_TESTS=ON +cmake --build build-cgal --target conformallab_cgal_tests -j$(nproc) + +# Baseline before any change +ctest --test-dir build-cgal -R '^cgal\.' --output-on-failure +# Expected: 246 passed, 0 failed +``` + +For performance findings, the smoke meshes used by the project are: +- `cathead.obj` (open, Fβ‰ˆ248) +- `brezel.obj` (genus-1, Fβ‰ˆ13824) +- `brezel2.obj` (genus-2) +exercised by `code/tests/cgal/test_scalability_smoke.cpp`. + +--- + +## Summary table + +| ID | Sev | Title | Primary location | +|----|-----|-------|------------------| +| B1 | πŸ”΄ | `newton_hyper_ideal` uses full-FD Hessian; the 96–1166Γ— faster block-FD exists but is unused. Inversive-Distance has no fast path at all | `newton_solver.hpp:433`, `:580` | +| A1 | πŸ”΄ | `assign_*_dof_indices`: three different naming conventions across the 5 functionals | 5 functional headers | +| B2 | 🟑 | `line_search` discards the gradient at the accepted point β†’ ~1 redundant full gradient eval per Newton iteration (Γ—5 solvers) | `newton_solver.hpp` | +| B3 | 🟑 | `has_edge_dof` O(E) scan runs every Newton iteration (layout is loop-invariant) | `newton_solver.hpp:264` | +| A4 | 🟑 | High-level CGAL entry points have inconsistent suffix (`_map` only on one) | `code/include/CGAL/*.h` | +| A5 | 🟑 | The two circle-packing models return different result types | `Discrete_circle_packing.h`, `Discrete_inversive_distance.h` | +| A2 | 🟑 | `compute_*_from_mesh`: inconsistent prefix + verb (`lambda0` vs `init`) | 3 functional headers | +| A3 | 🟑 | `gradient_check` (HyperIdeal) lacks the `_` suffix the other 4 have | `hyper_ideal_functional.hpp:470` | +| B4 | 🟑 | SimplicialLDLT symbolic factorization rebuilt every iteration (sparsity pattern is constant) | `newton_solver.hpp:73` | +| B5 | πŸ”΅ | Dead `SimplicialLDLT solver;` variable + per-call gradient heap allocation | `newton_solver.hpp:244`, `euclidean_functional.hpp:193` | + +--- + +# Part A β€” API consistency + +> Context: the project has two API layers. The **high-level CGAL layer** +> (`CGAL::discrete_*` + named parameters) intentionally follows CGAL conventions β€” +> that is correct and not a finding by itself. The **low-level free-function layer** +> (`setup_*`, `assign_*`, `*_gradient`) is the one used directly by tests and +> internal callers, and it is internally inconsistent. `setup__maps` IS +> uniform across all five functionals, which proves the convention is achievable β€” +> it simply was not held for the other verbs. + +## FINDING-A1 β€” πŸ”΄ `assign_*_dof_indices`: three conventions for one operation + +### Location +- `euclidean_functional.hpp:107,132` +- `spherical_functional.hpp:99,123` +- `hyper_ideal_functional.hpp:107` +- `cp_euclidean_functional.hpp:140` +- `inversive_distance_functional.hpp:133` + +### Problem +| Functional | Vertex variant | All variant | +|---|---|---| +| Euclidean | `assign_euclidean_vertex_dof_indices` | `assign_euclidean_all_dof_indices` | +| Spherical | `assign_vertex_dof_indices` ⚠️ no prefix | `assign_all_spherical_dof_indices` ⚠️ infix | +| HyperIdeal | β€” | `assign_all_dof_indices` ⚠️ no prefix | +| CP-Euclidean | `assign_cp_euclidean_face_dof_indices` | β€” | +| InversiveDist | `assign_inversive_distance_vertex_dof_indices` | β€” | + +Three patterns: `assign___dof_indices`, `assign__dof_indices` +(no geometry), and `assign_all__dof_indices` (geometry in the **middle**). +The prefix-less `assign_vertex_dof_indices` (spherical) and `assign_all_dof_indices` +(hyper-ideal) read as generic but are geometry-specific, which is misleading at +the call site (e.g. `test_newton_solver.cpp:81` calls `assign_vertex_dof_indices`). + +### Fix β€” DECIDED (2026-05-31) +Standardize on `assign___dof_indices` (matches the already-consistent +`setup__maps`). Confirmed renames: + +| Old | New | +|---|---| +| `assign_vertex_dof_indices` (spherical) | `assign_spherical_vertex_dof_indices` | +| `assign_all_spherical_dof_indices` | `assign_spherical_all_dof_indices` | +| `assign_all_dof_indices` (hyper-ideal) | `assign_hyper_ideal_all_dof_indices` | +| `assign_euclidean_vertex_dof_indices` | unchanged βœ“ | +| `assign_euclidean_all_dof_indices` | unchanged βœ“ | +| `assign_cp_euclidean_face_dof_indices` | unchanged βœ“ | +| `assign_inversive_distance_vertex_dof_indices` | unchanged βœ“ | + +Keep the old names as `[[deprecated]]` inline wrappers for one release so existing +call sites (tests, examples) keep compiling, then update all call sites. + +### Acceptance criteria +- All five functionals follow `assign___dof_indices`. +- Old names exist as `[[deprecated]]` aliases (or all call sites updated in the same PR). +- Full CGAL suite still 246/246. + +--- + +## FINDING-A2 β€” 🟑 `compute_*_from_mesh`: inconsistent prefix and verb + +### Location +- `euclidean_functional.hpp:152` β€” `compute_euclidean_lambda0_from_mesh` +- `spherical_functional.hpp:148` β€” `compute_lambda0_from_mesh` ⚠️ no prefix +- `inversive_distance_functional.hpp:187` β€” `compute_inversive_distance_init_from_mesh` ⚠️ `init` not `lambda0` + +### Problem +`compute_lambda0_from_mesh` (spherical) has no geometry prefix and is used directly +in `test_newton_solver.cpp:80,99`. Inversive-Distance uses the verb `init` instead +of `lambda0` for the conceptually parallel step. + +### Fix β€” DECIDED (2026-05-31) +- `compute_lambda0_from_mesh` β†’ `compute_spherical_lambda0_from_mesh` (add prefix). +- `compute_inversive_distance_init_from_mesh` β†’ **keep `init`**. Rationale: this step + initializes `I_e` and `r0`, i.e. genuinely more than just Ξ»β‚€, so `init` is the + honest verb. The distinction (`lambda0` vs `init`) will be documented in + `doc/api/extending.md`. +- `compute_euclidean_lambda0_from_mesh` β†’ unchanged βœ“. +- `[[deprecated]]` alias for the spherical rename. + +### Acceptance criteria +- Spherical compute fn carries the `spherical` prefix. +- Naming rationale for `init` vs `lambda0` documented in `doc/api/extending.md`. + +--- + +## FINDING-A3 β€” 🟑 `gradient_check` (HyperIdeal) breaks the `_` suffix pattern + +### Location +- `euclidean_functional.hpp:324` β€” `gradient_check_euclidean` +- `spherical_functional.hpp:367` β€” `gradient_check_spherical` +- `cp_euclidean_functional.hpp:331` β€” `gradient_check_cp_euclidean` +- `inversive_distance_functional.hpp:346` β€” `gradient_check_inversive_distance` +- `hyper_ideal_functional.hpp:470` β€” `gradient_check` ⚠️ no suffix + +### Fix β€” DECIDED (2026-05-31) +Rename `gradient_check` β†’ `gradient_check_hyper_ideal` with a `[[deprecated]]` alias. +The other four (`gradient_check_euclidean/_spherical/_cp_euclidean/_inversive_distance`) +are unchanged βœ“. + +### Acceptance criteria +- All five follow `gradient_check_`. + +--- + +## FINDING-A4 β€” 🟑 High-level CGAL entry points: inconsistent suffix + +### Location +`code/include/CGAL/Discrete_conformal_map.h`, `Discrete_circle_packing.h`, +`Discrete_inversive_distance.h` + +### Problem +```cpp +CGAL::discrete_conformal_map_euclidean // no trailing word +CGAL::discrete_conformal_map_spherical +CGAL::discrete_conformal_map_hyper_ideal +CGAL::discrete_circle_packing_euclidean // no _map +CGAL::discrete_inversive_distance_map // ⚠️ trailing _map +``` +`discrete_inversive_distance_map` ends in `_map`; `discrete_circle_packing_euclidean` +does not β€” yet both are circle-packing models. + +### Fix β€” DECIDED (2026-05-31) +`discrete_inversive_distance_map` β†’ **`discrete_inversive_distance_euclidean`** (drop +the trailing `_map`, add the geometry word), so it matches +`discrete_circle_packing_euclidean`. Because this is the **public CGAL API**, treat +it as a breaking change: add the new name, mark the old `[[deprecated]]`, announce in +`CHANGELOG.md`. + +> ⚠️ Sequencing: A4 touches the **public CGAL surface**. Do this rename only after the +> G0/G1 license/provenance outcome is known (see CGAL audit) β€” there is no point +> stabilizing a public API on a package whose release status is unresolved. A1–A3 are +> internal and can proceed now. + +### Acceptance criteria +- The two circle-packing entry points share a naming shape + (`discrete_*_euclidean`). +- CHANGELOG documents the rename; deprecated alias provided. + +--- + +## FINDING-A5 β€” 🟑 The two circle-packing models return different result types + +### Location +- `Discrete_circle_packing.h:100` β†’ returns `Circle_packing_result` +- `Discrete_inversive_distance.h:133,146` β†’ returns `Conformal_map_result` + +### Problem +CP-Euclidean and Inversive-Distance are both circle-packing models, but +`discrete_circle_packing_euclidean` returns `Circle_packing_result` while +`discrete_inversive_distance_map` returns `Conformal_map_result`. A user switching +between the two packers must rewrite their result-handling code, even though the +semantics (per-element radii + Newton diagnostics) are the same. + +### Fix β€” DECIDED (2026-05-31) +**Option (a):** both circle-packing entry points return `Circle_packing_result`. +`discrete_inversive_distance_*` switches from `Conformal_map_result` to +`Circle_packing_result` (semantically truthful β€” both produce circle radii). The +unified-result option (b) is rejected as over-engineered. + +> ⚠️ Same sequencing as A4 β€” public-API change, do after the G0/G1 outcome. + +### Acceptance criteria +- The two packing entry points return `Circle_packing_result`. +- If a field is renamed (e.g. `u_per_vertex` β†’ `radius_per_*`), document it in CHANGELOG. + +--- + +# Part B β€” Performance + +## FINDING-B1 β€” πŸ”΄ Fast Hessian paths exist but the solvers don't use them + +### Location +- `newton_solver.hpp:433` β€” `newton_hyper_ideal` calls `hyper_ideal_hessian_sym(...)` + (the **full-FD** O(nΒ·F) version) +- `hyper_ideal_hessian.hpp:220` β€” `hyper_ideal_hessian_block_fd_sym(...)` (the fast + block-FD version) **exists and is tested** but is never called by the solver +- `newton_solver.hpp:580-608` β€” `newton_inversive_distance` builds a **full-FD** + Hessian inline (2 gradient evals per DOF), with no fast path at all + +### Problem +**HyperIdeal:** the solver uses the slow full-FD Hessian even though a block-FD +version with a documented **33Γ— speedup on cathead, ~1166Γ— on brezel** already +exists (`hyper_ideal_hessian.hpp:130-136`). The fast code is implemented, tested +(`test_hyper_ideal_hessian.cpp`), and simply not wired into the solver. + +```cpp +// newton_solver.hpp:433 β€” current (slow): +auto H = hyper_ideal_hessian_sym(mesh, x, m, hess_eps); // full-FD, O(nΒ·F) +// available (fast, same result up to FD noise): +auto H = hyper_ideal_hessian_block_fd_sym(mesh, x, m, hess_eps); // block-FD +``` + +**Inversive-Distance:** the inline `build_hessian` lambda runs `2n` full gradient +evaluations per Hessian (each O(F)), i.e. **O(nΒ·F) per Newton iteration** β€” quadratic +in mesh size. No block-FD or analytic path exists yet. + +```cpp +// newton_solver.hpp:585-601 β€” current: +for (int j = 0; j < n; ++j) { + auto Gp = inversive_distance_gradient(mesh, xp, m); // O(F) + auto Gm = inversive_distance_gradient(mesh, xm, m); // O(F) β†’ 2n total per Hessian + ... +} +``` + +### Fix +1. **HyperIdeal (cheap win):** switch line 433 to `hyper_ideal_hessian_block_fd_sym`. + Verify the existing convergence tests (`test_newton_solver.cpp` HyperIdeal cases, + `test_newton_phase9a.cpp`) still pass within tolerance β€” block-FD is mathematically + equivalent up to FD rounding (locality lemma, documented at `hyper_ideal_hessian.hpp:120-128`). +2. **Inversive-Distance:** port a block-FD Hessian mirroring the HyperIdeal one + (the gradient is also face-decomposable). Track the analytic Glickenstein-2011 + Hessian separately as already noted in `doc/roadmap/research-track.md`. + +### Acceptance criteria +- `newton_hyper_ideal` uses block-FD; HyperIdeal convergence tests still pass. +- A timing assertion or smoke test demonstrates the speedup on `brezel.obj` + (or at minimum, the change is benchmarked and recorded in `doc/architecture/`). +- Inversive-Distance has a block-FD path; `test_newton_phase9a.cpp` still passes. + +--- + +## FINDING-B2 β€” 🟑 line_search discards the gradient at the accepted point + +### Location +`newton_solver.hpp` β€” `detail::line_search` (lines 140-201) and all 5 solver loops +(e.g. Euclidean lines 248, 277-280, 287) + +### Problem +Per Newton iteration the gradient over the whole mesh is computed redundantly: +1. line 248 β€” gradient for the convergence check +2. inside `line_search` β€” gradient at each trial point; the **vector is discarded**, + only its norm is kept +3. next iteration's line 248 β€” recomputes the gradient at the just-accepted point + (already computed in step 2) +4. line 287 (`G_final`) β€” recomputes once more after the loop + +That is roughly **one redundant full-mesh gradient per iteration** plus one at the end. + +### Fix +Have `line_search` return (via out-param) the gradient vector at the accepted point, +and feed it into the next iteration's convergence check instead of recomputing. The +end-of-loop `G_final` can reuse the last accepted gradient too. + +> Best done as part of the `newton_core` template refactor (test-coverage audit +> FINDING-H2) so the fix lands once instead of five times. + +### Acceptance criteria +- Gradient evaluations per iteration drop by ~1 (instrument with a counter in a test). +- All convergence tests pass unchanged. + +--- + +## FINDING-B3 β€” 🟑 has_edge_dof scan runs every Newton iteration + +### Location +`newton_solver.hpp:264-268` (inside the `newton_euclidean` iteration loop) + +### Problem +```cpp +bool has_edge_dof = false; +for (auto e : mesh.edges()) // O(E), runs EVERY iteration + if (m.e_idx[e] >= 0) { has_edge_dof = true; break; } +``` +The DOF layout (`m.e_idx`) does not change during the solve, so this scan is +loop-invariant. Over up to 200 iterations the mesh edge set is scanned 200Γ— for no +reason. + +### Fix +Hoist the scan above the `for (iter...)` loop; compute `has_edge_dof` once. + +### Acceptance criteria +- `has_edge_dof` computed exactly once per `newton_euclidean` call. +- Euclidean tests pass unchanged. + +--- + +## FINDING-B4 β€” 🟑 Symbolic factorization rebuilt every iteration + +### Location +`newton_solver.hpp:73` (`detail::solve_with_fallback`) + +### Problem +```cpp +Eigen::SimplicialLDLT> ldlt(A); // analyze + factorize +``` +A fresh `SimplicialLDLT` is constructed each iteration, which re-runs the symbolic +analysis (fill-reducing reordering / COLAMD). But the **sparsity pattern of the +Hessian is constant across iterations** (only the numeric values change). Eigen +supports `analyzePattern()` once + `factorize()` per iteration, skipping the +repeated symbolic step. + +### Fix +This needs a structural change: `solve_with_fallback` is stateless, so it cannot +cache the analysis. Either: +- thread a persistent solver object through the Newton loop (cleanest with the + `newton_core` refactor), calling `analyzePattern` on the first iteration and + `factorize` thereafter, or +- key a cached symbolic factorization on the matrix sparsity pattern. + +Keep the SparseQR fallback for the singular-pattern case (the pattern there is +also constant, so the same caching applies). + +### Acceptance criteria +- Symbolic analysis runs once per solve, not once per iteration. +- All Newton + fallback tests (incl. the 3 SparseQR tests) pass. +- Benchmarked on `brezel.obj`; improvement recorded. + +--- + +## FINDING-B5 β€” πŸ”΅ Dead variable + per-call gradient allocation + +### Location +- `newton_solver.hpp:244` β€” `Eigen::SimplicialLDLT<...> solver;` declared in + `newton_euclidean`, **never used** (the solve goes through `solve_with_fallback`) +- `euclidean_functional.hpp:193,197` β€” each `euclidean_gradient` call freshly + allocates `std::vector G(n)` and `std::vector h_alpha(nh)` + +### Problem +- The dead `solver` variable is harmless but misleading (suggests reuse that + doesn't happen β€” directly relevant once B4 is addressed). +- In the FD Hessian (B1) the per-call allocation means `2n Γ— 2` fresh heap + allocations per Hessian build. + +### Fix +- Remove the dead `solver` declaration. +- When addressing B1/B2, pass reusable scratch buffers (`G`, `h_alpha`) into the + gradient functions, or provide an overload that writes into a caller-owned buffer. + +### Acceptance criteria +- Dead variable removed (compiles clean with `-Wunused`). +- (Optional, with B1/B2) gradient functions support buffer reuse; allocation count + in the FD Hessian path drops measurably. + +--- + +## Suggested order of work + +1. **B1 (HyperIdeal half)** β€” one-line switch to `hyper_ideal_hessian_block_fd_sym`; + biggest win for the smallest change. Verify tests, benchmark. +2. **B3** β€” trivial loop hoist. +3. **A1, A2, A3** β€” low-level renames with `[[deprecated]]` aliases; do together. +4. **A4, A5** β€” high-level CGAL API politur; coordinate with CHANGELOG (breaking). +5. **newton_core refactor** (test-coverage audit FINDING-H2) β€” then fold in **B2, + B4, B5** in the same pass. +6. **B1 (Inversive-Distance half)** β€” port a block-FD Hessian; larger effort. + +## What is already good (do not "fix") + +- `setup__maps` is uniform across all five functionals β€” the consistency + target is proven achievable. +- The block-FD HyperIdeal Hessian is correct, tested, and well-documented β€” it + just needs to be wired into the solver (B1). +- The high-level CGAL API correctly uses CGAL named parameters and `Kernel_traits` β€” + idiomatic for a CGAL package. +- `Conformal_map_result` carries `sparse_qr_fallback_used` β€” good observability. diff --git a/doc/reviewer/cgal-submission-readiness-audit-2026-05-31.md b/doc/reviewer/cgal-submission-readiness-audit-2026-05-31.md new file mode 100644 index 0000000..f011aa7 --- /dev/null +++ b/doc/reviewer/cgal-submission-readiness-audit-2026-05-31.md @@ -0,0 +1,473 @@ +# CGAL Submission-Readiness Audit β€” ConformalLabpp + +**Date:** 2026-05-31 +**Auditor:** External reviewer (Claude Opus 4.8) +**Scope:** Readiness of the `code/include/CGAL/` package for submission to the +CGAL project (editorial-board review + integration into the CGAL release). +**Reference:** CGAL Developer Manual β€” "Adding a new package", CGAL coding +conventions, and the CGAL package directory structure. + +This document is self-contained. A new session can pick up any finding below and +act on it without prior context. + +Status legend: β›” Blocker (submission rejected on sight) Β· πŸ”΄ Critical Β· 🟑 Important Β· πŸ”΅ Polish + +> **Companion documents:** +> - `external-audit-2026-05-30.md` β€” port-faithfulness bugs +> - `test-coverage-error-handling-audit-2026-05-31.md` β€” test gaps + error handling +> - `api-performance-audit-2026-05-31.md` β€” API consistency + performance +> - **this file** β€” CGAL submission readiness +> +> **Bottom line:** the *algorithmic* maturity is strong, but the package is **not +> submittable as-is**. There are **three** hard blockers β€” **provenance/porting +> rights (G0)**, licensing (G1), and directory layout (G2) β€” plus several +> CGAL-convention gaps. G0 is a legal precondition that gates everything else: +> until the right to port the original Varylab/TU-Berlin code is clarified with +> its authors, no license decision (G1) can be made and no public release or +> submission should happen. The remaining items are packaging and +> documentation-format work β€” substantial but mechanical, no research required. + +--- + +## Verdict at a glance + +| # | Sev | Title | +|---|-----|-------| +| G0 | β›” | **Porting rights unclear** β€” the original Varylab/TU-Berlin project has *no license* (all rights reserved); the C++ code is a documented *port* (derivative work). Must clarify with the authors whether porting + (re)licensing is permitted **before** any license/release decision | +| G1 | β›” | **License is MIT** β€” CGAL requires GPLv3+ (algorithmic) or LGPLv3+. Moot until G0 is resolved | +| G2 | β›” | **Directory layout is not CGAL package layout** β€” no `package_info/`, no `doc//`, tests/examples not in CGAL harness | +| G3 | πŸ”΄ | **CGAL is vendored** (`code/deps/CGAL-6.1.1`) β€” a CGAL package must not bundle CGAL | +| G4 | πŸ”΄ | **Concept not in a Doxygen block** β€” `\cgalConcept` is in `//` comments, won't render; no `doc//Concepts/` file | +| G5 | πŸ”΄ | **No user manual** β€” CGAL requires `.txt` (\mainpage) + `PackageDescription.txt` | +| G6 | πŸ”΄ | **Test suite uses GoogleTest + FetchContent** β€” CGAL uses its own test harness; external network deps are disallowed | +| G7 | 🟑 | **Primary template `Default_conformal_map_traits` is undefined** β€” only `Surface_mesh` works; concept claims `FaceGraph` genericity | +| G8 | 🟑 | **`.hpp` extension for implementation headers** β€” CGAL convention is `.h` | +| G9 | 🟑 | **Single-header monolith risk** β€” heavy `inline` free functions in headers; CGAL prefers `internal/` split + documented public surface | +| G10 | 🟑 | **No demo, no benchmark dir** β€” recommended for a numeric package | +| G11 | πŸ”΅ | **Naming inconsistencies** carry into the public API (see api-performance-audit A1–A5) | +| G12 | πŸ”΅ | **Copyright line** β€” CGAL files carry an `INRIA/GeometryFactory` style header + the CGAL license boilerplate, not a bare MIT line | + +--- + +## G0 β€” β›” Porting rights: the original is unlicensed and this is a derivative work + +> **This is the precondition for everything else in this audit (and for the license +> finding G1). It must be resolved with the original authors before any public +> release, relicensing, or CGAL submission.** +> +> *Disclaimer: the auditor is not a lawyer. This is a practical risk assessment, +> not legal advice. Confirm with the project owner / institutional legal counsel.* + +### Evidence +The original project (`gitlab.discretization.de:varylab/conformallab.git`, +mirrored to `git.eulernest.eu`) is the **Varylab / TU-Berlin** discrete-conformal +codebase (DFG SFB/Transregio "Discretization in Geometry and Dynamics"; author +circle around Stefan Sechelmann, Boris Springborn). + +``` +$ find /Users/tarikmoussa/Desktop/conformallab -maxdepth 2 \ + \( -iname 'license*' -o -iname 'copying*' \) +# β†’ (nothing) +# Java sources carry NO license header; only build.xml + .project exist. +``` + +- The original has **no LICENSE file and no per-file license headers** β†’ under + default copyright law this means **all rights reserved** by the TU-Berlin/Varylab + authors. "Publicly visible on an academic GitLab" is **not** a license grant. +- `ConformalLabpp` describes itself as a **port**: source comments say + *"Ported from de.varylab.discreteconformal.functional.EuclideanCyclicFunctional"*, + and the test suite asserts "Java golden-oracle parity". That makes the C++ code a + **derivative work of the original source code**, not merely an independent + implementation of the published papers. + +### Why it blocks +A derivative work of all-rights-reserved code **cannot be licensed by the porter +alone** β€” neither as MIT (the current state) nor as GPL (the CGAL requirement) β€” +without permission from the original copyright holders. The existing MIT header on +`ConformalLabpp` is therefore on shaky ground today, independent of CGAL. + +Crucial distinction: + +| What was actually reproduced | Legal status | +|---|---| +| The **published algorithms** (Springborn 2020, Bobenko-Pinkall-Springborn 2010, Luo 2004) | Free to reimplement β€” mathematical methods are not copyrightable, only their concrete code expression | +| The **source code** of the Varylab Java project, line-by-line | Derivative work β†’ requires the authors' permission to port and (re)license | + +The "Ported from …" comments currently document a code derivation in writing, which +is the principal risk. + +### Action (owner-level, before any code/license change) +1. **Determine provenance honestly:** is `ConformalLabpp` a clean-room + reimplementation *from the papers*, or a translation *of the Varylab code*? + The current comments assert the latter. +2. **Contact the original authors** (Sechelmann / Springborn, TU Berlin / Varylab) + to either (a) obtain written permission to port and relicense (ideally under + GPLv3+ so CGAL submission stays open), or (b) confirm the original is intended to + be freely reusable and get that in writing / as an added LICENSE upstream. +3. **Pause public releases and CGAL submission** until 1–2 are settled. +4. If permission cannot be obtained, the only clean path is a genuine clean-room + reimplementation from the published papers, with the "Ported from …" provenance + comments removed and replaced by paper citations. + +### Done when +- Written clarity exists on the right to port + (re)license the original code; + the provenance comments in the source match that reality. + +--- + +## G1 β€” β›” License: MIT is incompatible with CGAL submission + +> Blocked by **G0** β€” do not act on G1 until porting/relicensing rights are clear. + +### Evidence +All 41 headers carry: +```cpp +// SPDX-License-Identifier: MIT +``` +(`grep -rh "SPDX-License-Identifier" code/include/` β†’ 41Γ— MIT, 0Γ— anything else.) + +### Why it blocks +CGAL is dual-licensed: the **algorithmic** packages are **GPLv3+**, the foundational +ones **LGPLv3+**, with a commercial license sold by GeometryFactory. A new package +must be contributed under the **same dual GPL/LGPL scheme**, with copyright assigned +or licensed appropriately. A package licensed MIT cannot be merged into the CGAL +tree β€” the editorial board will reject it at intake. + +### Action +This is a **licensing/legal decision for the project owner**, not a code change: +1. Decide whether to relicense the contributed package as GPLv3+ (the normal choice + for a new algorithmic CGAL package). +2. Replace every header's `SPDX-License-Identifier: MIT` with the CGAL license + header boilerplate (see `Installation/LICENSE*` in the CGAL tree for the exact + text and the `$URL$ $Id$ SPDX-License-Identifier: GPL-3.0-or-later` form). +3. Keep a separate MIT-licensed standalone build if a permissive standalone + distribution is also desired (dual-distribution is possible but must be explicit). + +> ⚠️ Do not auto-rewrite license headers without the owner's explicit decision β€” +> relicensing is irreversible for contributed copies and may involve other authors. + +### Done when +- Owner has chosen the license; headers carry the CGAL-conformant boilerplate. + +--- + +## G2 β€” β›” Directory layout is not a CGAL package + +### Evidence +Current layout: +``` +code/include/CGAL/... ← headers (good location) +code/examples/*.cpp ← flat, not per-package +code/tests/cgal/*.cpp ← GoogleTest, not CGAL harness +code/deps/CGAL-6.1.1 ← vendored CGAL (see G3) +(no package_info/ anywhere) +(no doc// with PackageDescription.txt) +``` + +CGAL expects a package rooted at a top-level package directory mirroring the CGAL +source tree: +``` +Discrete_conformal_map/ + include/CGAL/Discrete_conformal_map.h + include/CGAL/Conformal_map/... + doc/Discrete_conformal_map/ + Discrete_conformal_map.txt (user manual, \mainpage) + PackageDescription.txt (\package_listing entries) + Concepts/ConformalMapTraits.h (the concept, doxygen-only) + examples.txt, fig/ + examples/Discrete_conformal_map/*.cpp (+ CMakeLists using CGAL macros) + test/Discrete_conformal_map/*.cpp (CGAL test harness) + benchmark/Discrete_conformal_map/ (optional but recommended) + package_info/Discrete_conformal_map/ + copyright description.txt dependencies license.txt maintainer +``` + +### Action +Restructure into the CGAL package layout. This is mechanical but touches every file +location and the build. Recommended approach: +1. Create the `package_info/Discrete_conformal_map/` metadata files first (small). +2. Move headers under the package's `include/CGAL/` (already close). +3. Re-home examples β†’ `examples/Discrete_conformal_map/`, tests β†’ `test/Discrete_conformal_map/`. +4. Build the `doc/Discrete_conformal_map/` tree (G5). + +### Done when +- Layout matches an existing CGAL package (use e.g. `Surface_mesh_parameterization` + or `Heat_method_3` as a template β€” both are close in spirit). + +--- + +## G3 β€” πŸ”΄ CGAL is vendored inside the repo + +### Evidence +``` +code/deps/CGAL-6.1.1 ← full CGAL 6.1.1 source tree bundled +``` + +### Why it matters +A package being submitted *into* CGAL must build against the surrounding CGAL tree, +not bundle its own copy. The vendored `CGAL-6.1.1` (and the standalone build wiring +around it) is appropriate for the current standalone distribution, but must be +removed from the submitted package β€” the package's `dependencies` file declares CGAL +package dependencies instead. + +### Action +- For the CGAL-submission branch: drop `code/deps/CGAL-*`; rely on the host CGAL tree. +- Keep the vendored copy only on the standalone-distribution branch. +- List actual dependencies (`Surface_mesh`, `BGL`, `Property_map`, `Solver_interface`, + `Number_types`, plus the external `Eigen`) in `package_info/.../dependencies`. + +### Done when +- The submission branch contains no bundled CGAL; it builds inside a CGAL checkout. + +--- + +## G4 β€” πŸ”΄ The concept is not a documented CGAL concept + +### Evidence +`code/include/CGAL/Conformal_map_traits.h:36-76` declares the concept with +`\cgalConcept`, `\concept ConformalMapTraits`, `\cgalHasModelsBegin/End` β€” but the +whole block uses **plain `//` comments**, not a Doxygen `/*! ... */` block: + +```cpp +// ════════════════════════════════════════════════════════════════════════════ +// \cgalConcept +// +// \concept ConformalMapTraits +... +``` + +Doxygen only parses `/*! */` (or `///`) blocks, so none of this renders. There is +also no `doc//Concepts/ConformalMapTraits.h` β€” in CGAL, concepts live as +doxygen-only header files under the package `doc/.../Concepts/` directory, separate +from the model. + +### Action +1. Create `doc/Discrete_conformal_map/Concepts/ConformalMapTraits.h` containing the + concept as a `/*! \cgalConcept ... */`-documented (empty) class, following the + CGAL concept-file pattern. +2. Reference it via `\cgalHasModels` from `Default_conformal_map_traits`. +3. Convert the in-source `//` concept block to either a proper `/*! */` block or + remove it in favor of the doc/Concepts file. + +### Done when +- `ConformalMapTraits` appears in the generated reference manual under + `PkgConformalMapConcepts` with its model linked. + +--- + +## G5 β€” πŸ”΄ No CGAL user manual / PackageDescription + +### Evidence +`find . -name PackageDescription.txt -o -path '*doc*' -name '*.txt'` β†’ none (outside `deps/`). +The project has rich Markdown docs under `doc/` (api/, math/, architecture/), but +CGAL requires its own doxygen-driven manual files: +- `doc//.txt` β€” the user manual with `\mainpage`-style narrative, + `\cgalExample` references, figures. +- `doc//PackageDescription.txt` β€” the `\cgalPkgDescriptionBegin` listing + (authors, intro, license, dependencies, demo/example links) that populates the + CGAL "Packages" overview. + +### Action +- Author `Discrete_conformal_map.txt` (user manual). Much of the content can be + adapted from the existing `doc/api/pipeline.md` and `doc/math/*.md`. +- Author `PackageDescription.txt` with the standard CGAL macros. +- Wire both into a package `doc/Doxyfile.in` consistent with CGAL's doc build. + +### Done when +- `cgal_create_package` doc build produces a reference + user manual without warnings. + +--- + +## G6 β€” πŸ”΄ Test suite is GoogleTest + FetchContent + +### Evidence +`code/tests/cgal/CMakeLists.txt`: +```cmake +target_link_libraries(conformallab_cgal_tests PRIVATE GTest::gtest_main) +gtest_discover_tests(conformallab_cgal_tests ...) +``` +(plus FetchContent for GoogleTest elsewhere in the build). + +### Why it matters +CGAL's test infrastructure uses its own harness (`create_single_source_cgal_program`, +the `test/` programs return 0/non-0, driven by the CGAL testsuite scripts). External +network dependencies (FetchContent pulling GoogleTest) are not allowed in the CGAL +tree, and the GoogleTest dependency would need vendoring or removal. + +### Action +This is the largest mechanical task. Options: +- **(a)** Port the assertions to CGAL's plain-`assert` + `return EXIT_FAILURE` test + style, one program per test file under `test/Discrete_conformal_map/`. This is the + canonical CGAL approach and removes the GoogleTest dependency entirely. +- **(b)** Keep GoogleTest for the standalone branch; maintain a parallel CGAL-style + `test/` tree for the submission branch. + +The 272 existing tests are an asset β€” the assertions translate directly; only the +framework scaffolding changes. + +### Done when +- `test/Discrete_conformal_map/` runs under the CGAL testsuite with no GoogleTest. + +--- + +## G7 β€” 🟑 Genericity claimed but not delivered + +### Evidence +`Conformal_map_traits.h:80-92`: the primary template +`Default_conformal_map_traits` is **declared but undefined**; +only the `Surface_mesh` partial specialization (`:113`) is implemented. The concept +docstring (`:13-21`) advertises *"can run on any CGAL halfedge mesh β€” Surface_mesh, +Polyhedron_3, OpenMesh-adapter, pmp β€” without changes"*, and the API takes a +`TriangleMesh` template parameter, implying BGL-generic support. + +### Why it matters +CGAL reviewers scrutinize generic-programming claims. Shipping a `TriangleMesh` +template that compiles only for `Surface_mesh` is a documentation/implementation +mismatch: a user passing a `Polyhedron_3` gets a hard template error against an +undefined primary template, not a clean diagnostic. + +### Action +Either: +- **(a)** Implement a generic `FaceGraph`/`HalfedgeGraph` path using `boost::graph_traits` + + dynamic property maps (CGAL's `dynamic_vertex_property_t`), removing the + `Surface_mesh`-only restriction β€” this is the "Phase 8a.2" already on the roadmap; or +- **(b)** Until then, narrow the documented contract: state explicitly that only + `Surface_mesh` is supported, and `static_assert` a friendly message in the primary + template instead of leaving it undefined. + +### Done when +- The advertised mesh-type support matches what compiles; unsupported types yield a + `static_assert` message, not a raw template error. + +--- + +## G8 β€” 🟑 Header extension `.hpp` vs CGAL `.h` + +### Evidence +The implementation headers use `.hpp` (`euclidean_functional.hpp`, `newton_solver.hpp`, +…), while the CGAL-facing headers correctly use `.h` (`Discrete_conformal_map.h`). +CGAL's convention is `.h` for all headers. + +### Action +Rename the implementation headers to `.h` when they move under the package +`include/CGAL/Conformal_map/internal/` tree (G9), updating `#include` directives. +Low-risk, mechanical; do it as part of the G2 restructure. + +### Done when +- All package headers use `.h`. + +--- + +## G9 β€” 🟑 Public/internal header surface is undivided + +### Evidence +The 26 `code/include/*.hpp` headers are all effectively public (flat in `include/`), +mixing the user-facing CGAL entry points with low-level functional/geometry/solver +internals (`hyper_ideal_geometry.hpp`, `projective_math.hpp`, `clausen.hpp`, …). + +### Why it matters +CGAL packages keep a small documented public API and move implementation into +`include/CGAL//internal/`. Exposing every helper as a top-level header makes +the supported API surface ambiguous and the package harder to evolve without +breaking users. + +### Action +- Designate the public surface (the `CGAL/Discrete_*` entry points, the traits, the + layout/result types). +- Move everything else under `include/CGAL/Conformal_map/internal/`. +- Mark internal headers `\internal` for Doxygen (the parameters header already does + this correctly β€” `Conformal_map/internal/parameters.h` is the right pattern). + +### Done when +- Only the intended public headers are documented; the rest are under `internal/`. + +--- + +## G10 β€” 🟑 No demo / benchmark directory + +### Evidence +No `demo/` and no `benchmark/` directory. For a numerical package, CGAL reviewers +appreciate (and for some packages expect) a benchmark establishing performance +characteristics, and a small demo is a strong plus. + +### Action +- Add `benchmark/Discrete_conformal_map/` measuring solve time / iteration counts on + the existing smoke meshes (cathead/brezel/brezel2) β€” this also dovetails with the + performance findings in `api-performance-audit-2026-05-31.md` (B1/B4). +- A demo is optional; skip if time-constrained. + +### Done when +- A benchmark program exists and its numbers are referenced in the user manual. + +--- + +## G11 β€” πŸ”΅ Public-API naming inconsistencies (cross-ref) + +The naming inconsistencies documented in `api-performance-audit-2026-05-31.md` +(findings A1–A5: `assign_*`, `compute_*`, `gradient_check`, the `_map` suffix, and +the split result types) become **public-API-stability concerns** once the package is +in CGAL, because CGAL APIs are expected to be stable across releases. Resolve A1–A5 +**before** submission, not after β€” renaming a shipped CGAL API requires a deprecation +cycle. + +### Done when +- A1–A5 resolved; the public names are the ones intended to be permanent. + +--- + +## G12 β€” πŸ”΅ File header boilerplate + +### Evidence +Headers begin with `// Copyright (c) 2024-2026 Tarik Moussa.` + a bare SPDX line. +CGAL files use a standardized header including the `$URL$`/`$Id$` SVN/git keywords, +the SPDX line in CGAL's form, and the copyright holder convention used in the tree. + +### Action +Adopt the CGAL file-header template (tied to the G1 license decision). Mechanical +once G1 is settled. + +### Done when +- Headers match the CGAL file-header template. + +--- + +## What is already strong (genuine assets for submission) + +- **Named-parameter API** is idiomatic: `CGAL::parameters::gradient_tolerance(...) + .max_iterations(...)`, with the tag machinery correctly placed in + `Conformal_map/internal/parameters.h` and marked `\internal`. This is the hard part + done right. +- **Doxygen group hierarchy** (`PkgConformalMap` / `Ref` / `Concepts` / + `NamedParameters`) is set up correctly in `doxygen_groups.h`. +- **The `\internal` discipline** on the parameters header shows the author knows the + public/internal distinction β€” it just needs to be applied to the other helpers (G9). +- **Result types carry observability** (`sparse_qr_fallback_used`, iteration counts). +- **Algorithmic breadth + validation**: five DCE models with Java golden-oracle + parity and FD-gradient checks β€” a strong scientific basis, which is ultimately what + the editorial board cares most about. +- **Traits-based design intent** is correct CGAL architecture; it only needs the + genericity actually implemented (G7) and the concept properly documented (G4). + +--- + +## Suggested order of work (submission track) + +0. **G0 (porting rights)** β€” owner-level legal precondition; contact the + Varylab/TU-Berlin authors. **Nothing below should ship publicly until this is + resolved.** +1. **G1 (license decision)** β€” follows directly from G0; pick GPLv3+ if submission stays the goal. +2. **G2 + G3 + G8 (restructure)** β€” do the layout move and header rename together; + drop vendored CGAL on the submission branch. +3. **G9 (public/internal split)** β€” naturally part of the restructure. +4. **G11 / A1–A5 (lock the public names)** β€” before any docs are written against them. + *(A1–A3 are internal-API renames and are safe to do independently of G0; A4–A5 + touch the public CGAL surface and should wait for the G0/G1 outcome.)* +5. **G4 + G5 (concept file + user manual + PackageDescription)** β€” the doc deliverables. +6. **G6 (CGAL test harness)** β€” port the 272 tests off GoogleTest. +7. **G7 (deliver or narrow genericity)**. +8. **G10 (benchmark)** + **G12 (headers)** β€” polish. + +> Realistic framing for the owner: the **science is submission-grade**; the gap is +> **packaging + licensing + doc-format**. None of items G2–G12 require new research, +> but collectively they are a significant, mostly-mechanical effort. G1 is a decision, +> not work. diff --git a/doc/reviewer/test-coverage-error-handling-audit-2026-05-31.md b/doc/reviewer/test-coverage-error-handling-audit-2026-05-31.md new file mode 100644 index 0000000..05743ea --- /dev/null +++ b/doc/reviewer/test-coverage-error-handling-audit-2026-05-31.md @@ -0,0 +1,489 @@ +# Test-Coverage & Error-Handling Audit β€” ConformalLabpp + +**Date:** 2026-05-31 +**Auditor:** External reviewer (Claude Opus 4.8) +**Scope:** `code/include/` (31 headers), `code/tests/` (26 fast + 246 CGAL tests) +**Focus:** Test coverage gaps and error-handling correctness β€” NOT a port-faithfulness review. + +This document is self-contained. A new session can pick up any finding below and +act on it without prior context. Each finding includes: +- exact file path + line numbers (verified by direct file read) +- a minimal reproduction of the problematic code +- the recommended fix +- acceptance criteria for "done" + +Status legend: πŸ”΄ Critical Β· 🟑 Important Β· πŸ”΅ Hint / nice-to-have + +> **Note:** This audit is complementary to `external-audit-2026-05-30.md` (which +> covers port-faithfulness bugs). There is no overlap in findings β€” this one is +> strictly about *test coverage* and *error-handling robustness*. + +--- + +## How to read this document in a new session + +```bash +# 1. Build the CGAL test suite (needed to verify test-related findings) +cmake -S code -B build-cgal -DWITH_CGAL_TESTS=ON +cmake --build build-cgal --target conformallab_cgal_tests -j$(nproc) + +# 2. Run the full suite before making any change (establish baseline) +ctest --test-dir build-cgal -R '^cgal\.' --output-on-failure +# Expected: 246 passed, 0 failed + +# 3. Fast suite (for coverage-script findings) +cmake -S code -B build && cmake --build build --target conformallab_tests +ctest --test-dir build --output-on-failure # Expected: 26 passed + +# 4. Pick a finding below, apply the fix, re-run ctest, then commit. +``` + +The Java reference implementation (for any "what should the behavior be" question): +``` +/Users/tarikmoussa/Desktop/conformallab/src/de/varylab/discreteconformal/ +``` + +--- + +## Summary table + +| ID | Sev | Title | Primary file | +|----|-----|-------|--------------| +| C1 | πŸ”΄ | `solve_linear_system` discards its `ok` flag β€” double solver failure is invisible | `code/include/newton_solver.hpp:107` | +| C2 | πŸ”΄ | `coverage.sh` uses `\|\| true` on every lcov command β€” script can never fail | `scripts/quality/coverage.sh` | +| C3 | πŸ”΄ | Coverage not CI-gated β€” no enforcement mechanism exists | CI config | +| I1 | 🟑 | `NewtonResult` cannot distinguish MaxIter / LinearSolverFailed / LineSearchStalled | `code/include/newton_solver.hpp:51` | +| I2 | 🟑 | `serialization.hpp`: 4 throw statements, 0 negative tests | `code/include/serialization.hpp` | +| I3 | 🟑 | `load_mesh` non-triangle-mesh throw path untested | `code/tests/cgal/test_mesh_io.cpp` | +| I4 | 🟑 | `spherical_hessian.hpp:114` throw has no `EXPECT_THROW` test | `code/tests/cgal/test_spherical_hessian.cpp` | +| I5 | 🟑 | Coverage measures only the 26 fast tests (~9.6% of suite) | `scripts/quality/coverage.sh` | +| H1 | πŸ”΅ | `res.iterations` is wrong/stale when solver breaks on `!ok` | `code/include/newton_solver.hpp` (Γ—5) | +| H2 | πŸ”΅ | 5 near-identical Newton loops β€” maintenance risk | `code/include/newton_solver.hpp` | +| H3 | πŸ”΅ | `enforce_gauss_bonnet` does not report magnitude of applied correction | `code/include/gauss_bonnet.hpp` | +| H4 | πŸ”΅ | `reduce_to_fundamental_domain`: boundary `Im(Ο„)==0.0` untested | `code/tests/cgal/test_phase7.cpp` | +| H5 | πŸ”΅ | Degenerate-triangle path in Newton solver not integration-tested | `code/include/euclidean_hessian.hpp:85` | +| H6 | πŸ”΅ | 2 ARM64 tests marked `@Ignore` β€” platform-specific coverage gap | test suite | + +--- + +## FINDING-C1 β€” πŸ”΄ `solve_linear_system` silently discards its failure flag + +### Location +`code/include/newton_solver.hpp` lines 107–114 + +### Problem +The public linear-system solver computes `ok` from `solve_with_fallback` but never +returns or surfaces it. On double solver failure (both LDLT and SparseQR fail), +the function returns `Eigen::VectorXd::Zero(n)` β€” indistinguishable from a genuine +zero solution. + +```cpp +inline Eigen::VectorXd solve_linear_system( + const Eigen::SparseMatrix& A, + const Eigen::VectorXd& rhs, + bool* fallback_used = nullptr) +{ + bool ok = false; + return detail::solve_with_fallback(A, rhs, ok, fallback_used); + // ^^ `ok` computed, then dropped +} +``` + +This function is documented as a *public* primitive ("Exposing it publicly lets +callers reuse the logic"). A downstream caller has no way to detect that the +solve failed. + +### Fix +Add an optional `bool* ok` out-parameter (mirroring `fallback_used`), or return a +small struct `{ VectorXd x; bool ok; }`. Prefer the out-parameter to preserve the +existing return type and call sites. + +```cpp +inline Eigen::VectorXd solve_linear_system( + const Eigen::SparseMatrix& A, + const Eigen::VectorXd& rhs, + bool* fallback_used = nullptr, + bool* ok = nullptr) +{ + bool ok_local = false; + auto x = detail::solve_with_fallback(A, rhs, ok_local, fallback_used); + if (ok) *ok = ok_local; + return x; +} +``` + +### Acceptance criteria +- A new test in `test_newton_solver.cpp` constructs a genuinely unsolvable system + (e.g. an inconsistent over-determined sparse system that defeats both LDLT and QR) + and asserts `ok == false`. +- Existing 3 SparseQR-fallback tests still pass. + +--- + +## FINDING-C2 β€” πŸ”΄ Coverage script can never fail + +### Location +`scripts/quality/coverage.sh` lines 95–112 + +### Problem +Every lcov/genhtml invocation is suffixed with `|| true`: + +```bash +lcov --capture ... >/dev/null 2>&1 || true +lcov --extract ... --output-file "$BUILD_DIR/coverage.info" ... || true +genhtml ... || true +``` + +If lcov fails completely, `coverage.info` is empty, but the script still exits 0. +The script is documented as a future CI quality gate ("once a coverage threshold +is agreed … the gate can be a single line in cpp-tests.yml") β€” but as written it +would always pass, defeating the purpose of a gate. + +### Fix +- Remove `|| true` from the `--capture` and `--extract` steps; let real failures + propagate (the `set -euo pipefail` at the top will then catch them). +- Keep tolerance ONLY for the known-noisy `genhtml` cosmetic step if necessary, + but verify `coverage.info` is non-empty explicitly: + ```bash + [ -s "$BUILD_DIR/coverage.info" ] || { echo "FAIL: empty coverage trace" >&2; exit 1; } + ``` + +### Acceptance criteria +- Running the script with a deliberately broken lcov (e.g. `PATH` without gcov data) + exits non-zero. +- A successful run still produces a non-empty `coverage.info` and exits 0. + +--- + +## FINDING-C3 β€” πŸ”΄ Coverage is not CI-gated + +### Location +CI configuration (`.gitea/workflows/cpp-tests.yml`) + `scripts/quality/coverage.sh:12-14` + +### Problem +The script header states verbatim: *"Local-only. Not gated in CI yet; once a +coverage threshold is agreed with the reviewer (e.g. 80 %)…"*. There is no +enforcement: coverage can silently regress to 0% with no signal. + +### Decision β€” agreed thresholds (2026-05-31) +For a numerical, header-only library the agreed gate is: + +| Metric | Gate | Rationale | +|---|---|---| +| **Line coverage** | **80 %** | Industry-standard, defensible, not punitive | +| **Branch coverage** | **70 %** | Numerical code has legitimately hard-to-hit branches (degeneracy checks, solver fallbacks, NaN paths) | +| **Function coverage** | **90 %** | Header-only β†’ an untested function is an obvious gap | + +100 % is explicitly **not** the target β€” the degenerate-triangle and solver-failure +branches are expected to be only partially covered. + +### Fix +1. **First fix C2** (so the gate is trustworthy) **and I5** (so it measures the right + thing β€” a percentage over only the 26 fast tests is meaningless). +2. Establish a baseline by running the corrected script once. +3. If the measured baseline is **below** these gates, ratchet up: set the gate a few + points below baseline now and raise it as coverage improves β€” do not block work on + day one. If baseline already exceeds them, set the gates as above. +4. Add a `--fail-under` style check and wire it into the `/quality-gates` workflow. + +### Acceptance criteria +- `coverage.sh` accepts `COVERAGE_MIN_LINE` / `COVERAGE_MIN_BRANCH` / + `COVERAGE_MIN_FUNC` env vars (defaults 80 / 70 / 90) and exits 1 when any is missed. +- The quality-gates workflow invokes it with these thresholds. + +--- + +## FINDING-I1 β€” 🟑 NewtonResult cannot tell *why* it failed + +### Location +`code/include/newton_solver.hpp` lines 51–56 (struct) + the 5 solver loops +(`newton_euclidean`, `newton_spherical`, `newton_hyper_ideal`, +`newton_cp_euclidean`, `newton_inversive_distance`) + +### Problem +All five solvers collapse three distinct failure modes into `converged = false`: + +```cpp +if (!ok) break; // linear solver failed (numerically degenerate) +... +if (!improved) break; // line search stalled (bad start / non-convex region) +... +// loop exits normally // max_iter reached (might converge with more iterations) +``` + +A caller seeing `converged == false` cannot tell which happened, so cannot choose +an appropriate recovery (re-seed start point vs. raise max_iter vs. report +ill-posed problem). + +### Fix +Add to `NewtonResult`: +```cpp +enum class Status { Converged, MaxIterReached, LinearSolverFailed, LineSearchStalled }; +Status status = Status::MaxIterReached; +``` +Set it at each exit point. Keep the existing `converged` bool for backward +compatibility (`converged == (status == Status::Converged)`). + +### Acceptance criteria +- Each of the 4 statuses is asserted by at least one test (the existing + convergence tests cover `Converged`; add targeted tests for the 3 failure modes). + +--- + +## FINDING-I2 β€” 🟑 serialization.hpp: 4 throws, 0 negative tests + +### Location +`code/include/serialization.hpp` lines 82, 96, 198, 245 (all `throw std::runtime_error`) +Tests: `code/tests/cgal/test_layout.cpp` (only `ASSERT_NO_THROW` happy-path cases) + +### Problem +```cpp +if (!ofs) throw std::runtime_error("Cannot write: " + path); // L82, L198 +if (!ifs) throw std::runtime_error("Cannot open: " + path); // L96, L245 +``` +None of the four failure paths is exercised by a test. + +### Fix +Add to `test_layout.cpp`: +```cpp +TEST(Serialization, ThrowsOnUnwritablePath) { + EXPECT_THROW(save_result_json("/nonexistent_dir/x.json", res, ...), std::runtime_error); +} +TEST(Serialization, ThrowsOnMissingFileRead) { + EXPECT_THROW(load_result_json("/tmp/does_not_exist_conflab.json"), std::runtime_error); +} +``` +(Repeat for the XML variants to cover all four sites.) + +### Acceptance criteria +- All four throw sites are covered by `EXPECT_THROW`. + +--- + +## FINDING-I3 β€” 🟑 load_mesh non-triangle path untested + +### Location +`code/include/mesh_io.hpp` lines 61–64 (the `!is_triangle_mesh` throw) +Test file: `code/tests/cgal/test_mesh_io.cpp` + +### Problem +`test_mesh_io.cpp:111` tests the missing-file throw, but NOT the +"file exists but is a quad/polygon mesh" throw β€” which is the more interesting +guard (it protects every downstream functional from silent mis-handling). + +### Fix +Write a quad-only OFF file to `/tmp`, then: +```cpp +TEST(MeshIO, LoadMeshThrowsOnNonTriangleMesh) { + // write a single quad face OFF to a temp path ... + EXPECT_THROW(load_mesh(quad_path), std::runtime_error); +} +``` + +### Acceptance criteria +- The non-triangle branch in `load_mesh` is covered and asserts the runtime_error. + +--- + +## FINDING-I4 β€” 🟑 spherical_hessian throw untested + +### Location +`code/include/spherical_hessian.hpp` line 114 (`throw std::logic_error`) +Test file: `code/tests/cgal/test_spherical_hessian.cpp` + +### Problem +The Euclidean analogue (`euclidean_hessian.hpp:124`) IS tested via +`test_euclidean_hessian.cpp:237` (`EXPECT_THROW(..., std::logic_error)`). +The spherical equivalent has no such test. + +### Fix +Mirror the Euclidean test: assign an edge DOF (or whatever triggers the +`logic_error` at L114 β€” read the surrounding code to confirm the precondition) +and assert the throw. + +### Acceptance criteria +- `EXPECT_THROW(spherical_hessian(mesh, x, maps), std::logic_error)` present and passing. + +--- + +## FINDING-I5 β€” 🟑 Coverage measures only ~9.6% of the test suite + +### Location +`scripts/quality/coverage.sh` line 73 + +### Problem +```bash +ctest -E "^cgal\." --output-on-failure # excludes ALL 246 CGAL tests +``` +Coverage is computed from the 26 fast tests only. The core solvers +(`newton_solver.hpp`, `euclidean_functional.hpp`, `gauss_bonnet.hpp`, +`layout.hpp`, …) are exercised almost exclusively by the CGAL suite, so the +reported number does not reflect coverage of the project's actual core. + +The script's rationale is legitimate (CGAL tests under coverage instrumentation +need ~6 GB RAM). But the consequence β€” a coverage number that excludes the core β€” +should at minimum be made explicit, and ideally addressed. + +### Fix options +- **(a) Document loudly:** rename the metric to "fast-suite coverage" everywhere it + is reported, so nobody mistakes it for whole-project coverage. +- **(b) Tiered coverage:** add an opt-in `WITH_CGAL_COVERAGE=1` mode that runs the + CGAL suite under coverage on a high-memory machine (not the Pi runner), producing + a separate `coverage-cgal.info`. +- **(c) Merge traces:** if (b) is run, `lcov --add-tracefile` can combine fast + + CGAL into one report. + +### Acceptance criteria +- Decision recorded; if (a), the script + docs no longer imply whole-project coverage. + +--- + +## FINDING-H1 β€” πŸ”΅ res.iterations stale on `!ok` break + +### Location +`code/include/newton_solver.hpp` β€” all 5 solver loops (e.g. lines 269–284 for Euclidean) + +### Problem +```cpp +Eigen::VectorXd dx = detail::solve_with_fallback(H, -G, ok); +if (!ok) break; // res.iterations still holds the PREVIOUS iteration's value +... +res.iterations = iter + 1; // only reached if the solve + line search succeed +``` +On a linear-solver break, `res.iterations` does not reflect the iteration at which +the failure occurred (it's the prior value, or 0 on the first iteration). + +### Fix +Set `res.iterations = iter + 1;` immediately at the top of the loop body (or before +each `break`), so it always reflects the iteration count attempted. + +### Acceptance criteria +- After a forced solver failure, `res.iterations` equals the iteration index at failure. + +--- + +## FINDING-H2 β€” πŸ”΅ Five near-identical Newton loops + +### Location +`code/include/newton_solver.hpp` β€” `newton_euclidean`, `newton_spherical`, +`newton_hyper_ideal`, `newton_cp_euclidean`, `newton_inversive_distance` + +### Problem +The five solvers share an identical loop skeleton (gradient β†’ convergence check β†’ +Hessian β†’ solve_with_fallback β†’ line_search β†’ break logic). Differences are only: +the gradient fn, the Hessian fn, and the sign handling (spherical negates H). +Any fix to the shared logic (e.g. H1 above) must be applied five times. + +### Fix +Extract a template `newton_core(grad_fn, hessian_fn, x0, tol, max_iter)` and have +the five public functions become thin wrappers. The spherical sign flip can be a +parameter or handled inside its `hessian_fn` lambda. + +### Acceptance criteria +- All existing Newton tests (11 + 3 fallback) pass unchanged after refactor. +- The five public signatures are preserved (no caller changes). + +--- + +## FINDING-H3 β€” πŸ”΅ enforce_gauss_bonnet correction magnitude is silent + +### Location +`code/include/gauss_bonnet.hpp` (the `enforce_gauss_bonnet` function near L160) + +### Problem +`enforce_gauss_bonnet` adjusts cone angles so `check_gauss_bonnet` will subsequently +pass. If the input geometry is far from satisfying Gauss-Bonnet, a large silent +correction is applied with no signal to the caller. + +### Fix +Return (or log) the total/max applied correction so callers can detect +pathological input. Optionally warn above a configurable threshold. + +### Acceptance criteria +- A test feeds intentionally bad cone angles and asserts the reported correction is large. + +--- + +## FINDING-H4 β€” πŸ”΅ reduce_to_fundamental_domain boundary case + +### Location +`code/include/period_matrix.hpp` line 89 (`if (tau.imag() <= 0.0)`) +Test: `code/tests/cgal/test_phase7.cpp:315` + +### Problem +The guard is `<= 0.0`, but the existing test uses a clearly-negative `Im(Ο„)`. +The exact-boundary case `Im(Ο„) == 0.0` (on the real axis) is the more numerically +interesting trigger and is untested. + +### Fix +Add `EXPECT_THROW(reduce_to_fundamental_domain({1.0, 0.0}), std::domain_error);` + +### Acceptance criteria +- The `Im(Ο„)==0` boundary throws and is asserted. + +--- + +## FINDING-H5 β€” πŸ”΅ Degenerate-triangle path not integration-tested + +### Location +`code/include/euclidean_hessian.hpp` lines 85–90 (`euclidean_cot_weights` returns +`{0,0,0,false}` for degenerate triangles) + +### Problem +Degenerate faces yield zero cotangent weights, which can make the assembled Hessian +singular. The QR fallback will then "solve" it, but with undefined solution quality. +There is no test feeding a mesh with degenerate faces through `newton_euclidean` +to characterize the behavior. + +### Fix +Add a test with a near-degenerate (sliver) triangle mesh and assert either: +- the solver reports a meaningful non-converged status (after I1), or +- the result is documented as best-effort. + +### Acceptance criteria +- Behavior on degenerate input is characterized by an explicit test, not left undefined. + +--- + +## FINDING-H6 β€” πŸ”΅ Two ARM64 tests marked @Ignore + +### Location +Test suite (introduced in commit `e2b0cdf`: "mark 2 known ARM64 failures as @Ignore") + +### Problem +The primary CI runner is ARM64 (Raspberry Pi). Two tests are skipped there, so +those code paths run unverified on the production CI platform. + +### Fix / Action +- Locate the two `@Ignore`/`GTEST_SKIP`-guarded tests (grep for `ARM64`, `aarch64`, + `GTEST_SKIP`, `DISABLED_`). +- Document the root cause (numeric tolerance? CGAL kernel difference?) in this file. +- Either widen tolerances to make them pass on ARM64, or run them on an x86 CI lane. + +### Acceptance criteria +- Root cause documented; tests either re-enabled or moved to an x86 lane that runs them. + +--- + +## What is already good (do not "fix") + +- Consistent exception taxonomy: `std::runtime_error` at I/O boundaries, + `std::logic_error` for programmer errors, `std::domain_error` for math domain + violations β€” idiomatic and correct. +- `NewtonResult.converged` flag exists (many implementations omit it). +- Finite-difference gradient checks for every functional β€” the right validation style. +- Java golden-oracle parity tests β€” high value for a port. +- LDLTβ†’SparseQR fallback is itself unit-tested (tests #12–14 in `test_newton_solver.cpp`). +- Header-only design keeps the coverage scope (`code/include/`) clean. + +--- + +## Suggested order of work + +1. **C2** (make the coverage script honest) β€” small, unblocks C3 and C5. +2. **I5** (decide what coverage measures) β€” decision, then small script change. +3. **C3** (gate it) β€” needs owner's threshold decision. +4. **C1 + I1 + H1** (Newton error-reporting) β€” do together; they touch the same code. +5. **H2** (refactor the 5 loops) β€” do *after* C1/I1/H1 so you refactor the fixed version. +6. **I2, I3, I4, H4** (missing negative tests) β€” independent, parallelizable, low-risk. +7. **H3, H5, H6** (robustness characterization) β€” lower priority.