conformallab/ConformalLabpp
3
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix(minor): all five MINOR findings — doc, accuracy, and DRY

Browse Source 
MINOR-1 (spherical_functional.hpp:426)
  Wrong comment said "second derivative < 0 for a convex functional".
  The spherical energy is *concave* (NSD Hessian); the monotone-f argument
  applies to both convex and concave functionals equally.  Comment rewritten
  to explain the actual physics: increasing scale increases all angles and
  thus reduces Σ G_v.

MINOR-2 (spherical_functional.hpp:494-495)
  Forward finite difference O(ε) → central finite difference O(ε²):
    old: dft = (sum_Gv(t + fd_eps) - ft) / fd_eps
    new: dft = (sum_Gv(t + fd_eps) - sum_Gv(t - fd_eps)) / (2*fd_eps)
  Same cost when the extra sum_Gv(t - fd_eps) replaces the cached ft.

MINOR-3 (euclidean_functional.hpp, spherical_functional.hpp,
         inversive_distance_functional.hpp)
  New header gauss_legendre.hpp centralises the 10-point Gauss-Legendre
  nodes and weights (gl10_nodes() / gl10_weights()).  The three energy
  functions now use the shared accessors instead of duplicated local
  static arrays.

MINOR-4 (euclidean_functional.hpp, spherical_functional.hpp,
         hyper_ideal_functional.hpp, inversive_distance_functional.hpp)
  halfedge_to_index() centralised in conformal_mesh.hpp.  All four local
  aliases (eucl_hidx, spher_hidx, hidx, id_detail::hidx) now delegate to
  it as one-line wrappers; the aliases are kept for now to avoid a larger
  call-site churn, clearly documented as thin wrappers.

MINOR-5 (clausen.hpp:33-38)
  Added a comment above inits() explaining the intentional off-by-one
  return value and how it interacts with csevl() — matching the Java
  Clausen.inits() / csevl() contract.

277/277 CGAL + 26/26 pure-math tests pass, 0 failed.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Tarik Moussa
2026-05-31 00:47:21 +02:00
parent 2325328f77
commit 59a26123c8
8 changed files with 106 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files

6
code/include/clausen.hpp
View File

