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/dependency-license-audit-2026-05-31.md b/doc/reviewer/dependency-license-audit-2026-05-31.md new file mode 100644 index 0000000..42a2458 --- /dev/null +++ b/doc/reviewer/dependency-license-audit-2026-05-31.md @@ -0,0 +1,167 @@ +# Dependency & License-Compatibility Audit β€” ConformalLabpp + +**Date:** 2026-05-31 +**Auditor:** External reviewer (Claude Opus 4.8) +**Scope:** Vendored/auto-fetched dependencies, their license compatibility, and the +provenance/license of bundled **data assets** (test meshes). +**Focus:** Is the dependency + data licensing coherent β€” especially in light of the +unresolved provenance/license blocker (CGAL audit **G0/G1**)? + +Status legend: πŸ”΄ Critical Β· 🟑 Important Β· πŸ”΅ Polish + +> **Good news up front:** a `code/deps/THIRD-PARTY-LICENSES.md` already exists and is +> a genuinely good per-dependency compatibility matrix (CGAL, Eigen, libigl, GLFW, +> nlohmann/json). This audit does **not** redo that work. It flags two things that +> matrix cannot see: (D1) it is built on the MIT premise that G0 undermines, and +> (D2) the bundled **test meshes have no provenance or license at all**. + +--- + +## Summary table + +| ID | Sev | Title | Location | +|----|-----|-------|----------| +| D1 | πŸ”΄ | The dependency-license matrix is predicated on "conformallab++ is MIT" β€” a premise CGAL-audit G0 shows is unfounded; the whole compatibility conclusion is conditional on the unresolved license decision | `code/deps/THIRD-PARTY-LICENSES.md` | +| D2 | πŸ”΄ | Bundled test meshes (`cathead.obj`, `brezel*.obj`, torus OFFs) have **no provenance and no license** β€” likely third-party / Varylab-origin, redistributed without attribution | `code/data/` | +| D3 | 🟑 | No per-dependency license *files* under most vendored trees (only GLFW ships one); the matrix is the only record | `code/deps/*/` | +| D4 | πŸ”΅ | Vendored-tarball integrity is asserted ("bit-for-bit identical") but not checksum-verified in-repo | `code/deps/tarballs/` | + +--- + +## D1 β€” πŸ”΄ License matrix is conditional on the unresolved MIT premise + +### Evidence +`code/deps/THIRD-PARTY-LICENSES.md` opens its analysis with: +> *"## conformallab++ itself … **MIT** … Every C++ source file carries +> `SPDX-License-Identifier: MIT`"* + +and every dependency row evaluates "**Compatibility with MIT distribution**". + +### Problem +The matrix's central column β€” and therefore its conclusion that everything is +distributable β€” assumes conformallab++ *is* MIT-licensed and *may* be distributed as +MIT. CGAL-audit **G0** establishes that this premise is unfounded: the code is a +documented port (derivative work) of the unlicensed Varylab/TU-Berlin project, so the +MIT grant itself is on shaky ground. Two consequences cascade: + +1. If porting rights are **not** granted, there is no valid MIT (or any) license to + be "compatible with" β€” the matrix evaluates compatibility against a license the + project may not be entitled to use. +2. If the project relicenses to **GPLv3+** for CGAL submission (G1), the entire + "compatible with MIT distribution" analysis must be **redone against GPLv3+** + (e.g. the CGAL LGPL/GPL split, Eigen's MPL-2.0, libigl's MPL-2.0 are all + GPL-compatible β€” but that needs stating, not assuming). + +### Fix +- Resolve G0/G1 first (owner-level). +- Then re-run the matrix against the *actual* outbound license. Add a column + "compatibility with GPLv3+ distribution" if the CGAL path is taken. +- Until resolved, annotate `THIRD-PARTY-LICENSES.md` with a banner that its + conclusions are conditional on the pending G0/G1 decision. + +### Acceptance criteria +- The matrix's outbound-license assumption matches the resolved G0/G1 outcome and is + re-evaluated against it. + +--- + +## D2 β€” πŸ”΄ Bundled test meshes have no provenance or license + +### Evidence +``` +code/data/obj: brezel.obj brezel2.obj cathead.obj tetraflat.obj +code/data/off: simple_cupe.off torus_4x4.off torus_8x8.off torus_hex_6x6.off torus_skewed_4x4.off +``` +`find code/data -iname '*readme*' -o -iname '*licen*' -o -iname '*source*'` β†’ **nothing.** + +### Problem +These meshes are committed and redistributed with the project, used by the smoke and +parity tests (`test_scalability_smoke.cpp`, `test_geometry_utils.cpp` references +`brezel2.obj`, etc.). But: + +- `cathead.obj` is a **well-known third-party mesh** that circulates in graphics + courses/datasets β€” it is almost certainly not original to this project and carries + whoever's terms. +- `brezel.obj` / `brezel2.obj` ("Brezel" = pretzel; genus-1/genus-2) are the exact + example surfaces used in the **Varylab/TU-Berlin** discrete-conformal work β€” i.e. + likely the *same* upstream as the G0 code-provenance problem. + +Redistributing third-party data assets without attribution or a license is the same +class of issue as the code port (G0), and CGAL submission would flag it: CGAL example +data must have clear redistribution rights. + +### Fix +- Establish each mesh's origin and license. For `cathead.obj`, identify the standard + source and its terms. For the `brezel*` meshes, this is part of the **same + conversation with the Varylab/TU-Berlin authors** as G0. +- Add `code/data/PROVENANCE.md` listing each file: source, author, license, URL. +- Replace any mesh whose redistribution rights cannot be confirmed with a + cleanly-licensed or self-generated equivalent (the torus OFFs look procedurally + generated β€” confirm and document that, which would make them safe). + +### Acceptance criteria +- Every file under `code/data/` has documented provenance + redistribution rights; + unconfirmed assets are replaced. + +--- + +## D3 β€” 🟑 Most vendored trees ship no license file + +### Evidence +``` +eigen-3.4.0: NONE FOUND CGAL-6.1.1: NONE FOUND +libigl-2.6.0: NONE FOUND glfw-3.4: LICENSE.md βœ“ +``` +Only GLFW retains its upstream license file; the others rely solely on the central +`THIRD-PARTY-LICENSES.md`. + +### Problem +When vendoring source trees, the upstream LICENSE/COPYING file should travel with the +code (it is usually a license *requirement* β€” MPL-2.0 and LGPL both require preserving +license notices). A central summary is good practice but does not substitute for the +upstream notice files inside each tree. + +### Fix +Restore each upstream license file into its vendored tree (`eigen-3.4.0/COPYING.*`, +`CGAL-6.1.1/LICENSE*`, `libigl-2.6.0/LICENSE*`). They were likely stripped during +trimming of the vendored copies. + +### Acceptance criteria +- Each vendored dependency tree contains its upstream license notice file. + +--- + +## D4 β€” πŸ”΅ Vendored tarball integrity not checksum-verified + +### Evidence +`THIRD-PARTY-LICENSES.md` states the cached tarballs are "bit-for-bit identical to the +upstream releases", but no checksum manifest is committed. + +### Problem +The reproducibility claim (the stated rationale for vendoring) rests on an unverified +assertion. A corrupted or substituted tarball would not be detected. + +### Fix +Commit a `code/deps/tarballs/SHA256SUMS` and verify it in the extraction step of the +CMake/deps logic. + +--- + +## What is already good + +- `THIRD-PARTY-LICENSES.md` is a thorough, honest per-dependency matrix with real + compatibility reasoning (the CGAL LGPL/GPL split, Eigen's NonMPL2 gating, the + viewer-only scope of libigl/GLFW) β€” excellent practice, just built on a premise + (D1) that G0 destabilizes. +- The dependency choices themselves are clean: Eigen (MPL-2.0), libigl (MPL-2.0), + GLFW (zlib), nlohmann/json (MIT) are all permissive; only CGAL carries copyleft, and + its header-only LGPL consumption is correctly reasoned. +- Vendoring at fixed versions for build reproducibility is a defensible choice and is + documented. + +## Suggested order +1. **D2** (data provenance) β€” couple with the G0 conversation; CGAL-blocking and the + most clearly missing piece. +2. **D1** (re-base the matrix on the resolved license) β€” after G0/G1. +3. **D3** (restore upstream license files) β€” quick, and likely a license requirement. +4. **D4** (checksum manifest) β€” polish. diff --git a/doc/reviewer/input-validation-audit-2026-05-31.md b/doc/reviewer/input-validation-audit-2026-05-31.md new file mode 100644 index 0000000..e3d63a7 --- /dev/null +++ b/doc/reviewer/input-validation-audit-2026-05-31.md @@ -0,0 +1,197 @@ +# Input-Validation & Malformed-Input Audit β€” ConformalLabpp + +**Date:** 2026-05-31 +**Auditor:** External reviewer (Claude Opus 4.8) +**Scope:** Robustness of the library's input boundaries β€” mesh I/O (OFF/OBJ/PLY) and +result (de)serialization (JSON/XML) β€” against malformed, adversarial, or +out-of-domain input. +**Focus:** What happens with *bad* input. Distinct from the error-handling audit, +which covered *internal* error propagation and the deliberate `throw` paths. + +Status legend: πŸ”΄ Critical Β· 🟑 Important Β· πŸ”΅ Polish + +> **Threat model:** this is a scientific library, not a network service, so the bar +> is "fail cleanly and diagnosably", not "resist attackers". But meshes and result +> files routinely come from other tools, other people, and older versions of this +> code β€” so malformed input is a *when*, not an *if*. + +--- + +## Summary table + +| ID | Sev | Title | Location | +|----|-----|-------|----------| +| V1 | 🟑 | JSON deserialization has no error handling β€” nlohmann exceptions leak the abstraction, not the library's `runtime_error` | `serialization.hpp:96-110` | +| V2 | 🟑 | XML parser uses unchecked `std::stoi`/`std::stod` on attributes β†’ uncaught `std::invalid_argument` on garbage | `serialization.hpp:258-261` | +| V3 | 🟑 | No validation of vertex coordinate values on load β€” NaN/Inf coordinates flow silently into the solver | `mesh_io.hpp:56-66` | +| V4 | 🟑 | JSON loader accesses nested keys (`j["solver"]["converged"]`) without `contains` checks β†’ throws `type_error`/creates nulls on schema drift | `serialization.hpp:104-108` | +| V5 | πŸ”΅ | Hand-rolled XML parser assumes one element per line and no escaping β€” silently mis-reads valid-but-reformatted XML | `serialization.hpp:248-270` | +| V6 | πŸ”΅ | No upper bound / sanity check on DOF-vector length vs mesh size when loading | `serialization.hpp` | + +--- + +## V1 β€” 🟑 JSON deserialization leaks nlohmann exceptions + +### Evidence +`serialization.hpp:96-108`: +```cpp +std::ifstream ifs(path); +if (!ifs) throw std::runtime_error("Cannot open: " + path); +json j; ifs >> j; // ← malformed JSON β†’ nlohmann::parse_error +... +res->converged = j["solver"]["converged"].get(); // ← wrong type β†’ type_error +``` + +### Problem +The "file missing" case is handled with the library's own `std::runtime_error` +(consistent with `mesh_io.hpp`). But the moment the file *exists and is malformed*, +`ifs >> j` throws `nlohmann::json::parse_error` and the typed accessors throw +`nlohmann::json::type_error` β€” neither is the library's documented error type, and +neither carries the file path. A caller writing `catch (const std::runtime_error&)` +(the pattern the examples use) will **miss** these. (`nlohmann` exceptions derive from +`std::exception`, not `std::runtime_error`.) + +### Fix +Wrap the parse + extraction in `try { ... } catch (const json::exception& e)` and +rethrow as `std::runtime_error("conformallab: malformed JSON in " + path + ": " + e.what())`, +matching the `mesh_io.hpp` boundary convention. + +### Acceptance criteria +- Loading a truncated/garbage JSON throws `std::runtime_error` with the path. +- A negative test (`EXPECT_THROW(..., std::runtime_error)`) covers it. + +--- + +## V2 β€” 🟑 XML parser: unchecked stoi/stod on attributes + +### Evidence +`serialization.hpp:258-261`: +```cpp +res->iterations = std::stoi(detail_xml::xml_get_attr(line, "iterations")); +res->grad_inf_norm = std::stod(detail_xml::xml_get_attr(line, "grad_inf_norm")); +``` + +### Problem +If the attribute is missing or non-numeric (`xml_get_attr` returns `""` or garbage), +`std::stoi("")` throws `std::invalid_argument` and out-of-range values throw +`std::out_of_range` β€” both uncaught, both not the library's `runtime_error`, neither +carrying the path. Same abstraction leak as V1, different exception family. + +### Fix +Parse defensively: check the attribute is non-empty and numeric, or wrap in +try/catch and rethrow as `std::runtime_error` with context (path + attribute name). + +### Acceptance criteria +- An XML file with a non-numeric `iterations` attribute throws `std::runtime_error`, + not `std::invalid_argument`. + +--- + +## V3 β€” 🟑 No validation of vertex coordinates on load + +### Evidence +`mesh_io.hpp:56-66` β€” `load_mesh` checks read success and `is_triangle_mesh`, but +does **not** inspect the vertex coordinate values. + +### Problem +A mesh file containing `nan`/`inf` coordinates (common from a crashed upstream tool, +or `1e308`-style overflow) reads in cleanly. The NaN then propagates: edge lengths +β†’ `lambda0` β†’ gradient β†’ Hessian β†’ the Newton solver, where it silently poisons the +result (`converged` may even read `true` if `NaN < tol` evaluates falsely in a way +that exits the loop). This is the worst kind of failure: silent, late, and +hard to trace back to the input. + +### Fix +After `read_mesh`, scan vertex coordinates for finiteness +(`std::isfinite`) and throw `std::runtime_error("conformallab: non-finite vertex +coordinate in " + filename)` on the first offender. O(V), negligible cost at the I/O +boundary β€” the same philosophy as the existing `is_triangle_mesh` guard. + +### Acceptance criteria +- Loading a mesh with a NaN/Inf coordinate throws `std::runtime_error`. +- Covered by a negative test. + +--- + +## V4 β€” 🟑 JSON loader assumes schema without checking + +### Evidence +`serialization.hpp:104-108` reads `j["solver"]["converged"]`, `["iterations"]`, +`["grad_inf_norm"]` and `j.at("dof_vector")` β€” only `geometry` is guarded with +`j.contains(...)` (`:99`). + +### Problem +`operator[]` on a missing key in nlohmann **inserts a null** (for non-const json) or +throws; `.get()` on null throws `type_error`. So a result file written by a +*future or older* schema version fails with an opaque nlohmann error rather than a +clear "missing field X" message. (`dof_vector` correctly uses `.at()`, which throws a +clear `out_of_range` β€” but still not the library's type.) + +### Fix +Validate the expected keys up front (or use `.at()` uniformly + the V1 try/catch), +producing a single `std::runtime_error("conformallab: result JSON missing field +'solver.converged'")`-style message. + +### Acceptance criteria +- A JSON missing `solver.iterations` yields a `runtime_error` naming the field. + +--- + +## V5 β€” πŸ”΅ Hand-rolled XML parser is format-fragile + +### Evidence +`serialization.hpp:248-270` parses line-by-line with `line.find("…` text) fits patterns on one +line. + +### Problem +This is not an XML parser β€” it is a line-pattern matcher. A semantically identical +file that pretty-prints elements across multiple lines, or adds an XML declaration / +namespace / attribute reordering, will be **silently mis-read** (fields default to +zero/empty) rather than rejected. Round-trips written by this same code work; nothing +else is guaranteed. + +### Fix +Either (a) document the XML format as a strict internal-only subset (and reject input +that doesn't match, rather than mis-reading it), or (b) if interop matters, use a real +XML parser. Given JSON is the primary format, (a) is the pragmatic choice β€” but the +"silently mis-read" behavior must become "explicitly reject". + +### Acceptance criteria +- Reformatted-but-valid XML is either parsed correctly or rejected with an error β€” + never silently mis-read into zeros. + +--- + +## V6 β€” πŸ”΅ No DOF-vector vs mesh sanity check + +### Evidence +`load_result_json` returns `x = j.at("dof_vector")` with no check that `x.size()` +matches the DOF count of the mesh it will be applied to. + +### Problem +A result file from a *different* mesh loads happily; the size mismatch only surfaces +later (out-of-bounds or wrong-answer) when `x` is indexed against the new mesh. + +### Fix +Where the loaded `x` is paired with a mesh, assert/throw on a size mismatch with a +clear message. (May belong at the call site rather than the loader.) + +--- + +## What is already good + +- The **file-missing** path is handled correctly and consistently + (`runtime_error` + path) in both `mesh_io.hpp` and `serialization.hpp`. +- `load_mesh` already enforces the triangle-mesh precondition at the boundary β€” the + right place and the right philosophy; V3 just extends it to coordinate finiteness. +- `dof_vector` uses `.at()` (throwing) rather than `operator[]` β€” partial good + practice that V4 asks to make uniform. +- JSON (nlohmann) is a robust, well-tested parser β€” the gap is only the missing + try/catch *around* it (V1), not the parser itself. + +## Suggested order +1. **V3** (NaN/Inf coordinate guard) β€” highest impact, prevents silent solver poisoning. +2. **V1 + V4** (JSON error handling + schema checks) β€” do together. +3. **V2** (XML stoi/stod) β€” same pattern as V1. +4. **V5 + V6** (XML strictness, size check) β€” polish. diff --git a/doc/reviewer/math-derivation-citation-audit-2026-05-31.md b/doc/reviewer/math-derivation-citation-audit-2026-05-31.md new file mode 100644 index 0000000..2db1ba5 --- /dev/null +++ b/doc/reviewer/math-derivation-citation-audit-2026-05-31.md @@ -0,0 +1,192 @@ +# Math-Derivation & Citation Audit β€” ConformalLabpp + +**Date:** 2026-05-31 +**Auditor:** External reviewer (Claude Opus 4.8) +**Scope:** Correctness of the mathematical derivations in `doc/math/` and the +integrity/consistency of the literature citations in code comments and docs. +**Focus:** Are the derivations sound and are the citations accurate, complete, and +consistent? Distinct from `java-port-audit.md` (which checks code vs. Java) β€” this +checks code/docs vs. *the published mathematics*. + +Status legend: πŸ”΄ Critical Β· 🟑 Important Β· πŸ”΅ Polish + +> **Good news up front:** `doc/math/references.md` is unusually scholarly and already +> contains several *self-corrections* (the Glickenstein "eq. (4.6)" numbering fix β†’ +> Β§5.2 parametrization; the SchlΓ€fli boundary-term re-attribution to Rivin–Schlenker +> 1999; the Bowers–Stephenson formula-vs-theory clarification). That is exemplary and +> rare. The findings below are the gaps that remain *despite* this strong baseline. + +--- + +## Summary table + +| ID | Sev | Title | Location | +|----|-----|-------|----------| +| M1 | 🟑 | Kolpakov–Mednykh volume formula is used in code but **absent from `references.md`** | `hyper_ideal_utility.hpp:80`, `hyper_ideal_functional.hpp:321` | +| M2 | 🟑 | BPS circle-packing paper cited as **"2010"** in code but **"2015"** in `references.md` β€” inconsistent year for a load-bearing reference of an *implemented* phase | `cp_euclidean_functional.hpp`, `newton_solver.hpp`, `Discrete_circle_packing.h` vs `references.md` | +| M3 | 🟑 | Future-dated / arXiv-only citations (2025, 2026) back *unimplemented* phases but sit alongside implemented ones β€” re-verify against published versions before submission | `references.md` | +| M4 | πŸ”΅ | No canonical citation-key convention β€” the same work appears as `Luo 2004` / `Luo-2004` / `Luo (2004)` / `Luo's 2004` etc. | code + docs | +| M5 | πŸ”΅ | The large hand-derivations (notably `hyperideal-hessian-derivation.md`, 34 KB) have their *results* numerically validated but the *prose derivations* are unreviewed by a domain expert | `doc/math/*-derivation.md` | + +--- + +## M1 β€” 🟑 Kolpakov–Mednykh volume formula not in references.md + +### Evidence +```cpp +// hyper_ideal_utility.hpp:80 +/// the Kolpakov-Mednykh formula (arxiv math/0603097). Same as Java … +// hyper_ideal_functional.hpp:321 +/// * Exactly one vertex ideal (…) β†’ Kolpakov-Mednykh volume +``` +`grep -c Kolpakov doc/math/references.md` β†’ **0**. + +### Problem +The hyper-ideal **volume** computation β€” a core ingredient of the Springborn-2020 +functional and the energy itself β€” rests on the Kolpakov–Mednykh formula (and a +Meyerhoff/Ushijima component, cited 6Γ— elsewhere). The code cites it inline by arXiv +ID, but `references.md`, the stated "citation source of truth", omits it entirely. A +reviewer auditing the math foundation against the references file would find a +load-bearing formula with no entry. + +### Fix +Add a `references.md` row: Kolpakov, Mednykh β€” *(full title)*, arXiv `math/0603097`, +mapped to `hyper_ideal_utility.hpp`. Likewise confirm the Meyerhoff / Ushijima 2006 +volume reference (cited in code) has a row. + +### Acceptance criteria +- Every formula cited inline in `code/include/` has a matching `references.md` entry. + +--- + +## M2 β€” 🟑 BPS paper: "2010" in code vs "2015" in references.md + +### Evidence +- Code/docs cite it as **2010** (e.g. `cp_euclidean_functional.hpp`, + `newton_solver.hpp`, `CGAL/Discrete_circle_packing.h`, plus 26 doc hits of + "Bobenko-Pinkall-Springborn 2010"). +- `references.md` lists it as **2015**: *"Discrete conformal maps and ideal hyperbolic + polyhedra, Geometry & Topology 19(4) (2015), arXiv 1005.2698."* + +### Problem +Both are "correct" in a sense β€” the arXiv preprint (`1005.2698`) is **2010**, the +published Geometry & Topology version is **2015** β€” but the project uses the two years +interchangeably for the *same* paper, including in the public CGAL header +(`Discrete_circle_packing.h`) and the `phase-9a-validation.md` mapping. This is the +reference for an **implemented, shipped** phase (9a.1 CP-Euclidean), so the +inconsistency is user-visible and would be flagged in CGAL review. + +### Fix +Pick one canonical citation (recommend the **published 2015** Geometry & Topology +version, with the arXiv 2010 ID noted) and use it everywhere. Update the code comments +and docs that say "2010" to match `references.md`, or vice-versa β€” but make them agree. + +### Acceptance criteria +- The BPS circle-packing paper is cited with a single, consistent year across code and + docs (matching `references.md`). + +--- + +## M3 β€” 🟑 Future-dated / preprint citations must be re-verified before submission + +### Evidence +`references.md` cites, among others: +- Bobenko, Lutz **2025** (arXiv 2310.17529) β€” Phase 9d.2 (πŸ”œ planned) +- Bowers, Bowers, Lutz **2026** (arXiv 2601.22903) β€” Phase 9b-analytic / 10cβ€² +- Lutz **2024** PhD thesis; Bobenko-Lutz 2024 IMRN + +### Problem +Two issues: +1. **Verification:** arXiv-only and future-dated (2025/2026) references can change + (version bumps, final published venue, even withdrawal). Before any public release + or CGAL submission, each should be re-checked against its final published form. The + arXiv ID `2601.22903` in particular (Jan 2026) is very recent relative to this + audit and warrants a manual confirmation that it resolves. +2. **Scope confusion:** these back **unimplemented "future research"** phases (πŸ”œ), + but the Bowers-Bowers-Lutz 2026 row claims it "supports correctness of the analytic + Hessian" β€” which *is* implemented (Phase 9b-analytic, βœ…). A reader could conclude + the shipped analytic Hessian *depends* on a 2026 preprint. Clarify that the + implemented derivation stands on its own (it is numerically validated, see M5) and + the 2026 paper is corroborating, not foundational. + +### Fix +- Re-verify every 2024–2026 / arXiv-only citation against its published version; add + DOIs once available. +- Sharpen the wording so implemented-phase correctness is never described as resting + on an unpublished/future reference. + +### Acceptance criteria +- All post-2023 citations are confirmed to resolve; no implemented-phase correctness + claim depends solely on a preprint/future reference. + +--- + +## M4 β€” πŸ”΅ No canonical citation-key convention + +### Evidence +The same works appear in many surface forms across code and docs: +`Luo 2004` / `Luo-2004` / `Luo (2004)` / `Luo's 2004`; `Springborn 2020` / +`Springborn (2020)` / `Springborn-2020`; `Bowers-Stephenson` vs `Bowers, Stephenson`. + +### Problem +Cosmetic but it defeats grep-ability and cross-referencing (cf. the false-negative in +this audit's own tooling: `Bowers-Bowers-Lutz 2026` did not match the comma-form +`Bowers, Bowers, Lutz`). For a library aiming at CGAL, a consistent citation key +(e.g. `[SpringbornDCG2020]`) referenced from both code and `references.md` is the +clean solution. + +### Fix +Define short citation keys in `references.md` and use them verbatim in code comments +and docs. + +### Acceptance criteria +- One canonical key per reference; code/docs cite the key, not free-form variants. + +--- + +## M5 β€” πŸ”΅ Large derivations: results validated, prose unreviewed + +### Evidence +`doc/math/hyperideal-hessian-derivation.md` (34 KB) derives the analytic HyperIdeal +Hessian; `validation.md` and the FD-vs-analytic tests (`test_hyper_ideal_hessian.cpp`, +block-FD vs full-FD) check the *result* numerically. + +### Problem +The derivation **result** is well-protected β€” the analytic Hessian is tested against +finite differences, so an algebra error would surface as a test failure. But the +**prose derivation itself** (the step-by-step algebra a reader would follow to trust +or extend it) has not been reviewed by a domain expert, and a derivation that "happens +to match FD at the tested points" can still contain a sign/indexing slip that the +specific test meshes don't excite. + +### Fix +- Have a discrete-differential-geometry expert read `hyperideal-hessian-derivation.md` + and `discrete-conformal-theory.md` for derivational soundness (this is naturally + part of the planned reviewer meeting β€” see `briefing.md`). +- Strengthen confidence cheaply by adding a randomized/property-based FD check (random + valid DOF vectors on several genera) so the numerical safety net covers more than the + fixed smoke meshes. + +### Acceptance criteria +- The key derivations are signed off by a domain reviewer, and/or the FD validation is + randomized rather than fixed-mesh-only. + +--- + +## What is already good + +- `references.md` is **scholarly and self-correcting** β€” it explicitly fixes its own + earlier over-citations (Glickenstein equation numbering, SchlΓ€fliβ†’Rivin–Schlenker, + Bowers–Stephenson). This is well above typical project hygiene. +- Implemented vs. planned references are marked βœ… / πŸ”œ β€” the reader can tell which + citations underpin shipped code (M3 only asks to keep that discipline airtight). +- The primary source (Sechelmann 2016 PhD thesis, CC BY-SA 4.0) is correctly + identified, and the Java provenance is linked β€” directly relevant to CGAL audit G0. +- Derivation results are numerically validated (FD vs analytic) β€” the right safety net + (M5 just asks to widen it and get a human sign-off). + +## Suggested order +1. **M2** (BPS year) + **M1** (Kolpakov–Mednykh) β€” quick citation fixes, user-visible. +2. **M3** (re-verify post-2023 refs; fix scope wording) β€” before any submission. +3. **M5** (expert sign-off + randomized FD) β€” fold into the reviewer meeting. +4. **M4** (citation keys) β€” polish. diff --git a/doc/reviewer/numerical-stability-audit-2026-05-31.md b/doc/reviewer/numerical-stability-audit-2026-05-31.md new file mode 100644 index 0000000..71d957c --- /dev/null +++ b/doc/reviewer/numerical-stability-audit-2026-05-31.md @@ -0,0 +1,234 @@ +# Numerical-Stability Audit β€” ConformalLabpp + +**Date:** 2026-05-31 +**Auditor:** External reviewer (Claude Opus 4.8) +**Scope:** Tolerance hierarchy, conditioning, catastrophic cancellation, and the +hard-coded "magic" constants across `code/include/`. +**Focus:** Floating-point robustness of a numerical library β€” distinct from +port-faithfulness (`java-port-audit.md`) and from internal error propagation +(`test-coverage-error-handling-audit-2026-05-31.md`). + +Status legend: πŸ”΄ Critical Β· 🟑 Important Β· πŸ”΅ Polish + +> **Relationship to existing audits:** `java-port-audit.md` already verified several +> per-formula numerical edge cases (degenerate triangles #1, overflow centring #10). +> This audit takes the *cross-cutting* view those miss: how the tolerances relate to +> each other, where constants are hard-coded vs. centralized, and where the formulas +> can lose precision. No finding here duplicates java-port-audit. + +--- + +## Summary table + +| ID | Sev | Title | Location | +|----|-----|-------|----------| +| N1 | πŸ”΄ | FD-Hessian step (`1e-5`) sets an accuracy floor (~`1e-10`) too close to the Newton tol (`1e-8`) β€” can stall quadratic convergence | `newton_solver.hpp:408,569` | +| N2 | 🟑 | Tolerance hierarchy is undocumented and uncoordinated β€” 7+ independent literals with no stated relationship | many headers | +| N3 | 🟑 | Discontinuous clamp `b<0 β†’ 0.01` breaks gradient/Hessian continuity at the domain boundary | `hyper_ideal_functional.hpp:179,274` | +| N4 | 🟑 | Magic constants (`-30.0`, `0.01`, `1e-15`, `1.0-1e-15`) are unnamed, undocumented, and scattered | `spherical_functional.hpp`, `spherical_geometry.hpp`, `hyper_ideal_functional.hpp` | +| N5 | 🟑 | Cotangent/area formula is cancellation-prone for needle/cap triangles | `euclidean_hessian.hpp:81-93` | +| N6 | πŸ”΅ | `constants.hpp` holds only Ο€/2Ο€ β€” all tolerances live as bare literals | `constants.hpp` | +| N7 | πŸ”΅ | No conditioning diagnostic on the Hessian before the linear solve | `newton_solver.hpp` | + +--- + +## N1 β€” πŸ”΄ FD-Hessian step size fights the convergence tolerance + +### Evidence +- `newton_hyper_ideal` and `newton_inversive_distance` build the Hessian by **finite + differences** with `hess_eps = 1e-5` (`newton_solver.hpp:408`, `:569`). +- Both solvers converge against `tol = 1e-8` on `β€–Gβ€–βˆž` (`:406`, `:567`). + +### Problem +A central-difference Hessian with step `h = 1e-5` has error `O(hΒ²) + O(Ξ΅_machine/h)`. +With `h = 1e-5` the truncation term is ~`1e-10` and the round-off term is +~`1e-16/1e-5 = 1e-11`; the achievable Hessian accuracy floor is therefore +**~`1e-10`–`1e-11`**. That is only ~2–3 decades below the convergence target +`tol = 1e-8`. Near the solution, where Newton should converge quadratically, an +inaccurate Hessian degrades the step to linear convergence and can **stall** the +iteration just above `tol` β€” exactly the regime `test_newton_phase9a.cpp` exercises. + +This is also why the line-search "stall" exit (FINDING-H1 / error-handling audit) +can trigger on otherwise well-posed problems: the FD Hessian, not the geometry, is +the limiting factor. + +### Fix +- Primary: replace the FD Hessian with the analytic / block-FD paths (cross-ref + `api-performance-audit` B1 β€” the HyperIdeal block-FD already exists and is more + accurate as well as faster). +- Until then: document the coupling, and consider a richer FD stencil or + Richardson extrapolation if FD must stay. A relative step `h = 1e-6Β·(1+|x_i|)` + is more robust than an absolute `1e-5`. + +### Acceptance criteria +- The FD-step/tol relationship is documented, or the analytic/block-FD Hessian + replaces FD so the floor drops well below `tol`. +- A test demonstrates quadratic (not stalled) convergence near the solution. + +--- + +## N2 β€” 🟑 Tolerance hierarchy is undocumented and uncoordinated + +### Evidence (the full set of independent literals) +| Constant | Value | Location | Meaning | +|---|---|---|---| +| Newton `tol` | `1e-8` | `newton_solver.hpp` (all 5 solvers) | convergence on `β€–Gβ€–βˆž` | +| FD `hess_eps` | `1e-5` | `newton_solver.hpp:408,569`, `hyper_ideal_hessian.hpp:67` | Hessian FD step | +| `gradient_check` `eps`/`tol` | `1e-5` / `1e-4` | `hyper_ideal_functional.hpp:474-475` | test-only FD check | +| Armijo `c1` | `1e-4` | `newton_solver.hpp:149` | sufficient-decrease | +| sparsity drop | `1e-15` | `newton_solver.hpp:599` | triplet prune threshold | +| Gauss-Bonnet `tol` | `1e-8` | `gauss_bonnet.hpp:134,154` | Ο‡ residual | +| spherical clamp | `1e-15` | `spherical_geometry.hpp:34` | `asin` domain guard | + +### Problem +These seven thresholds were each chosen locally (several "to match the Java +`FunctionalTest`"), with **no documented relationship**. Yet they interact: N1 shows +`hess_eps` vs `tol`; the sparsity drop `1e-15` interacts with `hess_eps` (FD values +below `1e-15` are pruned, but FD noise is ~`1e-11`, so the prune threshold is +effectively never the limiting factor β€” possibly dead). A reviewer cannot tell +which are load-bearing and which are arbitrary. + +### Fix +Add a short `doc/math/tolerances.md` (or a section in `constants.hpp`) that states +each tolerance, its role, and the constraints between them (e.g. "FD floor must be +β‰ͺ Newton tol", "Armijo c1 ∈ [1e-4, 1e-1]"). Centralize them as named `constexpr`. + +### Acceptance criteria +- Every numerical threshold is named, documented, and its relationship to the + others stated. + +--- + +## N3 β€” 🟑 Discontinuous clamp breaks gradient/Hessian continuity + +### Evidence +`hyper_ideal_functional.hpp:179-181` and `:274-276`: +```cpp +if (v1b && b1 < 0.0) b1 = 0.01; // negative scale β†’ snap to 0.01 +``` + +### Problem +This snaps any negative `b` to a constant `0.01`. The map `b ↦ max(b, 0.01)` is +continuous but **not differentiable** at `b = 0`, and the snap-to-constant makes the +*returned angles* locally independent of `b` for `b < 0` β€” so the analytic gradient +and the FD Hessian disagree across the boundary, and Newton steps that cross `b = 0` +get an inconsistent local model. The comment says it "mirrors the Java original", +but the Java code is a reference, not a correctness proof: a hard clamp inside the +function being differentiated is a known source of Newton stalls. + +### Fix +- Prefer keeping iterates in the feasible region via the line search (reject steps + that drive `b < b_min`) rather than clamping inside the energy evaluation. +- If clamping must stay, use a smooth barrier / softplus so the derivative is + continuous, and make `b_min` a named constant (N4). + +### Acceptance criteria +- The energy/gradient used by Newton is CΒΉ across the feasibility boundary, or the + line search keeps iterates strictly feasible so the clamp never fires. + +--- + +## N4 β€” 🟑 Unnamed magic constants + +### Evidence +- `spherical_functional.hpp`: `m.lambda0[e] = -30.0;` (very-short-edge floor) +- `spherical_geometry.hpp:34`: `if (half >= 1.0) half = 1.0 - 1e-15;` +- `hyper_ideal_functional.hpp`: `0.01` clamp (N3), `b < 0.0` thresholds +- `newton_solver.hpp:599`: `if (std::abs(val) > 1e-15)` + +### Problem +Each is a bare literal with at most an inline comment. `-30.0` (β‰ˆ `exp(-15)`, i.e. +edge length ~`3e-7`) and `1.0 - 1e-15` (the `asin`/`acos` domain guard) encode real +numerical reasoning that is invisible to the next maintainer and impossible to tune +consistently. + +### Fix +Promote to named `constexpr` in `constants.hpp` with a one-line rationale each, e.g. +`constexpr double LOG_LENGTH_FLOOR = -30.0; // exp(-15): edge below ~3e-7 treated as 0`. + +### Acceptance criteria +- No bare numeric tolerance/floor literals remain in the math headers; each is a + documented named constant. + +--- + +## N5 β€” 🟑 Cotangent/area formula is cancellation-prone + +### Evidence +`euclidean_hessian.hpp:81-93`: +```cpp +const double t12 = -l12 + l23 + l31; +const double t23 = +l12 - l23 + l31; +const double t31 = +l12 + l23 - l31; +... +const double denom2 = 2.0 * std::sqrt(t12 * t23 * t31 * l123); // = 8Β·Area +``` + +### Problem +For a needle triangle (one edge β‰ˆ sum of the other two), one of `t12/t23/t31` is the +difference of nearly-equal lengths β†’ **catastrophic cancellation**, and the area via +`sqrt` of the product loses precision. The cotangent weights then carry large relative +error, which feeds directly into the Hessian and the linear solve. The `<= 0` guard +catches the fully-degenerate case but not the *ill-conditioned-but-valid* one. + +### Fix +Use a numerically stable area formula (Kahan's formula for triangle area from sorted +side lengths) instead of the raw product-under-sqrt. This is a well-known, drop-in +improvement for cotangent-Laplacian assembly. + +### Acceptance criteria +- A sliver-triangle test shows the cotangent weights match a high-precision + (e.g. `long double` / Kahan) reference to acceptable relative error. + +--- + +## N6 β€” πŸ”΅ constants.hpp holds only Ο€ + +### Evidence +`constants.hpp` defines exactly `PI` and `TWO_PI`. Every tolerance from N2 lives +elsewhere as a literal. + +### Fix +Extend `constants.hpp` (or a sibling `tolerances.hpp`) to be the single source of +truth for the N2/N4 constants. Tie to N2's documentation. + +--- + +## N7 β€” πŸ”΅ No conditioning diagnostic before the linear solve + +### Evidence +`solve_with_fallback` (`newton_solver.hpp:65`) tries LDLT then SparseQR but never +reports the conditioning of `H`. + +### Problem +On an ill-conditioned-but-non-singular Hessian, LDLT "succeeds" and returns a +low-accuracy step with no signal. The `sparse_qr_fallback_used` flag only fires on +outright LDLT failure, not on near-singularity. + +### Fix +Optionally expose an estimate (e.g. smallest pivot magnitude from LDLT, or a cheap +condition estimate) in the diagnostics, so callers/tests can flag near-singular +solves. Low priority; pairs naturally with the FINDING-I1 status enum +(error-handling audit). + +--- + +## What is already good + +- `constants.hpp` centralizes Ο€ at 30 digits β€” the right instinct, just not applied + to tolerances (N6). +- The spherical `asin` domain guard (`spherical_geometry.hpp:34`) exists at all β€” + many implementations forget it (it just needs a named constant, N4). +- Central differences (not forward) are used for FD gradients/Hessians β€” the correct + O(hΒ²) choice. +- `java-port-audit.md` #10 already flagged the inversive-distance edge-length overflow + centring β€” consistent with this audit's spirit; coordinate the two. + +## Suggested order + +1. **N1** (FD step vs tol) β€” highest correctness impact; largely subsumed by the + `api-performance-audit` B1 analytic/block-FD work. +2. **N3** (discontinuous clamp) β€” fixes a real Newton-stall source. +3. **N5** (stable area) β€” drop-in robustness for the Euclidean Hessian. +4. **N2 + N4 + N6** (document + centralize tolerances/constants) β€” do together. +5. **N7** (conditioning diagnostic) β€” polish, pairs with I1. 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. diff --git a/doc/reviewer/thread-safety-audit-2026-05-31.md b/doc/reviewer/thread-safety-audit-2026-05-31.md new file mode 100644 index 0000000..1d14a92 --- /dev/null +++ b/doc/reviewer/thread-safety-audit-2026-05-31.md @@ -0,0 +1,139 @@ +# Thread-Safety & Reentrancy Audit β€” ConformalLabpp + +**Date:** 2026-05-31 +**Auditor:** External reviewer (Claude Opus 4.8) +**Scope:** Concurrency contract of the header-only library β€” reentrancy, hidden +mutable global state, and safety of parallel solves. +**Focus:** Can two threads use this library at the same time, and is that contract +documented? Currently undocumented everywhere. + +Status legend: πŸ”΄ Critical Β· 🟑 Important Β· πŸ”΅ Polish + +> **Good news up front:** a scan for mutable function-local `static` variables (the +> classic hidden-shared-state hazard) found **none**. All `static` in the headers are +> stateless factory/accessor functions (`MobiusMap::identity`, +> `Default_conformal_map_traits::theta_map`, etc.). So the library is *close* to +> trivially reentrant β€” the findings below are about the mesh-mutation contract and +> the missing documentation, not about data races in hidden globals. + +--- + +## Summary table + +| ID | Sev | Title | Location | +|----|-----|-------|----------| +| T1 | 🟑 | No documented threading contract anywhere β€” users cannot know what is safe | all public headers | +| T2 | 🟑 | The solve pipeline mutates the mesh via property maps β†’ concurrent solves on the *same* mesh are unsafe and silently so | `*_functional.hpp`, `newton_solver.hpp` | +| T3 | πŸ”΅ | Property-map *creation* (`add_property_map`) on a shared mesh from two threads is a data race on CGAL's internal map registry | `setup_*_maps`, traits accessors | +| T4 | πŸ”΅ | Eigen threading not pinned β€” relies on Eigen's defaults; undocumented | `newton_solver.hpp` | + +--- + +## T1 β€” 🟑 No documented threading contract + +### Evidence +No header, no `doc/` file, and no README section states whether the API is +thread-safe, reentrant, or single-threaded-only. `briefing.md` positions the library +as potential "infrastructure for your work" β€” which invites multi-threaded use it +never characterizes. + +### Problem +Without a stated contract, a user must read the implementation to discover (T2) that +distinct meshes are safe but a shared mesh is not. The safe pattern is real and +simple β€” it just needs to be written down. + +### Fix +Add a short "Thread safety" section to the main API doc (and a sentence in the key +headers) stating the contract, e.g.: +> *The library performs no synchronization and uses no shared mutable global state. +> Operations on **distinct** `ConformalMesh` objects are independent and may run +> concurrently. Operations on a **single** mesh (including any `setup_*_maps`, +> `assign_*`, gradient, Hessian, or `newton_*` call) mutate that mesh's property maps +> and must not run concurrently on the same mesh.* + +### Acceptance criteria +- A "Thread safety" subsection exists in the public API documentation and matches the + actual behavior described in T2/T3. + +--- + +## T2 β€” 🟑 Solve pipeline mutates the mesh; same-mesh concurrency is unsafe + +### Evidence +- `setup_*_maps(mesh)` adds property maps to `mesh`. +- `assign_*_dof_indices(mesh, m)` writes `m.v_idx` / `m.e_idx` into the mesh. +- `compute_*_lambda0_from_mesh(mesh, m)` writes `m.lambda0`. +- `newton_*` reads those maps and (via the gradient/Hessian) reads the mesh + repeatedly; the functions take `ConformalMesh& mesh` (non-const). + +### Problem +Two threads calling `newton_euclidean(mesh, …)` on the **same** `mesh` race on the +shared property maps β€” a silent data race, not a clean error. Because the signature +is `ConformalMesh&` (mutable) this is technically "expected", but nothing warns the +user, and the natural-looking "solve the same input mesh under two geometries in +parallel" is exactly the unsafe case. + +### Fix +- Document the contract (T1). +- Optionally, offer a value-semantics convenience overload that takes the mesh by + `const&` and operates on an internal copy, for users who want fire-and-forget + parallel solves. (Larger change; document first.) + +### Acceptance criteria +- The same-mesh hazard is documented; if a const/copying overload is added, a test + runs two concurrent solves on copies without a race (clean under ThreadSanitizer). + +--- + +## T3 β€” πŸ”΅ Concurrent property-map creation races CGAL's registry + +### Evidence +`setup_*_maps` and the traits accessors call `mesh.add_property_map<…>(name, default)`. + +### Problem +CGAL's `Surface_mesh::add_property_map` mutates the mesh's internal property +container. Two threads creating maps on the same mesh concurrently is undefined +behavior at the CGAL level β€” a sub-case of T2 but worth calling out because the +traits accessors (`Conformal_map_traits.h:158-179`) create-on-demand, so even a +"read-only-looking" first access mutates. + +### Fix +Covered by the T1 contract ("any `setup_*` … on the same mesh must not run +concurrently"). No code change beyond documentation unless the const overload (T2) is +pursued. + +--- + +## T4 β€” πŸ”΅ Eigen threading is unpinned and undocumented + +### Evidence +`newton_solver.hpp` uses `SimplicialLDLT` / `SparseQR` without calling +`Eigen::setNbThreads(…)` or documenting OpenMP behavior. + +### Problem +Eigen may use OpenMP threads internally depending on build flags. For a library that +might itself be called from parallel user code, the nesting behavior (and whether +`EIGEN_DONT_PARALLELIZE` is wanted) is unspecified. Distinct Eigen objects are +thread-safe, so this is low risk β€” but it should be a documented choice. + +### Fix +State the intended Eigen threading mode in the build/threading docs; consider pinning +it explicitly (`EIGEN_DONT_PARALLELIZE` for predictability, or document that callers +control it). + +--- + +## What is already good + +- **No mutable static locals** anywhere in the headers β€” the single most important + reentrancy property holds. +- All `static` members are pure functions (factories/accessors) β€” no shared state. +- The header-only, value-oriented design (results returned by value as + `NewtonResult` / `Conformal_map_result`) means outputs are not shared. +- Distinct-mesh concurrency genuinely *is* safe today β€” the contract just needs + documenting (T1), not implementing. + +## Suggested order +1. **T1** (write the contract) β€” trivial, unblocks safe parallel use immediately. +2. **T2/T3** (covered by the contract; optional const overload later). +3. **T4** (pin/document Eigen threading) β€” polish.