conformallab/ConformalLabpp
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

Merge pull request 'docs(reviewer): add 7 external audit documents (test/error, API/perf, CGAL, numerical, input, thread-safety, dependency, math/citation)' (#35) from audits-2026-05-31 into main
All checks were successful
C++ Tests / test-fast (push) Successful in 2m21s
Details
C++ Tests / quality-gates (push) Has been skipped
Details
API Docs / doc-build (push) Has been skipped
Details
Markdown link check / check (push) Has been skipped
Details
Mirror to Codeberg / mirror (push) Successful in 54s
Details
C++ Tests / test-cgal (push) Has been skipped
Details

Browse Source
This commit is contained in:
user2595
2026-05-31 08:21:54 +00:00
parent d725166c6a b213ad2265
commit e13e7ea8e7
8 changed files with 2311 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

420
doc/reviewer/api-performance-audit-2026-05-31.md Normal file
View File

@@ -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 `_<geom>` 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_<geom>_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_<geom>_<scope>_dof_indices`, `assign_<scope>_dof_indices`
(no geometry), and `assign_all_<geom>_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_<geom>_<scope>_dof_indices` (matches the already-consistent
`setup_<geom>_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_<geom>_<scope>_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 `_<geom>` 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_<geom>`.
---
## 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<FT>`
- `Discrete_inversive_distance.h:133,146` → returns `Conformal_map_result<FT>`
### 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<Eigen::SparseMatrix<double>> 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<double> G(n)` and `std::vector<double> 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_<geom>_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.

473
doc/reviewer/cgal-submission-readiness-audit-2026-05-31.md Normal file
View File

@@ -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/<Pkg>/`, 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/<Pkg>/Concepts/` file |
| G5 | 🔴 | **No user manual** — CGAL requires `<Pkg>.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/<Pkg>/ 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/<Pkg>/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/<Pkg>/<Pkg>.txt` — the user manual with `\mainpage`-style narrative,
`\cgalExample` references, figures.
- `doc/<Pkg>/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<TriangleMesh, Kernel_>` 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/<Pkg>/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.

167
doc/reviewer/dependency-license-audit-2026-05-31.md Normal file
View File

@@ -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.

197
doc/reviewer/input-validation-audit-2026-05-31.md Normal file
View File

@@ -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<bool>(); // ← 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<bool>()` 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("<Solver")` etc. and
assumes each element (and the `<DOFVector>…</DOFVector>` 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.

192
doc/reviewer/math-derivation-citation-audit-2026-05-31.md Normal file
View File

@@ -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.

234
doc/reviewer/numerical-stability-audit-2026-05-31.md Normal file
View File

@@ -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.

489
doc/reviewer/test-coverage-error-handling-audit-2026-05-31.md Normal file
View File

@@ -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<double>& 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<double>& 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.

139
doc/reviewer/thread-safety-audit-2026-05-31.md Normal file
View File

@@ -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.