From cfbbc1b21fbd2b592683cdde91de64e2a9f509c6 Mon Sep 17 00:00:00 2001 From: Tarik Moussa Date: Sun, 31 May 2026 00:20:43 +0200 Subject: [PATCH] fix(dof-assign): correct "pin before" docs + add gauge-vertex overloads MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Finding-D from doc/reviewer/external-audit-2026-05-30.md. All three DOF-assignment functions iterated over every vertex unconditionally, overwriting any v_idx set before the call. The documentation said "pin before OR after" — the "before" option was silently wrong (the pin would be overwritten). For the inversive-distance variant the doc explicitly said the pre-call pin would make the function "a no-op for that vertex", which was false. Changes (three headers): - euclidean_functional.hpp assign_euclidean_vertex_dof_indices() - spherical_functional.hpp assign_vertex_dof_indices() - inversive_distance_functional.hpp assign_inversive_distance_vertex_dof_indices() For each: 1. Single-arg overload: doc corrected to "pin AFTER, not before; pre-call pins are overwritten" 2. New two-arg overload accepting a Vertex_index gauge: pins the requested vertex (v_idx=-1) in a single pass, preventing the user error entirely Three new GTests in test_euclidean_functional.cpp: SingleArg_PinBeforeHasNoEffect — documents the old pitfall TwoArg_GaugeIsPinnedOthersAreSequential — verifies the new overload TwoArg_NewtonConvergesWithGaugeOverload — end-to-end correctness 266/266 CGAL tests pass, 0 failed. Co-Authored-By: Claude Sonnet 4.6 --- code/include/euclidean_functional.hpp | 26 +++++- .../include/inversive_distance_functional.hpp | 24 ++++-- code/include/spherical_functional.hpp | 23 ++++- code/tests/cgal/test_euclidean_functional.cpp | 84 +++++++++++++++++++ doc/reviewer/external-audit-2026-05-30.md | 2 +- 5 files changed, 148 insertions(+), 11 deletions(-) diff --git a/code/include/euclidean_functional.hpp b/code/include/euclidean_functional.hpp index 37e766f..8ea93e0 100644 --- a/code/include/euclidean_functional.hpp +++ b/code/include/euclidean_functional.hpp @@ -95,10 +95,14 @@ inline EuclideanMaps setup_euclidean_maps(ConformalMesh& mesh) /// Assign sequential DOF indices `0..n-1` to all vertices. /// -/// **Note:** does NOT pin a gauge vertex. For closed meshes the caller -/// must set one `m.v_idx[v] = -1` either before or after this call to -/// remove the rotational mode (the Newton solver's SparseQR fallback -/// will otherwise pick a minimum-norm solution but at higher cost). +/// **Note:** this overload assigns indices to ALL vertices unconditionally. +/// Any `v_idx` set before the call is overwritten. To pin a gauge vertex, +/// either use the two-argument overload below, or set `m.v_idx[v] = -1` +/// **after** this call. Pinning before this call has no effect. +/// +/// For closed meshes one gauge vertex should be pinned to remove the +/// rotational null mode (the Newton solver's SparseQR fallback handles an +/// unpinned closed mesh automatically, but at higher cost). inline int assign_euclidean_vertex_dof_indices(ConformalMesh& mesh, EuclideanMaps& m) { int idx = 0; @@ -106,6 +110,20 @@ inline int assign_euclidean_vertex_dof_indices(ConformalMesh& mesh, EuclideanMap return idx; } +/// Assign sequential DOF indices to all vertices, pinning `gauge` +/// (`m.v_idx[gauge] = -1`). Use this overload on closed meshes to fix +/// the rotational gauge mode in a single call. +/// +/// \returns The number of free DOFs assigned (`num_vertices − 1`). +inline int assign_euclidean_vertex_dof_indices(ConformalMesh& mesh, EuclideanMaps& m, + Vertex_index gauge) +{ + int idx = 0; + for (auto v : mesh.vertices()) + m.v_idx[v] = (v == gauge) ? -1 : idx++; + return idx; +} + /// Assign DOF indices for all vertices AND all edges (vertex-DOFs first, /// then edge-DOFs). Use this overload for the "cyclic" formulation that /// includes per-edge log-length DOFs (`λ_e`) on top of per-vertex scale diff --git a/code/include/inversive_distance_functional.hpp b/code/include/inversive_distance_functional.hpp index 2d48e3e..2a5377b 100644 --- a/code/include/inversive_distance_functional.hpp +++ b/code/include/inversive_distance_functional.hpp @@ -122,11 +122,10 @@ inline InversiveDistanceMaps setup_inversive_distance_maps(ConformalMesh& mesh) /// Assign sequential DOF indices `0..n-1` to every vertex. /// -/// **Note:** this overload does NOT pin a gauge vertex. The caller -/// is expected to either: -/// 1. set one `m.v_idx[v] = -1` *before* calling this function (then -/// the call is a no-op for that vertex) — OR — -/// 2. flip one assigned index back to `-1` *after* this function. +/// **Note:** this overload assigns indices to ALL vertices unconditionally. +/// Any `v_idx` set before the call is overwritten. To pin a gauge vertex, +/// either use the two-argument overload below, or set `m.v_idx[v] = -1` +/// **after** this call. Pinning before this call has no effect. /// /// For a closed mesh, exactly one pin is required to remove the /// global rotational mode. @@ -138,6 +137,21 @@ inline int assign_inversive_distance_vertex_dof_indices(ConformalMesh& m return idx; } +/// Assign sequential DOF indices to all vertices, pinning `gauge` +/// (`m.v_idx[gauge] = -1`). Use this overload on closed meshes to fix +/// the rotational gauge mode in a single call. +/// +/// \returns The number of free DOFs assigned (`num_vertices − 1`). +inline int assign_inversive_distance_vertex_dof_indices(ConformalMesh& mesh, + InversiveDistanceMaps& m, + Vertex_index gauge) +{ + int idx = 0; + for (auto v : mesh.vertices()) + m.v_idx[v] = (v == gauge) ? -1 : idx++; + return idx; +} + /// Count the free DOFs (vertices with `v_idx >= 0`). inline int inversive_distance_dimension(const ConformalMesh& mesh, const InversiveDistanceMaps& m) diff --git a/code/include/spherical_functional.hpp b/code/include/spherical_functional.hpp index 0355c58..1ffac86 100644 --- a/code/include/spherical_functional.hpp +++ b/code/include/spherical_functional.hpp @@ -87,7 +87,14 @@ inline SphericalMaps setup_spherical_maps(ConformalMesh& mesh) } /// Assign sequential DOF indices `0..n-1` to all vertices (no edge DOFs). -/// Caller is expected to pin one gauge vertex with `m.v_idx[v] = -1`. +/// +/// **Note:** this overload assigns indices to ALL vertices unconditionally. +/// Any `v_idx` set before the call is overwritten. To pin a gauge vertex, +/// either use the two-argument overload below, or set `m.v_idx[v] = -1` +/// **after** this call. Pinning before this call has no effect. +/// +/// On closed spherical surfaces exactly one gauge vertex must be pinned +/// to remove the global-scale null mode. inline int assign_vertex_dof_indices(ConformalMesh& mesh, SphericalMaps& m) { int idx = 0; @@ -95,6 +102,20 @@ inline int assign_vertex_dof_indices(ConformalMesh& mesh, SphericalMaps& m) return idx; } +/// Assign sequential DOF indices to all vertices, pinning `gauge` +/// (`m.v_idx[gauge] = -1`). Use this overload on closed spherical +/// surfaces to fix the global-scale gauge mode in a single call. +/// +/// \returns The number of free DOFs assigned (`num_vertices − 1`). +inline int assign_vertex_dof_indices(ConformalMesh& mesh, SphericalMaps& m, + Vertex_index gauge) +{ + int idx = 0; + for (auto v : mesh.vertices()) + m.v_idx[v] = (v == gauge) ? -1 : idx++; + return idx; +} + /// Assign DOF indices for all vertices AND all edges (vertex-DOFs first, /// then edge-DOFs). Mirrors `assign_euclidean_all_dof_indices` for the /// cyclic spherical formulation. diff --git a/code/tests/cgal/test_euclidean_functional.cpp b/code/tests/cgal/test_euclidean_functional.cpp index 9857ae5..b66662f 100644 --- a/code/tests/cgal/test_euclidean_functional.cpp +++ b/code/tests/cgal/test_euclidean_functional.cpp @@ -645,3 +645,87 @@ TEST(EuclideanFunctional, CyclicHessian_Analytic_MatchesBlockFD_Tetrahedron) max_sym = std::max(max_sym, std::abs(Ha.coeff(i, j) - Ha.coeff(j, i))); EXPECT_LT(max_sym, 1e-9) << "analytic cyclic Hessian is not symmetric"; } + +// ════════════════════════════════════════════════════════════════════════════ +// DOF assignment gauge-vertex overload (Finding-D, external-audit-2026-05-30) +// +// The single-argument assign_euclidean_vertex_dof_indices() overwrites ALL +// v_idx unconditionally — setting a pin *before* the call has no effect. +// The two-argument overload (gauge vertex) pins the requested vertex in a +// single pass. These tests verify: +// (a) single-argument: gauge must be set AFTER, not before. +// (b) two-argument: the gauge vertex gets v_idx = -1, others are sequential. +// (c) Newton converges correctly when the gauge overload is used. +// ════════════════════════════════════════════════════════════════════════════ + +TEST(EuclideanDOFAssignment, SingleArg_PinBeforeHasNoEffect) +{ + // Pin first vertex before the call → the call overwrites it → not pinned. + auto mesh = make_tetrahedron(); + auto maps = setup_euclidean_maps(mesh); + auto first = *mesh.vertices().begin(); + + maps.v_idx[first] = -1; // set pin BEFORE — should have no effect + assign_euclidean_vertex_dof_indices(mesh, maps); + + EXPECT_GE(maps.v_idx[first], 0) + << "Pre-call pin was overwritten: v_idx[first] must be >= 0 after the call"; + EXPECT_EQ(euclidean_dimension(mesh, maps), + static_cast(mesh.number_of_vertices())) + << "All vertices should be free after single-arg assign"; +} + +TEST(EuclideanDOFAssignment, TwoArg_GaugeIsPinnedOthersAreSequential) +{ + auto mesh = make_tetrahedron(); + auto maps = setup_euclidean_maps(mesh); + auto first = *mesh.vertices().begin(); + + int n = assign_euclidean_vertex_dof_indices(mesh, maps, first); + + EXPECT_EQ(maps.v_idx[first], -1) + << "Gauge vertex must have v_idx = -1"; + EXPECT_EQ(n, static_cast(mesh.number_of_vertices()) - 1) + << "Returned DOF count must be num_vertices - 1"; + EXPECT_EQ(euclidean_dimension(mesh, maps), n) + << "euclidean_dimension must equal returned count"; + + // All non-gauge vertices must have distinct indices in [0, n). + std::vector seen; + for (auto v : mesh.vertices()) { + int iv = maps.v_idx[v]; + if (v == first) continue; + EXPECT_GE(iv, 0); + EXPECT_LT(iv, n); + seen.push_back(iv); + } + std::sort(seen.begin(), seen.end()); + for (int i = 0; i < n; ++i) + EXPECT_EQ(seen[static_cast(i)], i) + << "DOF indices must be sequential 0..n-1"; +} + +TEST(EuclideanDOFAssignment, TwoArg_NewtonConvergesWithGaugeOverload) +{ + // End-to-end: use the gauge overload, then run Newton — confirms the + // pinned-vertex DOF layout is consistent with the solver. + auto mesh = make_tetrahedron(); + auto maps = setup_euclidean_maps(mesh); + compute_euclidean_lambda0_from_mesh(mesh, maps); + auto first = *mesh.vertices().begin(); + + int n = assign_euclidean_vertex_dof_indices(mesh, maps, first); + + // Natural-theta: set targets = actual angle sums at x=0 so x*=0 is the solution. + std::vector x0(static_cast(n), 0.0); + auto G0 = euclidean_gradient(mesh, x0, maps); + for (auto v : mesh.vertices()) { + int iv = maps.v_idx[v]; + if (iv >= 0) maps.theta_v[v] -= G0[static_cast(iv)]; + } + + auto res = newton_euclidean(mesh, x0, maps, 1e-10, 50); + EXPECT_TRUE(res.converged) + << "Newton did not converge with gauge-overload DOF assignment"; + EXPECT_LT(res.grad_inf_norm, 1e-9); +} diff --git a/doc/reviewer/external-audit-2026-05-30.md b/doc/reviewer/external-audit-2026-05-30.md index 16bdedc..f208918 100644 --- a/doc/reviewer/external-audit-2026-05-30.md +++ b/doc/reviewer/external-audit-2026-05-30.md @@ -702,7 +702,7 @@ index. Add a comment: | A | `hyper_ideal_functional.hpp` | 319–344 | Bug | Critical (mixed config) | ✅ Fixed 2026-05-30 | | B | `gauss_bonnet.hpp` | 87–88, 128–134 | API error | Medium | ✅ Fixed 2026-05-31 | | C | `euclidean_hessian.hpp` | 26–27, 57–58 | Doc error | Medium | ✅ Fixed 2026-05-31 | -| D | `euclidean_functional.hpp` + 2 others | 97–107 | Doc error | Medium | 🟡 Open | +| D | `euclidean_functional.hpp` + 2 others | 97–107 | Doc error | Medium | ✅ Fixed 2026-05-31 | | E | `cp_euclidean_functional.hpp` | 338–349, 373–387 | Inconsistency | Medium | 🟡 Open | | F | test files | — | Test gap | Medium | 🟠 Open | | G | test files | — | Test gap | Medium | 🟠 Open |