Files
ConformalLabpp/doc/architecture/locked-vs-flexible.md
Tarik Moussa e04515c423 docs(doxygen): fix critical extraction bug; baseline 24% → 42% on public API
ROOT CAUSE FIX
The Doxyfile EXCLUDE_PATTERNS line contained `*/* 2.hpp` (note the
space — a stray glob from macOS-style "foo 2.hpp" duplicate files).
That pattern was silently matching ALL .hpp / .h files, so Doxygen was
indexing nothing under code/include/.  The pre-existing 556 KB of HTML
output was effectively documenting only README.md, CLAUDE.md and a
small stub for std:: — not the C++ API at all.

After fixing the pattern (and properly escaping the space-prefixed
"foo 2.hpp / foo 2.h" macOS-dup patterns), Doxygen now extracts 141
compounds and emits 248 HTML pages from the public headers.

WHAT THIS PR ADDS
1. Doxyfile fix: correct EXCLUDE_PATTERNS; add GENERATE_XML for the
   coverage measurement script; add MathJax for `$$...$$` math in
   markdown; add the missing CGAL `\cgalParamNBegin/End/Description/
   Default/...` aliases so CGAL-style param blocks render correctly.

2. New headers:
   - code/include/CGAL/Conformal_map/doxygen_groups.h
     defines `PkgConformalMap{,Ref,Concepts,NamedParameters}`,
     resolving 17 prior "non-existing group" warnings.
   - code/include/CGAL/Conformal_map/doxygen_namespaces.h
     gives every namespace under `CGAL::` and `conformallab::` a
     brief description.

3. New tool: scripts/doxygen-coverage.sh
   Parses the XML output and reports % of public symbols (excluding
   the `detail::` implementation namespaces by default) that have a
   non-empty brief/detailed description.  Supports `--list-undoc`
   and `--threshold N` for CI integration.

4. Substantial docstring additions to the public CGAL headers:
   `Conformal_map_traits.h`, `Discrete_circle_packing.h`,
   `Discrete_inversive_distance.h`, `conformal_mesh.hpp`,
   `Discrete_conformal_map.h` (Hyper_ideal_map_result fields).

5. Markdown housekeeping that the strict-warning Doxygen run surfaced:
   tests.md (escape literal `#` in table cell),
   locked-vs-flexible.md (broken section anchor),
   overall_pipeline.md (replace `$$LaTeX$$` with inline-unicode math).

CURRENT NUMBERS

  before:  ~24%  documented (public API; the prior "87%" claim was
                 based on the broken extraction)
  after:    42%  documented   (165 of 396 public symbols)
  warnings: 0   (was 27 spurious + a flood of bogus undocumented
                  warnings hidden by the buggy EXCLUDE pattern)

NEXT (in a follow-up commit on this branch)
The remaining 231 public symbols (mostly in `layout.hpp`,
`hyper_ideal_functional.hpp`, `spherical_functional.hpp`, the per-mode
functional/Hessian files) can be brought to ~100% with another pass of
short `///` brief descriptions.  The coverage script is the gate; CI
can begin enforcing `--threshold 95` once the next pass lands.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2026-05-23 23:44:54 +02:00

18 KiB
Raw Blame History

Locked-in vs flexible architecture decisions

Purpose. External collaborators (especially mathematicians evaluating whether to extend the library) need to know which design choices are load-bearing (changing them is expensive across the whole codebase) and which are opportunistic (made on first principles and easy to revisit).

Why this matters now. A v0.9.0 + Springborn-Bobenko-alumnus external review (May 2026) is the right moment to surface these decisions before they ossify further. If any of the locked decisions need revisiting, this is the cheapest moment in the project's life to do so.

Three tiers:

🔴 LOAD-BEARING         changing requires repo-wide refactoring
🟡 SEMI-FIXED           changing affects multiple subsystems; possible but not casual
🟢 OPPORTUNISTIC        changing is one-PR work

1. Mesh data structure → CGAL::Surface_mesh<P>

