Files
ConformalLabpp/doc/reviewer/cgal-submission-readiness-audit-2026-05-31.md
Tarik Moussa 7902ae8e2c docs(reviewer): add external audits — test/error-handling, API/perf, CGAL readiness
Three complementary, self-contained audit documents (each actionable by a
fresh session, with file:line, code snippets, fixes, acceptance criteria):

- test-coverage-error-handling-audit: coverage gaps + error-handling robustness;
  agreed gate 80% line / 70% branch / 90% function.
- api-performance-audit: API-naming consistency (A1–A5, decided) + Newton-solver
  performance (B1 block-FD, redundant gradient evals, factorization reuse).
- cgal-submission-readiness-audit: G0 porting-rights blocker (original is
  unlicensed Varylab/TU-Berlin code; this is a derivative work — clarify with
  authors before any license/release), G1 license, layout, concept, manual, tests.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-05-31 10:21:24 +02:00

22 KiB
Raw Permalink Blame History

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 A1A5)
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 12 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:

// 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:

// ════════════════════════════════════════════════════════════════════════════
// \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:

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 A1A5: 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 A1A5 before submission, not after — renaming a shipped CGAL API requires a deprecation cycle.

Done when

  • A1A5 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)

  1. G0 (porting rights) — owner-level legal precondition; contact the Varylab/TU-Berlin authors. Nothing below should ship publicly until this is resolved.
  2. G1 (license decision) — follows directly from G0; pick GPLv3+ if submission stays the goal.
  3. G2 + G3 + G8 (restructure) — do the layout move and header rename together; drop vendored CGAL on the submission branch.
  4. G9 (public/internal split) — naturally part of the restructure.
  5. G11 / A1A5 (lock the public names) — before any docs are written against them. (A1A3 are internal-API renames and are safe to do independently of G0; A4A5 touch the public CGAL surface and should wait for the G0/G1 outcome.)
  6. G4 + G5 (concept file + user manual + PackageDescription) — the doc deliverables.
  7. G6 (CGAL test harness) — port the 272 tests off GoogleTest.
  8. G7 (deliver or narrow genericity).
  9. 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 G2G12 require new research, but collectively they are a significant, mostly-mechanical effort. G1 is a decision, not work.