Skip to content

Fix multi-source heat-method geodesic distances (Dirichlet-pinned Poisson solve) - #73

Merged
petrasvestartas merged 3 commits into
compas-dev:mainfrom
jf---:jf/heat-multisource-fix
Jul 22, 2026
Merged

Fix multi-source heat-method geodesic distances (Dirichlet-pinned Poisson solve)#73
petrasvestartas merged 3 commits into
compas-dev:mainfrom
jf---:jf/heat-multisource-fix

Conversation

@jf---

@jf--- jf--- commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Problem

heat_geodesic_distances / HeatGeodesicSolver (and the two isoline functions built on them) return badly wrong distances whenever more than one source vertex is given — the more sources, the worse. The flagship use case "distance from the boundary" (seed every boundary vertex of an open mesh) reads ~0.5x the true distance near the boundary, with most near-boundary vertices reading below the euclidean straight-line lower bound, which is impossible for a distance. Single-source results are fine, which is why the examples (1–8 sources) never showed it.

Root cause

CGAL's Heat_method_3::Surface_mesh_geodesic_distances_3 recovers the Poisson potential phi only up to one additive constant, and phi is not constant across a multi-vertex source set (the normalized gradient field is not exactly integrable). Its value_at_source_set() then computes

d(i) = min over sources s of |phi(i) - phi(s)|

— the 1-D distance from the value phi(i) to the set of source values. Every vertex whose phi lands inside the source-value spread is folded toward 0, and the far field is shifted by max_s phi(s) instead of a single constant. With one source this degenerates to the correct shift; with many sources it collapses the near field. The fold is many-to-one, so no post-processing of the returned distances can repair it.

Fix

Replace the backend with an own Eigen implementation of the heat method (Crane, Weischedel & Wardetzky 2017) that solves the Poisson step Dirichlet-constrained: source vertices are removed from the unknowns and pinned to 0, so the spread cannot exist in the first place. Same discretization ingredients as before (cotan Laplacian with the same 1e-8 diagonal regularization, barycentric mass, t = mean_edge_length^2), one heat factorization reused across HeatGeodesicSolver.solve() calls.

Contract improvements that come with the pin:

  • distances are exactly 0 at every source (previously only approximately, via the fold),
  • distances are non-negative,
  • correct for arbitrary source sets, including dense bands and solid source regions,
  • empty source sets raise ValueError; meshes with a connected component containing no source raise RuntimeError (both previously returned meaningless values),
  • the docstring claim that the intrinsic-Delaunay variant was used is dropped (the binding instantiated the class default, Direct).

Validation (reviewers: one command)

pytest tests/test_geodesics.py -q

The new test_heat_geodesic_multisource_boundary_sources uses the new committed fixture data/flat_star_disk_irregular_rim.off: a flat star-shaped disk (z = 0) with irregular, variable-density boundary sampling, all 508 boundary vertices seeded as sources. Because the mesh is flat, the exact geodesic distance to the source set in the near band equals the euclidean distance to the nearest source (verified to ~1e-15 against exact polyhedral MMP geodesics when the fixture was generated), and the euclidean distance is a hard lower bound everywhere — so the test needs no external oracle. It fails on main and passes here:

band mean d / d_true band vertices below the euclidean LOWER bound
main (CGAL Heat_method_3) 0.544 83.4 %
this PR 0.979 1.1 % (marginal, sub-edge-length)

Cross-checks against exact polyhedral geodesics (pygeodesic MMP, not a test dependency):

case main this PR
12.9k-vertex open slice, 434 boundary sources, near-rim band 0.408 0.966
elephant.off, single source (near-source band) 0.995 0.995
elephant.off, 5 spread sources 0.951 1.002

All 69 existing tests pass unchanged (test_heat_geodesic_distances_multiple_sources already pinned d(source) == 0 and d >= 0 — contracts the fold satisfied by construction and the pin satisfies structurally).

Upstream

The defect is CGAL's, not this binding's; a minimal upstream fix (mean-over-sources shift in value_at_source_set, validated on the same fixture: 0.54 → 1.13, lower-bound violations 83 % → 2.4 %) is filed as CGAL/cgal#9565, with the same fixture and an equivalent C++ test. This PR removes the dependency on the affected class either way and makes the distances exact at the sources, which the mean-shift cannot.

…ned Poisson solve

CGAL Heat_method_3's value_at_source_set computes min_s |phi(i)-phi(s)| — a
distance between values — which folds every vertex whose Poisson potential
lands inside the source-value spread: all-boundary seeding on the new fixture
reads 0.54x truth near the rim, 83% of band vertices below the euclid lower
bound. Replace the backend with an own Eigen heat method (Crane 2017) whose
Poisson step pins phi=0 at every source: exactly 0 at sources, accurate for
arbitrary source sets (band ratio 0.98), single-source unchanged. Fail-loud on
empty source sets / unseeded components. Fixture + regression test included;
test fails on the previous backend.
@jf---

jf--- commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

Fixture provenance — data/flat_star_disk_irregular_rim.off is fully synthetic (CC0), generated with the script below (numpy + scipy.spatial.Delaunay, fixed seed, parameters n_rim=500, n_int=2500, amp=0.35, dens_pow=1.5, seed=7), then frozen as a file so the test is deterministic across scipy/Qhull versions. The flat-truth claim (euclid-to-nearest-source == exact geodesic in the 2h..6h band) was verified against exact polyhedral MMP geodesics (pygeodesic, cross-validated bit-identical with tvb-gdist): max disagreement in the band 6.9e-16.

