f25174ed69
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 <noreply@anthropic.com>
154 lines
5.5 KiB
Markdown
154 lines
5.5 KiB
Markdown
# 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.
|