From d9c1dec3decd765d444739f45f34de6b9d7ae9a8 Mon Sep 17 00:00:00 2001 From: Tarik Moussa Date: Sun, 24 May 2026 21:15:19 +0200 Subject: [PATCH] feat: output_uv_map for InversiveDistance, error for CP-Euclidean, reviewer trio MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three reviewer-meeting deliverables in one commit. (1) output_uv_map for the two remaining DCE entries ───────────────────────────────────────────────── * Discrete_inversive_distance.h: full implementation. After Newton, reconstruct effective Euclidean edge lengths from the converged log-radii via the Bowers-Stephenson identity `ℓᵢⱼ² = rᵢ² + rⱼ² + 2·Iᵢⱼ·rᵢ·rⱼ`, populate a temporary EuclideanMaps with `lambda0 = log(ℓᵢⱼ²)`, and reuse the existing `euclidean_layout(mesh, 0, eucl)` priority-BFS. Per-vertex Point_2 coordinates written into the user-supplied pmap. Optional `normalise_layout(true)` applies the canonical PCA centroid + major-axis rotation, same as the other 3 entries. * Discrete_circle_packing.h: throws std::runtime_error with a clear pointer to Phase 9c rather than silently producing nonsense. CP-Euclidean is face-based; the faithful output is a per-face circle packing in ℝ², not a per-vertex Point_2 map. A true layout requires BPS-2010 §6 (~150 lines, on the porting roadmap as Phase 9c). Failing loudly is the honest default. Tests: 2 new cases in test_cgal_phase8b_lite.cpp (OutputUvMap_InversiveDistance_PopulatesPmap; OutputUvMap_CPEuclidean_ThrowsClearly). Both green. Suite total now 259 (was 257, +2). CGAL subtotal: 234 → 236. (2) Reviewer meeting documents ────────────────────────── New directory doc/reviewer/ with three files: * briefing.md — one-page orientation for the reviewer. What the project is, where to look first (https://tmoussa.codeberg.page/ConformalLabpp/), the headline evidence (tests/coverage/sanitizers/license), what we want from them, what's deferred and why, and the 5 questions in a separate file. * questions.md — the 5 concrete decisions we want their second opinion on: Q1 Phase 9c (port-literal vs re-derive) Q2 Phase 9b-analytic (worth ~2 weeks for ~6× speedup?) Q3 CP-Euclidean output_uv_map (build now or defer?) Q4 CGAL submission strategy (one package or five?) Q5 geometry-central cross-validation co-authorship Plus an explicit "what would you say no to?" question at the bottom — negative feedback is the highest-value information. * agenda.md — my own internal playbook (NOT to be sent). 60-min flow: 5-min thank-you, 10-min architecture tour, 30-min for Q1-Q5 in the order Q4-Q1-Q2-Q5-Q3, 5-min "no" question, 5-min wrap-up. Includes post-meeting memo template to fill out in the 30 min after. * README.md — index for the directory; says which file goes to whom and when to send. (3) locked-vs-flexible.md known-limitations update ───────────────────────────────────────────── "output_uv_map covers 3 of 5 entries" → "covers 4 of 5". CP-Euclidean's throws-clearly behaviour documented as a Phase 9c deliverable rather than a passive gap. Bonus: extended .codespellrc ignore list (acknowledgement, the British-English spelling I used in agenda.md). Verifications on this commit: 259/259 tests pass (0 skipped) scripts/check-test-counts.sh: OK (23 + 236 = 259) scripts/quality/license-headers.sh: OK (66/66 SPDX) python3 scripts/quality/cgal-conventions.py: OK (0/6 violations) scripts/quality/codespell.sh: OK (0 typos) scripts/quality/shellcheck.sh: OK (0 findings) python3 scripts/check-markdown-links.py: OK (143/143) Co-Authored-By: Claude Opus 4.7 --- .codespellrc | 1 + code/include/CGAL/Discrete_circle_packing.h | 41 +++++ .../CGAL/Discrete_inversive_distance.h | 48 ++++++ code/tests/cgal/test_cgal_phase8b_lite.cpp | 54 +++++++ doc/api/tests.md | 4 +- doc/architecture/locked-vs-flexible.md | 2 +- doc/reviewer/README.md | 28 ++++ doc/reviewer/agenda.md | 114 +++++++++++++ doc/reviewer/briefing.md | 112 +++++++++++++ doc/reviewer/questions.md | 153 ++++++++++++++++++ 10 files changed, 554 insertions(+), 3 deletions(-) create mode 100644 doc/reviewer/README.md create mode 100644 doc/reviewer/agenda.md create mode 100644 doc/reviewer/briefing.md create mode 100644 doc/reviewer/questions.md diff --git a/.codespellrc b/.codespellrc index 0269020..b53ba42 100644 --- a/.codespellrc +++ b/.codespellrc @@ -69,6 +69,7 @@ ignore-words-list = bessel,ist,sinces,nd,te,inout,nin,numer,neet,anc,sinks,doubl iff, categorise,categorised,categorises,categorising, optimisation,optimisations, + acknowledgement,acknowledgements,acknowledging, neighbour,neighbours,neighbouring,neighboured, labelled,labelling,labels,labelled, fulfil,fulfils,fulfilled,fulfilling, diff --git a/code/include/CGAL/Discrete_circle_packing.h b/code/include/CGAL/Discrete_circle_packing.h index 175975d..e2ca4a6 100644 --- a/code/include/CGAL/Discrete_circle_packing.h +++ b/code/include/CGAL/Discrete_circle_packing.h @@ -34,6 +34,8 @@ class (Strategy C of the Phase 8b architecture audit). #include "../cp_euclidean_functional.hpp" #include "../newton_solver.hpp" +#include + namespace CGAL { // ── Default traits for CP-Euclidean ─────────────────────────────────────────── @@ -158,6 +160,45 @@ auto discrete_circle_packing_euclidean( result.iterations = nr.iterations; result.gradient_norm = nr.grad_inf_norm; result.converged = nr.converged; + + // ── output_uv_map (Phase 8b-Lite extension) ──────────────────────────── + // + // The CP-Euclidean functional carries one DOF per *face* (the log of the + // face-circle radius `ρ_f = log R_f`), not per vertex. A faithful + // layout therefore produces a circle packing in ℝ² — each face f is + // mapped to a circle of radius `R_f` at some centre `c_f`, with + // adjacent circles meeting at the prescribed intersection angle `θ_e`. + // That is a per-face output, not the per-vertex Point_2 that + // `output_uv_map` is typed for. + // + // For Phase 8b-Lite we deliberately don't fake it. If the caller + // supplies `output_uv_map(pmap)` we throw `std::runtime_error` with a + // clear pointer to Phase 9c (BPS-2010 §6 face-based circle-packing + // layout, ~150 lines, on the porting roadmap). Failing loudly is + // better than silently writing zeros. + // + // Users who want a UV-like coordinate today can: + // 1. Solve a Euclidean DCE on the same mesh (vertex DOFs), + // 2. Use `discrete_inversive_distance_map(... output_uv_map(pmap))`, + // 3. Or compute face-centre positions by hand from `result.rho_per_face` + // + the per-edge `θ_e` values, plus a priority-BFS of their own. + { + auto uv_param = parameters::get_parameter( + np, Conformal_map::internal_np::output_uv_map); + constexpr bool has_uv = !std::is_same_v< + decltype(uv_param), internal_np::Param_not_found>; + if constexpr (has_uv) { + throw std::runtime_error( + "CGAL::discrete_circle_packing_euclidean: the " + "`output_uv_map(...)` named parameter is not yet supported " + "for face-based CP-Euclidean. The faithful output is a " + "circle packing in the plane (per-face), not per-vertex " + "UVs. Tracked as Phase 9c; " + "see doc/architecture/locked-vs-flexible.md and " + "doc/tutorials/add-output-uv-map.md."); + } + } + return result; } diff --git a/code/include/CGAL/Discrete_inversive_distance.h b/code/include/CGAL/Discrete_inversive_distance.h index 341b346..30eb28b 100644 --- a/code/include/CGAL/Discrete_inversive_distance.h +++ b/code/include/CGAL/Discrete_inversive_distance.h @@ -183,6 +183,54 @@ auto discrete_inversive_distance_map( result.iterations = nr.iterations; result.gradient_norm = nr.grad_inf_norm; result.converged = nr.converged; + + // ── Optional layout step (Phase 8b-Lite extension) ───────────────────── + // + // If the caller supplied `output_uv_map(pmap)`, lay out the converged + // packing in ℝ² and write per-vertex `Point_2` coordinates into `pmap`. + // + // Method: the converged Inversive-Distance radii `r_i = exp(u_i)` + // together with the fixed per-edge `I_ij` constants determine effective + // Euclidean edge lengths via the Bowers-Stephenson identity + // ℓᵢⱼ² = rᵢ² + rⱼ² + 2·Iᵢⱼ·rᵢ·rⱼ + // so we can populate a temporary `EuclideanMaps` whose `lambda0` carries + // `log(ℓᵢⱼ²)` per edge and then reuse `euclidean_layout(mesh, 0, eucl)` + // — the existing priority-BFS trilateration on the resulting triangle + // metric. All vertex/edge DOF indices stay at −1 (pinned), so the empty + // DOF vector `0` produces lengths driven purely by `lambda0`. + auto uv_param = parameters::get_parameter( + np, Conformal_map::internal_np::output_uv_map); + constexpr bool has_uv = !std::is_same_v< + decltype(uv_param), internal_np::Param_not_found>; + if constexpr (has_uv) { + if (nr.converged) { + auto eucl = ::conformallab::setup_euclidean_maps(mesh); + for (auto e : mesh.edges()) { + auto h = mesh.halfedge(e); + const double u_i = result.u_per_vertex[mesh.source(h).idx()]; + const double u_j = result.u_per_vertex[mesh.target(h).idx()]; + const double I = maps.I_e[e]; + const double l2 = ::conformallab::id_detail::edge_length_squared(u_i, u_j, I); + eucl.lambda0[e] = (l2 > 0.0) ? std::log(l2) : -30.0; + } + // Empty DOF vector: every vertex is pinned (idx=-1), so the + // layout depends purely on the lambda0 we just computed. + std::vector zero; + auto layout = ::conformallab::euclidean_layout(mesh, zero, eucl); + + const bool do_norm = parameters::choose_parameter( + parameters::get_parameter(np, Conformal_map::internal_np::normalise_layout), + false); + if (do_norm) ::conformallab::normalise_euclidean(layout); + + for (auto v : mesh.vertices()) { + const auto& uv = layout.uv[v.idx()]; + put(uv_param, v, + typename Traits::Kernel::Point_2(uv.x(), uv.y())); + } + } + } + return result; } diff --git a/code/tests/cgal/test_cgal_phase8b_lite.cpp b/code/tests/cgal/test_cgal_phase8b_lite.cpp index 9a78db7..9fe24ea 100644 --- a/code/tests/cgal/test_cgal_phase8b_lite.cpp +++ b/code/tests/cgal/test_cgal_phase8b_lite.cpp @@ -288,6 +288,60 @@ TEST(CGALPhase8bLite, OutputUvMap_HyperIdeal_PointsInPoincareDisk) // No assertion needed; this is documented behaviour.) } +TEST(CGALPhase8bLite, OutputUvMap_InversiveDistance_PopulatesPmap) +{ + // Inversive-Distance: per-vertex u_i = log r_i. With output_uv_map + // the entry function reconstructs effective Euclidean edge lengths via + // the Bowers-Stephenson identity and reuses the euclidean_layout + // priority-BFS to populate per-vertex Point_2 coordinates. + using K = CGAL::Simple_cartesian; + auto mesh = make_quad_strip(); + + auto uv_map = mesh.add_property_map( + "v:test_uv_id", K::Point_2(0, 0)).first; + + auto res = CGAL::discrete_inversive_distance_map( + mesh, CGAL::parameters::output_uv_map(uv_map)); + + ASSERT_TRUE(res.converged) << "ID Newton did not converge on quad_strip"; + EXPECT_EQ(res.u_per_vertex.size(), num_vertices(mesh)); + // Every UV must be finite; not all zero. + bool any_nonzero = false; + for (auto v : mesh.vertices()) { + const auto& p = uv_map[v]; + ASSERT_TRUE(std::isfinite(p.x())); + ASSERT_TRUE(std::isfinite(p.y())); + if (std::abs(p.x()) > 1e-9 || std::abs(p.y()) > 1e-9) any_nonzero = true; + } + EXPECT_TRUE(any_nonzero) << "all UVs are zero — layout did not run"; +} + +TEST(CGALPhase8bLite, OutputUvMap_CPEuclidean_ThrowsClearly) +{ + // CP-Euclidean is face-based; its natural layout is a per-face + // circle packing in ℝ², not a per-vertex Point_2 map. The entry + // throws std::runtime_error with a helpful message rather than + // silently producing nonsense. See doc/architecture/locked-vs-flexible.md. + using K = CGAL::Simple_cartesian; + auto mesh = make_quad_strip(); + + auto uv_map = mesh.add_property_map( + "v:test_uv_cp", K::Point_2(0, 0)).first; + + EXPECT_THROW( + CGAL::discrete_circle_packing_euclidean( + mesh, CGAL::parameters::output_uv_map(uv_map)), + std::runtime_error) + << "expected discrete_circle_packing_euclidean to reject " + "`output_uv_map(...)` (face-based DOF, Phase 9c)."; + + // Sanity: without output_uv_map the entry function still works fine. + auto res = CGAL::discrete_circle_packing_euclidean(mesh); + // Convergence depends on the mesh; we only check no-throw + a sane + // shape of the result struct. + EXPECT_EQ(res.rho_per_face.size(), num_faces(mesh)); +} + TEST(CGALPhase8bLite, OutputUvMap_Absent_DoesNotRunLayout) { // Sanity: without the parameter, no layout work happens. Verified diff --git a/doc/api/tests.md b/doc/api/tests.md index d97c84d..ce63251 100644 --- a/doc/api/tests.md +++ b/doc/api/tests.md @@ -64,9 +64,9 @@ All tests have CTest prefix `cgal.` (set via `TEST_PREFIX "cgal."` in CMakeLists | `InversiveDistanceFunctional` | `test_inversive_distance_functional.cpp` | 11 | Phase 9a.2 — Luo-2004 vertex-based packing (from literature) | | `HyperIdealHessian` | `test_hyper_ideal_hessian.cpp` | 7 | Phase 9b — block-FD vs full-FD cross-validation + PSD + speed-up | | `NewtonPhase9a` | `test_newton_phase9a.cpp` | 7 | Phase 9a-Newton — convergence for the two new circle-packing solvers | -| `CGALPhase8bLite` | `test_cgal_phase8b_lite.cpp` | 15 | Phase 8b-Lite — CGAL entries for all 5 DCE models + `output_uv_map` + pipe-operator chaining | +| `CGALPhase8bLite` | `test_cgal_phase8b_lite.cpp` | 17 | Phase 8b-Lite — CGAL entries for all 5 DCE models + `output_uv_map` (Euclidean, Spherical, HyperIdeal, Inversive-Distance) + CP-Euclidean throws-clearly + pipe-operator chaining | -**Total: 234 tests, 0 skipped.** +**Total: 236 tests, 0 skipped.** --- diff --git a/doc/architecture/locked-vs-flexible.md b/doc/architecture/locked-vs-flexible.md index a52a8fa..93558f6 100644 --- a/doc/architecture/locked-vs-flexible.md +++ b/doc/architecture/locked-vs-flexible.md @@ -258,7 +258,7 @@ mechanical next step rather than a missing piece of theory. | Limitation | Status | Effort to close | |---|---|---| -| **`output_uv_map` covers 3 of 5 entries** — Euclidean, Spherical, HyperIdeal are wired to call the appropriate `*_layout()` after Newton. CP-Euclidean (face-based) and Inversive-Distance (vertex-based) entries still require the user to call `*_layout()` by hand. | tutorial + plumbing in place; just needs to be copy-pasted into the two remaining entry headers | ~0.5 day | +| **`output_uv_map` covers 4 of 5 entries** — Euclidean, Spherical, HyperIdeal, **and Inversive-Distance** (new on the structural-tests branch) are wired to call the appropriate `*_layout()` after Newton. CP-Euclidean is face-based: the faithful output is a per-face circle packing, not a per-vertex `Point_2`, so the entry deliberately throws `std::runtime_error` with a helpful message rather than silently producing nonsense. Implementing a true CP-Euclidean circle-packing layout (BPS-2010 §6, ~150 lines) is tracked as Phase 9c. | Inversive-Distance via Bowers-Stephenson edge-length reconstruction + euclidean_layout reuse; CP-Euclidean deferred to Phase 9c | ID closed; CP-Euclidean ~3 days (genuinely new algorithm, not just plumbing) | | **Named-parameter chaining: `\|`-operator only, no `.a().b().c()`.** Member-style chaining would need a patch to CGAL's upstream `parameters_interface.h`, which we treat as a read-only vendored dependency. The pipe operator is documented, ADL-discoverable, and equivalent in expressive power. | pipe shipped, member-chain deferred until upstream extension point exists | ~2 days (only if upstream PR is accepted) | | **Phase 9b-analytic: derivation complete, code uses block-FD.** The Schläfli-based analytic HyperIdeal Hessian is fully derived in [`hyperideal-hessian-derivation.md`](../math/hyperideal-hessian-derivation.md) (805 lines, all sign pitfalls covered). The shipped code still uses per-face block-FD (already 96× faster than the legacy full-FD path). | research-ready writeup; implementation gated on the reviewer's view of whether the additional ~6× is worth it | ~2 weeks | | **Doxygen `WARN_IF_UNDOCUMENTED = NO`.** With `EXTRACT_ALL = YES`, every symbol is in the generated HTML — live at (auto-published from `main` by `.gitea/workflows/doxygen-pages.yml`) — but symbols without explicit doc comments show only their signature. Public API surface (entry functions, named-parameter helpers, traits typedefs) has hand-written Doxygen; internal helpers vary. | clean (0 warnings) under current policy; not yet enforced "no undocumented symbol"; pursued on a separate branch | ~3 days to drive `WARN_IF_UNDOCUMENTED = YES` to zero | diff --git a/doc/reviewer/README.md b/doc/reviewer/README.md new file mode 100644 index 0000000..47eab9c --- /dev/null +++ b/doc/reviewer/README.md @@ -0,0 +1,28 @@ +# Reviewer meeting materials + +A single landing page for the three documents associated with the +external-reviewer meeting (Springborn/Bobenko alumnus, May 2026). + +| Document | Audience | Purpose | +|---|---|---| +| [`briefing.md`](briefing.md) | **the reviewer** | one-page orientation: what the project is, where to look first, what we want from them | +| [`questions.md`](questions.md) | **the reviewer** | the 5 concrete decisions we want their opinion on (skim before the meeting) | +| [`agenda.md`](agenda.md) | **me** | my own meeting playbook: timing, order, the "no" question, post-meeting memo template | + +After the meeting, a fourth file `2026-XX-XX-meeting-notes.md` should +land here too — the memo template at the bottom of `agenda.md` is the +suggested structure. + +## When to send to the reviewer + +- **briefing.md** + **questions.md**: as part of the meeting-confirmation + email, ~5 business days before the meeting. Subject line: *"Pre-read + for our conformallab++ chat (~15 min)"*. +- **agenda.md**: never send. This is internal scaffolding. + +## Quick links the reviewer should bookmark + +- 🌐 Reviewer-hub landing page: +- 📦 Source: +- 📝 Architecture decisions: [`../architecture/locked-vs-flexible.md`](../architecture/locked-vs-flexible.md) +- 📐 Schläfli derivation (for Q2): [`../math/hyperideal-hessian-derivation.md`](../math/hyperideal-hessian-derivation.md) diff --git a/doc/reviewer/agenda.md b/doc/reviewer/agenda.md new file mode 100644 index 0000000..a7bc60f --- /dev/null +++ b/doc/reviewer/agenda.md @@ -0,0 +1,114 @@ +# Reviewer meeting — agenda (for me) + +> **Audience.** Myself, before the meeting. Not shared with the +> reviewer (they have [`briefing.md`](briefing.md) and +> [`questions.md`](questions.md) instead). +> +> **Purpose.** Keep the conversation on the high-stakes items +> without slipping into detail-tour mode. + +**Length:** ~60 min (default; willing to extend if conversation is +productive). + +## Opening — 5 min + +- **Thank-you** + ~30 s on why I value their time specifically + (Springborn/Bobenko background = best-positioned audience for + this material). +- **Confirm format**: they've read `briefing.md`? Yes → skip ahead. + No → take 3 min to walk through it together via the reviewer-hub + URL. + +## §1 Architectural tour — 10 min + +Goal: they leave §1 with a mental map of what's where. + +1. Open . +2. Click through: + - Doxygen → `discrete_conformal_map_euclidean` (the canonical entry, + shows the named-parameter pattern + traits). + - `locked-vs-flexible.md` § Summary table. + - `dependencies.md` § TL;DR. +3. **Don't** demo running the test suite live unless they ask — + they can verify the green badges later. + +**Cue to move on**: when they have no more "where is X?" questions. + +## §2 The five questions — 30 min + +Goal: get clear answers to as many of the 5 questions in +[`questions.md`](questions.md) as possible. Order matters: + +1. **Q4 first** (CGAL submission strategy — single package vs five). + This sets the framing for the rest. ~5 min. +2. **Q1** (Phase 9c port-literal vs re-derive). ~5 min. +3. **Q2** (Phase 9b-analytic — is the 6× speedup worth ~2 weeks?). + May trigger longer math discussion; cap at 10 min and offer to + continue async. +4. **Q5** (GC-1 cross-validation co-authorship). ~5 min. +5. **Q3** (CP-Euclidean output_uv_map). ~5 min. + +**Trap to avoid**: don't relitigate any of the already-made decisions +(e.g. Surface_mesh as default, Eigen as backend) unless they bring it +up under "anything you'd say no to". Move forward, not sideways. + +## §3 The "no" question — 5 min + +Last paragraph of [`questions.md`](questions.md): *"Is there one +architecture decision where you think 'no, that's the wrong call'?"* + +Ask explicitly. Wait. Don't fill silence. Negative feedback at this +phase is the highest-value information of the entire meeting. + +## §4 Wrap-up — 5 min + +- **Summarise**: 1 sentence per Q1–Q5 answer. +- **Next steps**: + - Items they offered to help on (if any) → I'll send a follow-up + email within 48 h. + - Items they flagged as worth changing → I'll write a one-page + plan + send for sign-off before starting work. +- **Cadence**: do they want a periodic update? Once-a-quarter? When + hitting Phase 9c milestone? +- **Citation**: ask whether they want acknowledgement in the + CGAL submission's `THANKS.md`. + +## Material I have ready (in tabs) + +1. +2. (source) +3. Local terminal in the repo (for any "show me X" requests) +4. `doc/math/hyperideal-hessian-derivation.md` open in editor + (for Q2) +5. `doc/architecture/locked-vs-flexible.md` open at "Summary table" +6. This file, [`questions.md`](questions.md), and + [`briefing.md`](briefing.md) for backstop. + +## After the meeting + +- 30-min decompression — don't take notes during the meeting beyond + bullet-tracking of Q1–Q5 answers; write up a full memo in the 30 min + AFTER. +- Memo template: + ``` + Date: ____ + Reviewer: ____ + Duration: ____ min + + Q1 answer: ____ + Q2 answer: ____ + Q3 answer: ____ + Q4 answer: ____ + Q5 answer: ____ + "No" pointer: ____ + + Action items: + [ ] (within 48 h) ____ + [ ] (within 2 wks) ____ + [ ] (longer term) ____ + + Cadence agreed: ____ + ``` + +- Store the memo at `doc/reviewer/2026-XX-XX-meeting-notes.md` + (gitignored if confidential, otherwise committed). diff --git a/doc/reviewer/briefing.md b/doc/reviewer/briefing.md new file mode 100644 index 0000000..df00bf8 --- /dev/null +++ b/doc/reviewer/briefing.md @@ -0,0 +1,112 @@ +# Reviewer briefing — conformallab++ + +> **Audience.** External reviewer with a discrete-conformal-geometry +> background (Springborn / Bobenko alumnus, ~30 minutes lead-time +> before the meeting). +> +> **Purpose of this document.** A single page they can read once +> before the meeting and feel oriented — what the project is, what +> we want from them, and where to look first. + +## In one paragraph + +`conformallab++` is a C++17 header-only re-implementation of the Java +library [`ConformalLab`](https://github.com/varylab/conformallab) +(Sechelmann 2016, TU Berlin), built around CGAL's `Surface_mesh` and +Eigen. v0.9.0 ships five Discrete Conformal Equivalence (DCE) solvers +— Euclidean, Spherical, HyperIdeal, Circle-Packing Euclidean +(BPS 2010, face-based), Inversive-Distance (Luo 2004, vertex-based) — +plus the Newton infrastructure, layout (priority-BFS trilateration in +ℝ², S², Poincaré disk), Möbius holonomy, period matrix, cut graph, and +JSON/XML serialisation. Long-term goal: a CGAL package. + +## Where to start (one URL, 5 minutes) + +**👉 https://tmoussa.codeberg.page/ConformalLabpp/** + +The landing page is a hand-curated reviewer hub, not an auto-generated +index. It links to the Doxygen API, the key markdown documents, and +shows static quality-gate status. + +## What's true about this snapshot + +| Claim | Concrete evidence | +|---|---| +| **Library is header-only and standalone** | Verification recipe in `doc/architecture/dependencies.md`: `env -i PATH=… cmake … && ctest` passes with zero quality tools installed. | +| **Tests: 259 pass, 0 skipped** | `bash scripts/check-test-counts.sh` enforces this against `doc/api/tests.md`; CI fails on drift. | +| **Doxygen: 100 % public-API coverage, 0 warnings** | `bash scripts/doxygen-coverage.sh --threshold 100` is in CI. | +| **License hygiene: 66/66 files carry MIT SPDX** | `bash scripts/quality/license-headers.sh` is in CI (strict). | +| **Build reproducibility: byte-identical between runs** | `bash scripts/quality/reproducible-build.sh` (local, ~6 min). | +| **Sanitizers (ASan + UBSan) clean on fast suite** | `bash scripts/quality/sanitizers.sh` (local, ~3 min). | +| **CGAL conventions: 6 rules, 0 violations** | `python3 scripts/quality/cgal-conventions.py` (CI required). | + +## What we want from you + +Five concrete questions are in +[`doc/reviewer/questions.md`](questions.md) — please skim them +beforehand. They are deliberately scoped: each can be answered with +"go this way" / "no, go that way" / "either is fine". + +The high-stakes ones: + +1. **Phase 9c (4g-polygon)** — port the Java implementation literally, + or re-derive from Springborn 2020 §5? +2. **Phase 9b-analytic** — is the ~6× speedup over our current block-FD + Hessian (full Schläfli-based analytic) worth ~2 weeks? Working-mesh + size that justifies it? +3. **CGAL submission strategy** — submit as a single package or split + the five DCE solvers into separate packages? + +## What's deliberately deferred (so we can discuss with you first) + +| Item | Why deferred | +|---|---| +| Phase 9b-analytic (Schläfli-based HyperIdeal Hessian) | derivation done (805-line LaTeX doc), implementation depends on your opinion of payoff | +| Phase 9c (fundamental-polygon utility, 4g-polygon canonical form) | algorithm choice up to you | +| Cross-validation against geometry-central (GC-1) | potential paper, scope depends on your interest | +| CP-Euclidean `output_uv_map` (per-face circle packing) | needs the BPS-2010 §6 layout algorithm, ~3 days | +| `.a().b().c()` member-style named-parameter chaining | requires patching CGAL upstream; pipe-operator (`a | b | c`) shipped instead | + +These are all flagged in +[`doc/architecture/locked-vs-flexible.md`](../architecture/locked-vs-flexible.md) +§"Known limitations". + +## Architectural decisions you might want to challenge + +12 decisions classified 🔴 load-bearing / 🟡 semi-fixed / 🟢 opportunistic +in [`doc/architecture/locked-vs-flexible.md`](../architecture/locked-vs-flexible.md). +The ones most worth your time: + +- **#1 Surface_mesh as default** — 🔴 ~3 weeks to change. Are you OK + with this default, or should we wire Polyhedron_3 / OpenMesh now? +- **#6 Eigen as linear-algebra back-end** — 🔴 ~2 weeks to change. Are + the Eigen sparse solvers (SparseCholesky + SparseQR fallback) + sufficient for the mesh sizes you've seen, or should we look at + CHOLMOD / PETSc? +- **#7 Strategy C** (one Default trait per functional, not a unified + trait) — 🟡 ~1 week to refactor. CGAL convention agrees; do you? + +## How to actually run something + +```bash +git clone https://codeberg.org/TMoussa/ConformalLabpp && cd ConformalLabpp +cmake -S code -B build && cmake --build build --target conformallab_tests +ctest --test-dir build # ~2 s, 23 pure-math tests +# CGAL tests (adds Boost as a system dep): +cmake -S code -B build -DWITH_CGAL_TESTS=ON +cmake --build build --target conformallab_cgal_tests -j +ctest --test-dir build # ~3 min, 236 CGAL tests +``` + +A more end-to-end recipe lives in `scripts/try_it.sh` (also run in CI). + +## Meeting logistics + +- **Format**: video call (you suggested), ~60 min +- **Materials needed on your side**: just a browser to follow the + reviewer-hub URL. +- **Materials I'll have ready**: a screen-share-able terminal with + the repo open, my own agenda in `doc/reviewer/agenda.md`, and the + questions doc above. + +Looking forward. diff --git a/doc/reviewer/questions.md b/doc/reviewer/questions.md new file mode 100644 index 0000000..fb0d4d9 --- /dev/null +++ b/doc/reviewer/questions.md @@ -0,0 +1,153 @@ +# Questions for the external reviewer + +> Please skim this before the meeting. Each question is scoped so that +> "do A" / "do B" / "either is fine" is a sufficient answer; deeper +> dives are welcome but not required. + +These are the five decisions that would benefit most from a second +opinion. Items 1-3 affect the porting roadmap directly; 4-5 affect +the long-term goal (CGAL submission). + +--- + +## Q1 — Phase 9c (4g-polygon canonical form): port-literal vs re-derive? + +The Java original has two utility classes: + +- `FundamentalPolygonUtility` (~600 lines) +- `CanonicalFormUtility` (~900 lines) + +that together compute the canonical 4g-polygon for a higher-genus +surface from its cut graph. + +We can either: + +- **(A) Port literally** (~2 weeks). Faithful, predictable, fixes + the Java algorithm in C++. Down-side: we inherit the Java code's + ad-hoc style and edge-case handling. +- **(B) Re-derive from Springborn 2020 §5** (~3 weeks). Uses our + existing `cut_graph.hpp` + holonomy infrastructure cleanly. + Down-side: longer; potential for new bugs not seen by the Java + original's test cases. + +**Question**: which route do you prefer, and is there a Springborn-era +reference implementation (Mathematica notebook, paper appendix) we +should cross-validate against? + +Context: `doc/roadmap/phases.md` §Phase 9c, `doc/roadmap/porting-status.md`. + +--- + +## Q2 — Phase 9b-analytic Hessian: implement now or later? + +We have: + +- **Done**: per-face block-FD Hessian (96× faster than naive full-FD). +- **Derived but not implemented**: analytic Hessian via the Schläfli + identity, expected ~6× further speedup over block-FD. +- **Derivation document**: + [`doc/math/hyperideal-hessian-derivation.md`](../math/hyperideal-hessian-derivation.md) + (805 lines, all sign pitfalls covered, references Schläfli 1858, + Milnor 1982, Vinberg 1993, Cho-Kim 1999, Glickenstein 2011, + Springborn 2020). + +**Question**: + +- At what mesh size does the ~6× become user-visible enough to + justify ~2 weeks of implementation work? +- Are you aware of subtleties in the Schläfli-based derivation our + document might be missing? + +If the answer is "implement", we'd target Phase 9b-analytic right +after the meeting. + +--- + +## Q3 — `output_uv_map` for CP-Euclidean: build now or defer? + +For CP-Euclidean (face-based DOFs, BPS 2010) the natural output is a +**per-face circle packing** in ℝ² — not a per-vertex `Point_2` map +the way the other four DCE entries produce. + +Today the entry throws `std::runtime_error` with a helpful pointer if +the caller passes `output_uv_map(...)`. A faithful layout would be +~150 lines implementing BPS-2010 §6. + +**Question**: do *your* CP-Euclidean use cases need a UV-like output, +or are the face circle radii themselves the deliverable? The answer +shapes whether Phase 9c gets it now or later. + +--- + +## Q4 — CGAL submission strategy: one package or five? + +For the long-term CGAL submission: + +- **(A) One package** "Discrete_conformal_map" — single entry header, + five solver functions, one set of named parameters. Easier for + users to find; harder to compartmentalise reviews. +- **(B) Five packages** "Discrete_*" — each DCE model is its own + CGAL package with its own concept + reference manual. More + ceremony for users; more familiar review surface for CGAL editors. + +Today the code is structured per-functional (Strategy C — see +[`locked-vs-flexible.md`](../architecture/locked-vs-flexible.md) §7). +Either submission packaging is achievable from this base. + +**Question**: what's the CGAL editor convention for related-but-distinct +algorithms — Polygon_mesh_processing as one example (one package, many +algorithms), vs Triangulation_2/Triangulation_3/Periodic_*/Hyperbolic_* +as another (multiple packages for related algorithms)? + +--- + +## Q5 — geometry-central cross-validation (GC-1) + +Two libraries solve the DCE problem from opposite algorithmic +directions: + +- **conformallab++**: Newton on the fixed mesh (no intrinsic flips). +- **geometry-central**: Ptolemy flips on an intrinsic triangulation. + +An automated comparison on common meshes would be: + +- A nice paper (the disagreement modes are interesting in their own + right). +- A confidence-building tool for both libraries (we benefit from + catching corner-case bugs, they benefit from cross-validation + against a Newton baseline). +- ~3 days of plumbing (CMake-fetch geometry-central, write 5 common + test meshes, compare `u_per_vertex` to a tolerance). + +**Question**: would you be interested in co-authoring such a comparison +note (with us doing the implementation)? Or do you know someone in +the Crane group who would? + +--- + +## Things you do *not* need to comment on (unless you want) + +- C++ style choices captured in `.clang-format` + `.clang-tidy`. +- Test framework choice (GTest). +- License (MIT, with vendored deps catalogued in + [`code/deps/THIRD-PARTY-LICENSES.md`](../../code/deps/THIRD-PARTY-LICENSES.md)). +- Build system (CMake ≥ 3.20, header-only consumer + optional + CLI/Viewer). + +These are conscious decisions matched to CGAL conventions and aren't +load-bearing in the sense that revisiting them later is cheap. + +--- + +## What I'm hoping you'll say "no" to + +This is a deliberately blunt question because positive feedback is +nice but negative feedback is rarer and more valuable: + +**Looking at any of the 12 architecture decisions in +[`locked-vs-flexible.md`](../architecture/locked-vs-flexible.md), is +there one where you think "no, that's the wrong call, here's why"?** + +The 🔴 load-bearing decisions are the most consequential to revisit, +because waiting longer makes them more expensive. No is the most +useful answer.