Status 🔴 load-bearing
Locked since Phase 3 (2025)
Alternative considered OpenMesh / pmp-library / custom halfedge / Java CoHDS literal port
Why locked Every header code/include/*.hpp uses ConformalMesh = CGAL::Surface_mesh<Point3> and CGAL property maps explicitly. Phase 8a Traits provide a concept abstraction but the Default model is Surface_mesh-only.
Cost to change ~3 weeks: rewrite traits, every functional, every test mesh factory. Touched in ~30 headers + ~25 test files.
Mitigation Traits-based design (Phase 8a) means user-supplied mesh types CAN be added as Default-traits specialisations without touching algorithms — Phase 8a.2 plan documents this. But the Default model is Surface_mesh.
When to revisit If a major user wants Polyhedron_3 or OpenMesh as default. Currently no concrete request → keep.

Recommended posture for an external contributor: use the existing Surface_mesh-based API for any new functional. Adding generic-FaceGraph support is a single architectural step that doesn't need to be repeated per functional — wait for one user to need it.


2. Floating-point kernel → CGAL::Simple_cartesian<double>

Status 🟡 semi-fixed
Locked since Phase 3 (deliberate decision: conformal geometry doesn't need exact predicates)
Cost to change Per-functional template parameterisation. The Phase 8 MVP wrapper already deduces the kernel from the mesh point type, so user code can specify any CGAL kernel — but the legacy code/include/*.hpp headers are hardcoded.
When to revisit If a user reports floating-point catastrophic cancellation in the half-tangent angle formula on extreme meshes. Hasn't happened in 250 tests over 9 phases.

Recommended posture: stay with Simple_cartesian<double>. If a specific algorithm needs Exact_predicates_inexact_constructions_kernel for robustness, parameterise that one algorithm — don't refactor the whole codebase.


3. Header-only, no compiled library

Status 🔴 load-bearing
Locked since Phase 1
Cost to change Significant: would need to introduce .cpp files, link order, ABI compatibility decisions. But everything in code/include/*.hpp is inline or template, so a header-only-to-compiled migration is mechanical.
Why locked CGAL package convention is also header-only. Forking would break that goal.
When to revisit If compilation times become prohibitive (currently < 30 s clean build with all 5 functionals). Or if a future cyclic dependency between functionals forces it. Neither is on the horizon.

Recommended posture: stay header-only. This is also the de-facto norm for CGAL packages of comparable scope.


4. Five DCE models on the same mesh

Status 🟡 semi-fixed
Locked since Phase 9a (2026-05) — when CP-Euclidean introduced face-DOFs
Why semi-fixed Each model has its own *Maps bundle with its own property-map name prefix (ev:, sv:, v:, cf:/ce:, iv:/ie:) so all five can coexist on the same CGAL::Surface_mesh. Adding a sixth model means picking a new prefix + writing a new *Maps struct + Default trait.
Cost to add a sixth model ~1 week (CP-Euclidean took ~3 days, Inversive-Distance ~3 days, Hessian + Newton + CGAL entry add another ~3 days).
When to revisit If a unified base-Maps abstraction would actually win something (currently it would not — the property-map sets differ in kind, not just in name).

Recommended posture for adding a new functional:

  1. Pick a 2-letter prefix not in {ev, sv, v, cf, ce, iv, ie}.
  2. Define your *Maps struct with that prefix.
  3. Define your Default_*_traits<Surface_mesh, K> class in a new header code/include/CGAL/Discrete_*.h.
  4. Wire it into newton_solver.hpp (template-copy from newton_inversive_distance is the closest pattern for vertex-DOFs; newton_cp_euclidean for face-DOFs).
  5. Add a CGAL entry in the new header.
  6. Add tests following add-inversive-distance.md.

This recipe has been validated three times now (CP-Euclidean, Inversive Distance, and the four Phase-8b-Lite wrappers).


5. Newton solver with line search + SparseQR fallback

Status 🟡 semi-fixed
Locked since Phase 4
Cost to change Each newton_* function is ~50-80 lines; replacing the solver across all five is ~2 days.
When to revisit If a future functional needs trust-region or BFGS. None of the five currently does — Newton converges quadratically near the optimum and the line search handles bad initial points.

Recommended posture: Newton with line search is enough for any strictly-convex variational problem. For non-convex variants, consider adding a newton_with_trust_region() helper alongside, not replacing.


6. Eigen as the linear-algebra back-end

Status 🔴 load-bearing
Locked since Phase 4
Alternative considered PETSc/Tao (Java original) / Boost.uBLAS / Blaze
Why locked Eigen is header-only (no external dependency at build time), bundled as a CGAL dependency, fast, and offers SimplicialLDLT + SparseQR which the gauge-singular-mesh case needs.
Cost to change Significant — every Hessian header (*_hessian.hpp) and every Newton solver uses Eigen::SparseMatrix and Eigen::VectorXd directly. ~2 weeks repo-wide.
When to revisit If a sparse-solver feature (e.g. parallel Cholesky) is needed that Eigen doesn't offer.

Recommended posture: stay with Eigen.


7. CGAL public-API surface layout

Status 🟡 semi-fixed (Phase 8a-MVP design decision, 2026-05-19)
Locked since PR #6 (v0.9.0)
Strategy chosen "Strategy C" — functional-specific Default traits, one entry function per functional, no fat unified trait.
Cost to change to unified trait ~1 week — refactor Default_*_traits<> into a single Default_conformal_map_traits<> with all property-map fields. Existing 8 tests would need updating.
When to revisit When the first cross-functional algorithm (e.g. a hybrid functional that uses both face and vertex DOFs) lands. Speculation today.

Recommended posture: stay with Strategy C. CGAL's own Polygon_mesh_processing package follows the same convention — one default trait per algorithm family.


8. Named-parameter mechanism

Status 🟢 opportunistic
Locked since Phase 8 MVP (2026-05-19)
Current state Six tags: vertex_curvature_map, fixed_vertex_map, gradient_tolerance, max_iterations, output_uv_map, normalise_layout. Pipe-operator `
Cost to extend with .member() chaining ~2 days IF CGAL upstream is forked / patched; otherwise the pipe-operator workaround is the maintainable path.
When to revisit At any time; this is the lowest-risk change in the codebase. Tutorials add-output-uv-map.md §4 explains the mechanism.

Recommended posture: the pipe-operator | is already shipped and sufficient. Add .member() chaining only if a concrete user pushes for the CGAL-canonical syntax AND we are willing to fork CGAL upstream.


9. Property-map name conventions

Status 🟢 opportunistic
Locked since Phase 3 + 9a (prefix ev:/sv:/v:/cf:/ce:/iv:/ie: set when each functional was introduced)
Cost to change One sed-replace + recompile. No user-visible effect because the names are an internal convention; the CGAL public API never exposes them.
When to revisit If a future functional reuses an existing letter prefix. Already discussed in §4 "Five DCE models on the same mesh" above.

10. Tests: GTest, not CGAL's own test format

Status 🟡 semi-fixed
Locked since Phase 1
Cost to change ~1 week — rewrite test harnesses to CGAL's test/Conformal_map/ convention. This is Phase 8d (planned for CGAL submission).
Why GTest now Faster development cycle, IDE-friendly (Xcode / VSCode / CLion all have native GTest support). No CGAL submission is in progress yet.
When to revisit When committing to CGAL submission (Phase 8c-d, decided to be a future commitment, see release-policy.md).

Recommended posture: keep GTest as primary. When/if CGAL submission happens, add a test/Conformal_map/ shim that calls into the GTest suite — both formats can coexist.


11. License: MIT

Status 🔴 load-bearing
Locked since Project inception
Cost to change High organisational cost (requires consent of all contributors); ~no code cost.
Why locked MIT was chosen for academic friendliness (citing, modifying, embedding). CGAL upstream requires LGPL for submitted packages.
Trade-off Submitting to CGAL upstream is not possible without re-licensing. The codebase architecture is "CGAL-style" but the project would publish independently.
When to revisit If/when a concrete CGAL upstream submission is decided. See release-policy.md for the formal policy.

Recommended posture: stay with MIT. Build the "CGAL-style package for external distribution" as the primary deliverable. Re-license only when the CGAL editorial board commits to accepting the submission.


12. Documentation pattern: Markdown + Doxygen

Status 🟢 opportunistic
Locked since Phase 7.5 (2026-05)
Current state code/include/*.hpp carry Doxygen-style /// comments (87% coverage); doc/*.md for prose; Doxyfile generates HTML in doc/doxygen/.
Cost to add more Per-file basis; ~1 hour per header for full Doxygen.
When to revisit When chasing CGAL-submission readiness (need PackageDescription.txt + User_manual.md).

Recommended posture: keep adding /// comments incrementally with each new public function.


Summary table

Decision Tier Cost to change
1. CGAL::Surface_mesh as default mesh 🔴 load-bearing ~3 weeks
2. Simple_cartesian kernel 🟡 semi-fixed per-functional
3. Header-only architecture 🔴 load-bearing medium (mechanical)
4. Five DCE models, separate Maps 🟡 semi-fixed ~1 week per new model
5. Newton + line search + SparseQR 🟡 semi-fixed ~2 days
6. Eigen back-end 🔴 load-bearing ~2 weeks
7. Strategy C (per-functional traits) 🟡 semi-fixed ~1 week
8. Named-parameter mechanism 🟢 opportunistic pipe ; .member() ~2 days
9. Property-map name conventions 🟢 opportunistic ~1 hour
10. GTest, not CGAL test format 🟡 semi-fixed ~1 week
11. MIT license 🔴 load-bearing organisational
12. Markdown + Doxygen 🟢 opportunistic per-file

Key insight: the load-bearing decisions are all good in 2026. Surface_mesh + Eigen + header-only + MIT are the right defaults for a research-quality CGAL-style package. The semi-fixed decisions are all behind one concrete blocker (single user request, CGAL submission commitment, etc.). The opportunistic decisions are cheap to revisit any time.

The architecture is in a good place for the v0.9.0 → v0.10.0 transition. No "expensive corner" has been painted into; every locked decision matches the project's three-goal hierarchy in research-track.md.


Known limitations (state at the time of the reviewer meeting)

These are deliberate, honestly-flagged gaps in the v0.9.0 snapshot the reviewer will see. None of them are load-bearing — each is a small, 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
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 (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 https://tmoussa.codeberg.page/ConformalLabpp/ (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
check-test-counts.sh not wired into CI. The script exists, runs locally, and gives the correct answer (23 + 234 = 257 / 0 skipped). CI does not yet fail on a mismatch. local guard exists, CI integration pending next workflow touch ~1 hour
CP-Euclidean and Inversive-Distance research-track entries. Both ship a working DCE solver, but lack the auxiliary utilities the Euclidean / HyperIdeal entries have (curvature inspection helpers, edge-flip Delaunay maintenance for ID). Out of port scope; listed in research-track.md. research-track, not blocking per-utility
StereographicUnwrapper, CircleDomainUnwrapper, CuttingUtility, KoebePolyhedron. Mentioned in roadmap + research-track docs but not yet ported. Java versions still authoritative. documented as Phase 11+ / optional weeks each — explicit "out of port scope unless requested"

The honest framing for the meeting: the porting layer hits its target for the 5 Phase-8b-Lite DCE entries; the hackability layer is demonstrably in place (3 tutorials, named-parameter chaining, Doxygen-HTML); the research-track items are scoped but not built.


Open questions for the external reviewer

Items where the project would benefit from a second opinion:

  1. Phase 9c (4g-polygon) algorithm choice. Two routes:

    • Port the Java FundamentalPolygonUtility + CanonicalFormUtility literally (~2 weeks).
    • Or: re-derive from Springborn 2020 §5 using the existing cut_graph.hpp + holonomy infrastructure (~3 weeks, cleaner architecture). Which is preferred? See phases.md §Phase 9c.
  2. Phase 10a (forms) priorities. Three sub-items (DiscreteHarmonicFormUtility, DiscreteHolomorphicFormUtility, CanonicalBasisUtility) interlock. Which to start with?

  3. Analytic Hessian payoff. The Schläfli-based analytic HyperIdeal Hessian (Phase 9b-analytic — derivation already written: hyperideal-hessian-derivation.md) would add another ~6× over block-FD. Is that worth ~2 weeks of implementation effort for a working-mesh size on which?

  4. CGAL upstream vs independent distribution. Does the reviewer know a CGAL editor / has personal opinion on the LGPL-vs-MIT trade-off?

  5. geometry-central cross-validation (GC-1). Two libraries solve the same DCE problem from different algorithmic directions (Newton-on-mesh vs Ptolemaic-flips-on-intrinsic-triangulation). An independent comparison would be a nice paper. Interested?