generator
import numpy as np
from scipy.spatial import Delaunay

def build(n_rim=500, n_int=2500, amp=0.35, dens_pow=1.5, seed=7):
    """Flat star disk; rim sampled with variable density (smooth warp of the parameter)."""
    rng = np.random.RandomState(seed)
    u = np.sort(rng.uniform(0, 1, n_rim))
    t = 2 * np.pi * (u + 0.35 / (2 * np.pi * 3) * np.sin(2 * np.pi * 3 * u) * dens_pow)
    r = 1.0 + amp * np.sin(7 * t)
    rim = np.c_[r * np.cos(t), r * np.sin(t)]
    pts = []
    while len(pts) < n_int:
        p = rng.uniform(-1.4, 1.4, size=(n_int * 4, 2))
        ang = np.arctan2(p[:, 1], p[:, 0])
        rad = np.hypot(p[:, 0], p[:, 1])
        keep = rad < (1.0 + amp * np.sin(7 * ang)) * 0.985
        pts.extend(p[keep].tolist())
    P = np.vstack([rim, np.array(pts[:n_int])])
    F = Delaunay(P).simplices.astype(np.int32)
    c = P[F].mean(axis=1)                      # drop triangles outside the concave region
    ang = np.arctan2(c[:, 1], c[:, 0])
    F = F[np.hypot(c[:, 0], c[:, 1]) < 1.0 + amp * np.sin(7 * ang)]
    V = np.c_[P, np.zeros(len(P))]
    used = np.unique(F)                        # drop unused vertices, remap
    remap = -np.ones(len(V), dtype=np.int64)
    remap[used] = np.arange(len(used))
    return np.ascontiguousarray(V[used]), np.ascontiguousarray(remap[F].astype(np.int32))

Why this shape: the defect scales with the spread of the Poisson potential over the source set, and the spread is driven by irregular boundary sampling density (a regular grid barely shows it: ratio 0.96). This mesh maximizes the failure (0.54) while both correct normalizations stay within a few percent.

@jf---
jf--- requested a review from petrasvestartas July 10, 2026 13:54
Comment thread tests/test_geodesics.py
mesh = Mesh.from_off(fixture)
V, F = mesh.to_vertices_and_faces()
V = np.asarray(V, dtype=np.float64)
F = np.asarray(F, dtype=np.int64)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use input as a compas mesh and internally in the wrapper function convert it to numpy arrays.
The wrappers must be user friendly and very minimal python compas style functions.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in e6889ed, via @overload rather than a Mesh-only switch.

Rationale: 7 wrappers (booleans, measure, meshing, projection, slicer, subdivision, skeletonization) take the (V, F) VerticesFaces tuple; isolines() is the only Mesh-input one, and only because it reads a named vertex attribute off the mesh. Geodesics needs no attributes, so a Mesh-only signature would make it the odd one out and break existing tuple callers.

So all four entry points now accept either a compas.datastructures.Mesh or the (V, F) tuple — declared with @overload, coerced through a single _as_vertices_faces seam. heat_geodesic_distances(mesh, sources) now works directly, and tests/test_geodesics.py::test_mesh_input_matches_tuple_input locks Mesh == tuple parity across all four.

Comment thread tests/test_geodesics.py Outdated
- Dirichlet-pinned heat method (this backend): ratio ~0.98, ~1% marginal
violations, exactly 0 at every source.
"""
fixture = Path(__file__).parent.parent / "data" / "flat_star_disk_irregular_rim.off"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For file imports use:

import pathlib

Define input file path

IFILE = pathlib.Path(file).parent.parent / "data" / "shell_final.json"

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already on pathlibPath(__file__).parent.parent / "data" / "flat_star_disk_irregular_rim.off". Hoisted it to a module-level IFILE constant in e6889ed.

Comment thread tests/test_geodesics.py

ratio = float(np.mean(d[band] / eu[band]))
assert 0.85 < ratio < 1.3, f"near-band mean ratio {ratio:.3f} (the CGAL fold reads ~0.54)"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add in docs example with an image

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For example:
image

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

amazing, thanks for the review @petrasvestartas

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jf--- would it be possible to make these little adjustments so that we could merge this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done — regenerated in 99f3de8, rendered on this (fixed) backend so the multi-source row (bottom) now shows the corrected field. Also chrome-free (direct framebuffer grab, no title bar / sidebar) and higher-res than the previous screenshot.

all four wrappers take Mesh|VerticesFaces, one _as_vertices_faces seam.
test: Mesh==tuple contract + regression fed via Mesh; IFILE const.
@jf---

jf--- commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator Author

yep coming up

@petrasvestartas
petrasvestartas merged commit 37f05df into compas-dev:main Jul 22, 2026
8 checks passed
@petrasvestartas

Copy link
Copy Markdown
Collaborator

@jf--- if i merged to early reopen this pr

@jf---

jf--- commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator Author

hi @petrasvestartas what was missing from this PR? checked but seems complete?

@petrasvestartas

Copy link
Copy Markdown
Collaborator

all good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants