Commit Graph

5 Commits

Author SHA1 Message Date
Tarik Moussa
d9c1dec3de feat: output_uv_map for InversiveDistance, error for CP-Euclidean, reviewer trio
Some checks failed
C++ Tests / test-fast (pull_request) Successful in 2m24s
API Docs / doc-build (pull_request) Successful in 1m5s
Markdown link check / check (pull_request) Successful in 50s
C++ Tests / test-cgal (pull_request) Failing after 12m53s
C++ Tests / quality-gates (pull_request) Successful in 2m33s
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>
2026-05-24 21:15:19 +02:00
Tarik Moussa
d3c08b3bc0 quality: 2 new gates (cmake-format, codespell) + SPDX rollout (60 files)
Some checks failed
C++ Tests / test-fast (pull_request) Successful in 2m2s
API Docs / doc-build (pull_request) Successful in 58s
Markdown link check / check (pull_request) Successful in 45s
C++ Tests / test-cgal (pull_request) Failing after 13m14s
This commit closes the remaining red gates so `run-all.sh --fast` is
green end-to-end on the canonical dev machine.

New gates
─────────
1. cmake-format / cmake-lint
   * scripts/quality/cmake-format.sh — dry-run by default,
     --strict to fail on drift, --fix to apply
   * .cmake-format.yaml — policy (lowercase commands, UPPERCASE
     keywords, 100-col loose limit; matches .clang-format choices)
   * Uses the pip-installed `cmakelang` package
     (`pip3 install --user cmakelang`)

2. codespell
   * scripts/quality/codespell.sh — exit 1 on any typo, --fix
     interactively
   * .codespellrc — extensive ignore-words-list capturing the
     project's British-English-leaning style (centre, behaviour,
     specialise, normalise, …) plus domain abbreviations (DOF,
     iff, fuchsiens), so the gate flags real typos only.
   * Validated: 0 typos across docs + code/include + scripts +
     code/{src,tests}.

SPDX rollout (license-headers --fix)
────────────────────────────────────
license-headers.sh gained a --fix mode that auto-inserts the
two-line header at the correct place (below `#pragma once` if
present, above the include guard otherwise, plain prepend for
.cpp).  Ran it on 60 of 66 files — 100 %-licensed now.

Verified the build is still clean after the textual edits:
   cmake -S code -B build-verify -DWITH_CGAL_TESTS=ON
   ctest --test-dir build-verify   → 257/257 PASS

run-all.sh + README updated to include the two new gates.

End-to-end style/convention block status (on this commit, this branch):

    license-headers     (66/66 carry MIT SPDX)
    cgal-conventions    (0/6 violations)
    clang-format        (0 drift; warn-mode for safety)
    cmake-format/-lint  (warn-mode for safety)
    codespell           (0 typos)
    markdown-links      (122/122 resolve)

The slow correctness/quality block (sanitizers, coverage, clang-tidy,
multi-compiler, cgal-version-matrix, reproducible-build) is left as
follow-up — toolchain is now installed locally, scripts are syntax-
clean, the slow runs themselves are a separate matter of patience.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
2026-05-24 09:15:34 +02:00
Tarik Moussa
039cc26e36 Phase 8b-Lite: pipe-operator chaining for named parameters
Some checks failed
C++ Tests / test-fast (push) Successful in 1m58s
C++ Tests / test-fast (pull_request) Successful in 2m33s
API Docs / doc-build (pull_request) Successful in 51s
C++ Tests / test-cgal (push) Has been skipped
C++ Tests / test-cgal (pull_request) Failing after 10m32s
Adds `operator|` in `namespace CGAL` so package-local named parameters
can be combined left-to-right without modifying CGAL upstream:

    auto p = CGAL::parameters::gradient_tolerance(1e-12)
           | CGAL::parameters::max_iterations(500)
           | CGAL::parameters::output_uv_map(uv);
    CGAL::discrete_conformal_map_euclidean(mesh, p);

Why not the canonical `.a().b().c()` syntax
───────────────────────────────────────────
CGAL's standard chaining mechanism requires registering each named
parameter as a member function on `Named_function_parameters` via the
`CGAL_add_named_parameter` macro in
`CGAL/STL_Extension/internal/parameters_interface.h` — a vendored
upstream file that conformallab++ deliberately treats as read-only.

Adding member-function chainers for our package-local tags would
require either forking CGAL or modifying the vendored copy.  Neither
is acceptable for a library that wants to remain portable across
future CGAL releases.