@@ -30,6 +30,12 @@ inline double csevl(double x, const double* cs, int n) noexcept {
// Count Chebyshev terms needed so truncation error <= eta. // Count Chebyshev terms needed so truncation error <= eta.
// Corresponds to Java Clausen.inits(). // Corresponds to Java Clausen.inits().
//
// Note on return value: the loop decrements n one extra time after the
// stopping condition is met, so the returned value is one less than the
// last index checked. Callers pass the result directly to csevl() as
// the term count, which evaluates terms [0, n-1] — this is intentional
// and matches the Java Clausen.inits() / csevl() contract exactly.
inline int inits(const double* series, int n, double eta) noexcept { inline int inits(const double* series, int n, double eta) noexcept {
double err = 0.0; double err = 0.0;
while (err <= eta && n-- != 0) { while (err <= eta && n-- != 0) {

20
code/include/conformal_mesh.hpp
View File

@@ -37,6 +37,7 @@
#include <CGAL/Simple_cartesian.h> #include <CGAL/Simple_cartesian.h>
#include <CGAL/Surface_mesh.h> #include <CGAL/Surface_mesh.h>
#include <cstdint>
#include <string> #include <string>
namespace conformallab { namespace conformallab {
@@ -107,4 +108,23 @@ inline auto add_face_properties(ConformalMesh& mesh)
return ftype; return ftype;
} }
// ── Half-edge index helper ────────────────────────────────────────────────────
//
// Convert a Halfedge_index to std::size_t for use as a vector subscript.
// Each functional previously duplicated this one-liner under a name like
// eucl_hidx / spher_hidx / hidx. Centralised here (MINOR-4 fix).
//
// Implementation: the double-cast uint32_t → size_t is intentional. CGAL 6.x
// Surface_mesh stores half-edge indices internally as 32-bit unsigned integers.
// Casting directly to size_t on a 64-bit system would produce the same result
// because CGAL index types use non-negative values, but the explicit intermediate
// cast documents the assumption and silences spurious sign-conversion warnings.
/// Convert a `Halfedge_index` to `std::size_t` for vector subscript use.
/// Replaces the duplicated `eucl_hidx`, `spher_hidx`, `hidx` helpers.
inline std::size_t halfedge_to_index(Halfedge_index h) noexcept
{
return static_cast<std::size_t>(static_cast<std::uint32_t>(h));
}
} // namespace conformallab } // namespace conformallab

26
code/include/euclidean_functional.hpp
View File

@@ -41,6 +41,7 @@
#include "conformal_mesh.hpp" #include "conformal_mesh.hpp"
#include "constants.hpp" #include "constants.hpp"
#include "gauss_legendre.hpp"
#include "euclidean_geometry.hpp" #include "euclidean_geometry.hpp"
#include <CGAL/boost/graph/iterator.h> #include <CGAL/boost/graph/iterator.h>
#include <vector> #include <vector>
@@ -173,11 +174,10 @@ static inline double eucl_dof_val(int idx, const std::vector<double>& x)
return idx >= 0 ? x[static_cast<std::size_t>(idx)] : 0.0; return idx >= 0 ? x[static_cast<std::size_t>(idx)] : 0.0;
} }
/// Convert a CGAL half-edge index to a plain `std::size_t` for vector indexing. // halfedge_to_index is defined in conformal_mesh.hpp (included above).
static inline std::size_t eucl_hidx(Halfedge_index h) // The old local alias eucl_hidx is retained as a thin wrapper for now so
{ // call-sites below do not need touching; a follow-up can remove it.
return static_cast<std::size_t>(static_cast<std::uint32_t>(h)); static inline std::size_t eucl_hidx(Halfedge_index h) { return halfedge_to_index(h); }
}
/// Compute the Euclidean-functional gradient G(x): /// Compute the Euclidean-functional gradient G(x):
/// * `G_v = Θ_v − Σ_faces α_v(face)` /// * `G_v = Θ_v − Σ_faces α_v(face)`
@@ -272,20 +272,8 @@ inline double euclidean_energy(
const std::vector<double>& x, const std::vector<double>& x,
const EuclideanMaps& m) const EuclideanMaps& m)
{ {
static const double gl_s[10] = { const double* gl_s = gl10_nodes();
-0.9739065285171717, -0.8650633666889845, const double* gl_w = gl10_weights();
-0.6794095682990244, -0.4333953941292472,
-0.1488743389816312, 0.1488743389816312,
0.4333953941292472, 0.6794095682990244,
0.8650633666889845, 0.9739065285171717
};
static const double gl_w[10] = {
0.0666713443086881, 0.1494513491505806,
0.2190863625159820, 0.2692667193099963,
0.2955242247147529, 0.2955242247147529,
0.2692667193099963, 0.2190863625159820,
0.1494513491505806, 0.0666713443086881
};
const std::size_t n = x.size(); const std::size_t n = x.size();
double E = 0.0; double E = 0.0;

49
code/include/gauss_legendre.hpp Normal file
View File

@@ -0,0 +1,49 @@
#pragma once
// Copyright (c) 2024-2026 Tarik Moussa.
// SPDX-License-Identifier: MIT
// gauss_legendre.hpp
//
// 10-point Gauss-Legendre quadrature nodes and weights on [-1,1].
//
// Previously duplicated verbatim in:
// euclidean_functional.hpp, spherical_functional.hpp,
// inversive_distance_functional.hpp (MINOR-3 fix)
//
// Usage (integration over [0,1] via change of variables t=(1+s)/2, w=w_GL/2):
//
// const auto* s = conformallab::gl10_nodes();
// const auto* w = conformallab::gl10_weights();
// for (int k = 0; k < 10; ++k) {
// double t = (1.0 + s[k]) * 0.5;
// double wt = w[k] * 0.5;
// E += wt * dot(G(t*x), x);
// }
namespace conformallab {
/// 10-point Gauss-Legendre nodes on [−1, 1].
inline const double* gl10_nodes() noexcept {
static constexpr double s[10] = {
-0.9739065285171717, -0.8650633666889845,
-0.6794095682990244, -0.4333953941292472,
-0.1488743389816312, 0.1488743389816312,
0.4333953941292472, 0.6794095682990244,
0.8650633666889845, 0.9739065285171717
};
return s;
}
/// 10-point Gauss-Legendre weights on [−1, 1].
inline const double* gl10_weights() noexcept {
static constexpr double w[10] = {
0.0666713443086881, 0.1494513491505806,
0.2190863625159820, 0.2692667193099963,
0.2955242247147529, 0.2955242247147529,
0.2692667193099963, 0.2190863625159820,
0.1494513491505806, 0.0666713443086881
};
return w;
}
} // namespace conformallab

7
code/include/hyper_ideal_functional.hpp
View File

@@ -129,11 +129,8 @@ static inline double dof_val(int idx, const std::vector<double>& x)
return idx >= 0 ? x[static_cast<std::size_t>(idx)] : 0.0; return idx >= 0 ? x[static_cast<std::size_t>(idx)] : 0.0;
} }
/// Convert a CGAL half-edge index to a plain `std::size_t` for vector indexing. // halfedge_to_index is defined in conformal_mesh.hpp.
static inline std::size_t hidx(Halfedge_index h) static inline std::size_t hidx(Halfedge_index h) { return halfedge_to_index(h); }
{
return static_cast<std::size_t>(static_cast<std::uint32_t>(h));
}
// ── Pure-math face-angle kernel ────────────────────────────────────────────── // ── Pure-math face-angle kernel ──────────────────────────────────────────────
// //

23
code/include/inversive_distance_functional.hpp
View File

@@ -68,6 +68,7 @@
#include "conformal_mesh.hpp" #include "conformal_mesh.hpp"
#include "constants.hpp" #include "constants.hpp"
#include "gauss_legendre.hpp"
#include "euclidean_geometry.hpp" // euclidean_angles(λ12, λ23, λ31) #include "euclidean_geometry.hpp" // euclidean_angles(λ12, λ23, λ31)
#include <CGAL/boost/graph/iterator.h> #include <CGAL/boost/graph/iterator.h>
#include <vector> #include <vector>
@@ -229,10 +230,8 @@ inline double dof_val(int idx, const std::vector<double>& x) noexcept
return idx >= 0 ? x[static_cast<std::size_t>(idx)] : 0.0; return idx >= 0 ? x[static_cast<std::size_t>(idx)] : 0.0;
} }
inline std::size_t hidx(Halfedge_index h) noexcept // halfedge_to_index is defined in conformal_mesh.hpp.
{ inline std::size_t hidx(Halfedge_index h) noexcept { return halfedge_to_index(h); }
return static_cast<std::size_t>(static_cast<std::uint32_t>(h));
}
// Inversive-distance edge length squared: ℓ² = exp(2u_i) + exp(2u_j) + 2 I r_i r_j // Inversive-distance edge length squared: ℓ² = exp(2u_i) + exp(2u_j) + 2 I r_i r_j
// where r_i = exp(u_i), so: ℓ² = r_i² + r_j² + 2 I r_i r_j. // where r_i = exp(u_i), so: ℓ² = r_i² + r_j² + 2 I r_i r_j.
@@ -323,20 +322,8 @@ inline double inversive_distance_energy(
const std::vector<double>& x, const std::vector<double>& x,
const InversiveDistanceMaps& m) const InversiveDistanceMaps& m)
{ {
static const double gl_s[10] = { const double* gl_s = gl10_nodes();
-0.9739065285171717, -0.8650633666889845, const double* gl_w = gl10_weights();
-0.6794095682990244, -0.4333953941292472,
-0.1488743389816312, 0.1488743389816312,
0.4333953941292472, 0.6794095682990244,
0.8650633666889845, 0.9739065285171717
};
static const double gl_w[10] = {
0.0666713443086881, 0.1494513491505806,
0.2190863625159820, 0.2692667193099963,
0.2955242247147529, 0.2955242247147529,
0.2692667193099963, 0.2190863625159820,
0.1494513491505806, 0.0666713443086881
};
const std::size_t n = x.size(); const std::size_t n = x.size();
double E = 0.0; double E = 0.0;

37
code/include/spherical_functional.hpp
View File

@@ -36,6 +36,7 @@
#include "conformal_mesh.hpp" #include "conformal_mesh.hpp"
#include "spherical_geometry.hpp" #include "spherical_geometry.hpp"
#include "gauss_legendre.hpp"
#include <CGAL/boost/graph/iterator.h> #include <CGAL/boost/graph/iterator.h>
#include <vector> #include <vector>
#include <cmath> #include <cmath>
@@ -196,11 +197,8 @@ static inline double spher_eff_lambda(const SphericalMaps& m,
: (m.lambda0[e] + u_i + u_j); : (m.lambda0[e] + u_i + u_j);
} }
/// Convert a CGAL half-edge index to a plain `std::size_t` for vector indexing. // halfedge_to_index is defined in conformal_mesh.hpp.
static inline std::size_t spher_hidx(Halfedge_index h) static inline std::size_t spher_hidx(Halfedge_index h) { return halfedge_to_index(h); }
{
return static_cast<std::size_t>(static_cast<std::uint32_t>(h));
}
// ── Gradient only (no energy) ───────────────────────────────────────────────── // ── Gradient only (no energy) ─────────────────────────────────────────────────
@@ -321,21 +319,8 @@ inline double spherical_energy(
const std::vector<double>& x, const std::vector<double>& x,
const SphericalMaps& m) const SphericalMaps& m)
{ {
// 10-point Gauss-Legendre nodes and weights on [-1, 1]. const double* gl_s = gl10_nodes();
static const double gl_s[10] = { const double* gl_w = gl10_weights();
-0.9739065285171717, -0.8650633666889845,
-0.6794095682990244, -0.4333953941292472,
-0.1488743389816312, 0.1488743389816312,
0.4333953941292472, 0.6794095682990244,
0.8650633666889845, 0.9739065285171717
};
static const double gl_w[10] = {
0.0666713443086881, 0.1494513491505806,
0.2190863625159820, 0.2692667193099963,
0.2955242247147529, 0.2955242247147529,
0.2692667193099963, 0.2190863625159820,
0.1494513491505806, 0.0666713443086881
};
const std::size_t n = x.size(); const std::size_t n = x.size();
double E = 0.0; double E = 0.0;
@@ -423,8 +408,11 @@ inline bool gradient_check_spherical(
// Apply the shift by adding t* to every vertex DOF in x. // Apply the shift by adding t* to every vertex DOF in x.
// //
// Implementation: bisection on f(t) = Σ_v G_v(x + t·1_v). // Implementation: bisection on f(t) = Σ_v G_v(x + t·1_v).
// f is strictly monotone decreasing (second derivative < 0) for a convex // f is strictly monotone decreasing because increasing the global scale
// functional, so bisection converges in O(log₂(2·bracket/tol)) iterations. // increases all effective edge lengths and thereby all corner angles, which
// reduces Σ G_v = Σ(Θ_v − Σα_v). This holds for the concave spherical
// energy (NSD Hessian) just as well as for a convex one.
// Bisection converges in O(log₂(2·bracket/tol)) iterations.
// //
// Parameters: // Parameters:
// bracket – initial search interval [−bracket, +bracket] (default 50) // bracket – initial search interval [−bracket, +bracket] (default 50)
@@ -477,7 +465,7 @@ inline double spherical_gauge_shift(
// ── No sign change (zero may lie at a domain boundary). ─────────────────── // ── No sign change (zero may lie at a domain boundary). ───────────────────
// Use damped Newton's method with backtracking line search. // Use damped Newton's method with backtracking line search.
// f'(t) estimated by forward finite difference. // f'(t) estimated by central finite difference (O(ε²) vs O(ε) for forward).
// When the Newton step overshoots the valid domain (ΣG_v jumps back up // When the Newton step overshoots the valid domain (ΣG_v jumps back up
// because faces become degenerate), backtracking halves the step until // because faces become degenerate), backtracking halves the step until
// |f| strictly decreases. // |f| strictly decreases.
@@ -488,8 +476,7 @@ inline double spherical_gauge_shift(
for (int iter = 0; iter < 120; ++iter) { for (int iter = 0; iter < 120; ++iter) {
if (std::abs(ft) < tol) return t; if (std::abs(ft) < tol) return t;
double ftp = sum_Gv(t + fd_eps); double dft = (sum_Gv(t + fd_eps) - sum_Gv(t - fd_eps)) / (2.0 * fd_eps);
double dft = (ftp - ft) / fd_eps;
if (std::abs(dft) < 1e-14) return t; // gradient flat — give up if (std::abs(dft) < 1e-14) return t; // gradient flat — give up
double dt_raw = -ft / dft; double dt_raw = -ft / dft;

10
doc/reviewer/external-audit-2026-05-30.md
View File

@@ -708,11 +708,11 @@ index. Add a comment:
| G | test files | — | Test gap | Medium | ✅ Fixed 2026-05-31 | | G | test files | — | Test gap | Medium | ✅ Fixed 2026-05-31 |
| H | test files | — | Test gap | Medium | ✅ Fixed 2026-05-31 | | H | test files | — | Test gap | Medium | ✅ Fixed 2026-05-31 |
| I | CI workflow | — | Arch risk | High | 🔵 Open | | I | CI workflow | — | Arch risk | High | 🔵 Open |
| MINOR-1 | `spherical_functional.hpp` | 404 | Doc error | Minor | 🟡 Open | | MINOR-1 | `spherical_functional.hpp` | 404 | Doc error | Minor | ✅ Fixed 2026-05-31 |
| MINOR-2 | `spherical_functional.hpp` | 470–471 | Accuracy | Minor | 🟡 Open | | MINOR-2 | `spherical_functional.hpp` | 470–471 | Accuracy | Minor | ✅ Fixed 2026-05-31 |
| MINOR-3 | three files | — | DRY | Minor | 🟡 Open | | MINOR-3 | three files | — | DRY | Minor | ✅ Fixed 2026-05-31 |
| MINOR-4 | four files | — | DRY | Minor | 🟡 Open | | MINOR-4 | four files | — | DRY | Minor | ✅ Fixed 2026-05-31 |
| MINOR-5 | `clausen.hpp` | 33–38 | Doc | Minor | 🟡 Open | | MINOR-5 | `clausen.hpp` | 33–38 | Doc | Minor | ✅ Fixed 2026-05-31 |
--- ---