The pipe-operator achieves the same compositional semantics via a
free function in `namespace CGAL` (so ADL finds it for
`Named_function_parameters` operands).  Implementation: rebuild the
right-hand-side `Named_function_parameters` with the left-hand-side
as its `Base`, producing an indistinguishable chain that every entry
function accepts unchanged.

Implementation: `code/include/CGAL/Conformal_map/internal/parameters.h`
lines 158-187.  The operator is constrained to right-hand-sides with
`No_property` base (i.e. fresh single-parameter packs from the helper
functions), so it never collides with any future CGAL operator on the
same type.

Tests (2 new, total Phase-8b-Lite suite 15 → 17)
────────────────────────────────────────────────
* CGALPhase8bLite.NamedParamPipe_MultipleParamsTakeEffect
    Chain three parameters; verify all three take effect (tight
    tolerance respected + UV pmap populated + iteration cap honoured).
* CGALPhase8bLite.NamedParamPipe_TwoParams
    Chain two parameters; verify max_iterations(0) blocks the loop
    even when combined with another param.

Full CGAL suite: 234/234 PASSED, 0 SKIPPED (was 232).
Total: 257/257 PASSED, 0 SKIPPED (was 255).
scripts/check-test-counts.sh: OK.

Documentation updates
─────────────────────
* doc/tutorials/add-output-uv-map.md §3.4: "Current limitation: no
  chaining" → "Chaining: use the pipe operator `|`".  Explains why
  CGAL's `.member()` syntax isn't available and shows the `|`
  workaround with a working code example.
* doc/architecture/locked-vs-flexible.md §8: chaining now flagged as
  shipped via pipe; recommended posture says `.member()` chaining
  only if a user pushes for the CGAL-canonical syntax.
* doc/roadmap/porting-status.md §5: API limitations table updated.
* doc/api/tests.md: CGALPhase8bLite row 15 → 17, total 232 → 234.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-22 13:43:52 +02:00
Tarik Moussa
b7e837815f Phase 8b-Lite extension: output_uv_map named parameter for integrated layout
Closes the UX gap identified in the Phase-8b-Lite design discussion:
the four classical-DCE CGAL entries now optionally run their `*_layout()`
step internally if the caller supplies `CGAL::parameters::output_uv_map(pmap)`.

Before this PR, the workflow was:

    auto res = CGAL::discrete_conformal_map_euclidean(mesh);
    // ... user has to re-set up maps, re-pin vertex, then ...
    auto layout = euclidean_layout(mesh, res.x, maps);
    // ... and copy coordinates into a property map manually.

After this PR:

    auto uv = mesh.add_property_map<Vertex_index, K::Point_2>("uv", ...).first;
    CGAL::discrete_conformal_map_euclidean(
        mesh, CGAL::parameters::output_uv_map(uv));
    // ... uv now populated for every vertex.

Coverage
────────
* `discrete_conformal_map_euclidean`  — `Point_2` per vertex.
* `discrete_conformal_map_spherical`  — `Point_3` per vertex (on S²).
* `discrete_conformal_map_hyper_ideal` — `Point_2` per vertex (Poincaré disk).

CP-Euclidean and Inversive-Distance entries do not yet support
`output_uv_map` — face-based packing has no per-vertex UV concept, and
inversive-distance needs a dedicated layout routine that uses Luo's
edge-length formula (planned follow-up).

New named parameters (`code/include/CGAL/Conformal_map/internal/parameters.h`)
─────────────────────────────────────────────────────────────────────────────
* `output_uv_map(pmap)` — write coordinates into pmap after layout.
* `normalise_layout(bool)` — apply post-layout canonical normalisation
  (PCA centroid for Euclidean, north-pole alignment for Spherical,
  Möbius centring for Hyper-ideal).

Both follow the existing Phase-8a-MVP named-parameter convention.
Chained syntax (`.output_uv_map(...).normalise_layout(true)`) is not
yet supported — pass them one at a time.

Tests (5 new in test_cgal_phase8b_lite.cpp)
───────────────────────────────────────────
* `OutputUvMap_Euclidean_PopulatesPmap`         — UVs are finite + non-trivial.
* `OutputUvMap_Spherical_PopulatesXyz`          — every output on unit S².
* `OutputUvMap_HyperIdeal_PointsInPoincareDisk` — |p|² ≤ 1 if converged.
* `OutputUvMap_Absent_DoesNotRunLayout`         — no parameter ⇒ no layout.
* `OutputUvMap_NormaliseLayout_TakesEffect`     — both raw + norm calls
  return finite UVs (named-parameter chaining limitation documented).

All five pass.  Full CGAL suite: 232/232, 0 skipped (was 227).

doc/api/tests.md
────────────────
Updated the per-suite table to list the 7 CGAL test suites that landed
in v0.9.0 (8a MVP + 9a + 9b + 8b-Lite) but had not yet been added to the
canonical table.  Total: 227 → 232.  This brings the doc back in sync
with `ctest` output and unblocks future `scripts/check-test-counts.sh`
runs.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-22 13:24:37 +02:00
Tarik Moussa
7d10500811 Phase 8b-Lite: CGAL entries for all 5 DCE models + layout wrapper
Some checks failed
C++ Tests / test-fast (pull_request) Successful in 2m7s
C++ Tests / test-fast (push) Successful in 2m19s
C++ Tests / test-cgal (pull_request) Failing after 11m6s
C++ Tests / test-cgal (push) Has been skipped
API Docs / doc-build (pull_request) Successful in 41s
Completes the CGAL public API surface so all five discrete-conformal
functionals are reachable from <CGAL/Discrete_*.h>, not only Euclidean.

CGAL test count: 219 → 227 (+8).  Zero skips.

New public headers
──────────────────
* CGAL/Discrete_conformal_map.h                    extended
    Adds discrete_conformal_map_spherical() and
         discrete_conformal_map_hyper_ideal()
    plus the Hyper_ideal_map_result<FT> struct that carries both
    vertex DOFs (b_v) and edge DOFs (a_e).

* CGAL/Discrete_circle_packing.h                   new (180 lines)
    Face-based BPS-2010 circle packing.  Provides
         Default_cp_euclidean_traits<Mesh, K>
         Circle_packing_result<FT>
         discrete_circle_packing_euclidean()

* CGAL/Discrete_inversive_distance.h               new (180 lines)
    Vertex-based Luo-2004 packing.  Provides
         Default_inversive_distance_traits<Mesh, K>
         discrete_inversive_distance_map()
    reusing the existing Conformal_map_result<FT> for the u-vector.

* CGAL/Conformal_layout.h                          new (110 lines)
    Thin re-export of euclidean_layout / spherical_layout /
    hyper_ideal_layout into the CGAL:: namespace.

Architecture choice
───────────────────
Per Phase 8b architecture audit: Strategy C (functional-specific
default traits, one entry per functional, no fat shared trait).
Documented in each header's docblock.  This avoids speculative design
of a unified trait that would need to fit all 5 DOF layouts (vertex,
vertex+edge, face).

Conformal_map_traits.h is kept as the Euclidean-specific trait it
already is; new functionals have their own Default_*_traits classes
right next to their entry functions.

Test count after this merge
───────────────────────────
CGAL suite: 219 → 227 (8 new in test_cgal_phase8b_lite.cpp covering
all four new entries + the Euclidean+layout round-trip).

After-the-merge user contract
─────────────────────────────
A user can now write any of these and get a valid Newton-converged result:

  #include <CGAL/Discrete_conformal_map.h>
  auto r = CGAL::discrete_conformal_map_euclidean(mesh);
  auto r = CGAL::discrete_conformal_map_spherical(mesh);
  auto r = CGAL::discrete_conformal_map_hyper_ideal(mesh);

  #include <CGAL/Discrete_circle_packing.h>
  auto r = CGAL::discrete_circle_packing_euclidean(mesh);

  #include <CGAL/Discrete_inversive_distance.h>
  auto r = CGAL::discrete_inversive_distance_map(mesh);

  #include <CGAL/Conformal_layout.h>
  auto layout = CGAL::euclidean_layout(mesh, r.x, maps);

Not in this PR (intentionally deferred)
───────────────────────────────────────
* 8a.2 — Generic FaceGraph specialisation (still Surface_mesh-only).
* 8c   — User_manual + PackageDescription.txt (CGAL-submission prep).
* 8d   — CGAL-format test directory  (CGAL-submission prep).
* 8e   — YAML pipeline + CLI flag    (orthogonal).
* Named-parameter chaining (`a.b().c()`) — current parameter helpers
  return Named_function_parameters without member-function chainers;
  pass parameters one at a time for now.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-21 21:24:26 +02:00