Boundary tangent-slip: BoundingSurface objects + mesh.boundary_slip API (step 1, with geometry/ownership hardening)#225
Merged
Conversation
Design for a mesh-orchestrated, boundary-surface-owned tangent-slip + restore contract. Each boundary gets a BoundingSurface object (kind radial/plane/facet/ free) that owns its geometry, state flags, and tangent_project/restore/release methods; the mesh orchestrates cross-surface vertex classification. Replaces the slip logic duplicated across the spring/ma/mmpde/ot movers and the runtime _is_radial_coords heuristic. mesh.boundaries (the persisted gmsh/DMPlex labels) is left untouched; surfaces live in a new mesh.bounding_surfaces collection. This is the step-1 (interface) doc; implementation follows in this branch toward a development PR. Step 2 (movers consume the API) stays on the mover feature branch. Underworld development team with AI support from Claude Code
Additive, self-contained boundary tangent-slip API per docs/developer/design/boundary-slip-strategy.md. No behaviour change: the development movers do not call the new API, so nothing existing is affected. - New meshing/bounding_surface.py: BoundingSurface (kind radial/plane/facet/ free) owns a boundary's geometry, slip state, and normals/tangent_project/ restore/release methods. radial restore = exact |r| snap about a known centre; plane restore = orthogonal projection onto a face; release() flips a rigid surface to free (restore becomes a no-op — the free-surface follow). + register_radial_surfaces / register_box_face_surfaces constructor helpers. - Mesh gains a SEPARATE mesh.bounding_surfaces collection (mesh.boundaries — the persisted gmsh/DMPlex labels — is left untouched), plus the orchestrator mesh.boundary_slip (returns (is_pinned, project)), the per-surface delegates mesh.tangent_project / restore_to_surface, the in-place project_to_slip_surface convenience, and register_tangent_slip_provider. Slip-vs-pin is label-driven: a vertex slips iff it lies on exactly one registered analytic surface; junctions / unregistered / degenerate-normal vertices are pinned (the step-1 safe default; facet restore is a follow-up). - Annulus / SphericalShell / CubedSphere register radial surfaces (Upper/Lower at the known radii about the origin); UnstructuredSimplexBox / StructuredQuadBox register plane surfaces for their axis-aligned faces. - tests/test_0762_bounding_surfaces.py (11 tests): constructor registration, mesh.boundaries untouched, radial/plane restore, release, register + type check, slip orchestrator keeps slipped vertices on the boundary while leaving interior alone, pin fallback when no surface is registered. The mover-side swap to this API (step 2) lands on the mover feature branch, which has the unified projector; the development movers are untouched here. Underworld development team with AI support from Claude Code
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new, additive API for mesh-owned boundary tangent-slip by adding first-class BoundingSurface objects and a mesh.boundary_slip() orchestrator, plus constructor-time registration for common analytic geometries (annulus/spherical shell/boxes). It also adds a design document and a dedicated test suite to lock down the new interface, while keeping existing mover behavior unchanged in this step.
Changes:
- Add
BoundingSurfaceobjects (radial/planein step 1) plus registration helpers for mesh constructors. - Add
mesh.bounding_surfaces,mesh.register_tangent_slip_provider(),mesh.boundary_slip()and convenience projection helpers onMesh. - Register analytic bounding surfaces in
Annulus,SphericalShell/CubedSphere, and box constructors; add tests + design doc.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
tests/test_0762_bounding_surfaces.py |
New tests covering constructor registration, restore behavior, release→free, and boundary_slip() orchestration. |
src/underworld3/meshing/bounding_surface.py |
New BoundingSurface implementation plus constructor-side registration helpers. |
src/underworld3/discretisation/discretisation_mesh.py |
Adds bounding_surfaces storage and the public mesh APIs (boundary_slip, registration, projection helpers). |
src/underworld3/meshing/annulus.py |
Registers radial bounding surfaces for Upper/Lower at construction. |
src/underworld3/meshing/spherical.py |
Registers radial bounding surfaces for spherical shell constructors. |
src/underworld3/meshing/cartesian.py |
Registers plane bounding surfaces for axis-aligned box faces. |
docs/developer/design/boundary-slip-strategy.md |
Design document describing the slip/restore contract, motivation, and migration plan. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+72
to
+80
| self.centre = None if centre is None else np.asarray(centre, dtype=float).ravel() | ||
| self.radius = None if radius is None else _as_float(radius) | ||
| self.point = None if point is None else np.asarray(point, dtype=float).ravel() | ||
| self.normal = None if normal is None else _unit(normal) | ||
| if kind == "radial" and (self.centre is None or self.radius is None): | ||
| raise ValueError("radial BoundingSurface requires centre and radius") | ||
| if kind == "plane" and (self.point is None or self.normal is None): | ||
| raise ValueError("plane BoundingSurface requires point and normal") | ||
|
|
Comment on lines
+2120
to
+2142
| from underworld3.meshing.smoothing import _pinned_mask, _auto_pinned_labels | ||
|
|
||
| dm = self.dm | ||
| pStart, pEnd = dm.getDepthStratum(0) | ||
| n_verts = pEnd - pStart | ||
| if reference_coords is None: | ||
| reference_coords = numpy.asarray(self.X.coords, dtype=float) | ||
| ref = numpy.asarray(reference_coords, dtype=float) | ||
|
|
||
| all_labels = (tuple(boundary_labels) if boundary_labels is not None | ||
| else _auto_pinned_labels(self)) | ||
| is_bnd = _pinned_mask(dm, all_labels) | ||
|
|
||
| slip_labels, free_labels = self._resolve_slip_spec(slip_spec) | ||
| surf = self.bounding_surfaces | ||
| # Only labels with a registered analytic surface can slip (step 1). | ||
| usable = [lab for lab in slip_labels if lab in surf] | ||
| masks = {lab: _pinned_mask(dm, (lab,)) for lab in usable} | ||
| count = numpy.zeros(n_verts, dtype=int) | ||
| for m in masks.values(): | ||
| count += m.astype(int) | ||
| slip_mask = is_bnd & (count == 1) | ||
| is_pinned = is_bnd & ~slip_mask |
…slip vertices Addresses Copilot review on #225: - BoundingSurface now rejects a degenerate/zero or non-finite plane normal (which would make restore() a silent no-op via _unit() returning a zero vector) and a non-positive / non-finite radius (which would collapse radial projections). Validated by test_degenerate_geometry_raises. - mesh.boundary_slip projects only OWNED slip vertices; leaves/ghosts receive their owner's projected value through the movers' existing halo-sync, so we no longer modify non-owned coordinates (parallel safety). Serial behaviour is unchanged (every vertex is owned). Documented as a follow-up (not fixed here): _pinned_mask expands labels through vertices/edges only, so a 3D face-only boundary label (markVertices=False) is not picked up. This is a pre-existing limitation of the shared helper used by every metric mover; the fix belongs with _pinned_mask itself. Underworld development team with AI support from Claude Code
Member
Author
|
Thanks @copilot — addressed in ce681ad:
On the face-only-label point: |
Copilot
AI
changed the title
Boundary tangent-slip: BoundingSurface objects + mesh.boundary_slip API (step 1)
Boundary tangent-slip: BoundingSurface objects + mesh.boundary_slip API (step 1, with geometry/ownership hardening)
Jun 9, 2026
…crumb) Document why normals() re-solves Gamma_P1 on the current mesh state (state-aware, deformation-following — needed for the free/release() surface-follow mode) rather than reading the cached _n_proj field the retired _build_slip_projector used. Notes the cost (~one projection solve per mover outer-iteration), the validated parity (~1e-16 on a centred annulus), and the fix path if it ever becomes a hot-path regression (add a cached fast-path; re-solve only for free surfaces). Underworld development team with AI support from Claude Code
The radius>0 check added in the previous commit rejected radius==0, but a SOLID sphere/annulus (radiusInner=0) legitimately registers its inner "Lower" boundary at the centre (radius 0). The mid-construction ValueError also left PETSc/mesh state corrupted, cascading into a spurious SphericalShell coordinate-reshape failure later in the full-suite run. Relax to radius>=0 (still reject negative / non-finite). Fixes the test_0001_meshes solid-mesh tests and the cascaded test_0762 SphericalShell failure. Underworld development team with AI support from Claude Code
SphericalShell construction is fragile under the accumulated PETSc/mesh state of the heavy test_05*/07* CI batch (a pre-existing mesh-lifecycle issue: the coordinate DM / cdim goes stale, giving "cannot reshape ... into shape (3)" at build time -- unrelated to boundary slip). test_0762's SphericalShell case hit this in CI, while test_0001 builds spheres cleanly in the early batch. Move the 3D radial-registration test to tests/test_0002_bounding_surface_3d.py so it runs early; the Annulus tests in test_0762 cover the 2D radial logic. Verified: the test_00[0-4]* batch (116 passed) and the test_05*/07* batch (364 passed) are both green. Underworld development team with AI support from Claude Code
lmoresi
added a commit
that referenced
this pull request
Jun 9, 2026
…lip engine (step 2) Swap all five metric movers (mmpde, spring, ma, ot, anisotropic) from their private / inline boundary tangent-slip onto the mesh-owned mesh.boundary_slip contract (step 1, PR #225) and delete the duplicates. Net -317 lines. mesh.boundary_slip: - Facet fallback: a slip label with no registered analytic surface now builds a transient `facet` BoundingSurface from the reference facets (was: pinned). - Normals computed ONCE per build (not per project() call) to avoid a Gamma_P1 re-solve inside the movers' line-search backtrack; a DESIGN NOTE on BoundingSurface.normals records the re-solve-vs-cached trade-off + escape hatch. - Projects only OWNED slip vertices (parallel safety; ghosts via the movers' halo-sync). BoundingSurface rejects degenerate plane normals / bad radii. Movers: - mmpde / spring / ma / ot / anisotropic call mesh.boundary_slip(...) with the current coords as reference. spring/ma/anisotropic drop their per-ring rotation anchor (mmpde never had one; signed-area guards prevent tangle — only cosmetic azimuthal re-parameterisation). OT keeps its radial-only slip gating and now emits a DeprecationWarning (incomplete; superseded by mmpde + a scalar metric). Deletions: _build_slip_projector, _gamma_p1_at_vertices, _label_vertex_mask, _boundary_centre (no remaining callers). Validation: parity vs the deleted engine was machine precision (1.57e-16 annulus, 0.0 box); convection-harness A/B (serial 40-step + np=5) physics unchanged; 22 slip tests green. Documented follow-up (inline TODO): _pinned_mask misses 3D face-only labels (markVertices=False) -- a shared-helper limitation. Tests: test_0855 migrated to mesh.boundary_slip; test_0762 covers the facet fallback + degenerate-geometry rejection; new test_0763_boundary_slip_correctness locks exact-on-surface landing. Also fixes a latent missing `import warnings`. Underworld development team with AI support from Claude Code
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Architectural Review Submission
Review Type: Code Implementation / Testing / Documentation
Priority: MEDIUM
Review Period: 2026-06
📄 Review Document
docs/developer/design/boundary-slip-strategy.mddocs/reviews/YYYY-MM/REVIEW-TRACKING-INDEX.md📋 Executive Summary
This PR introduces the step-1 mesh-owned boundary tangent-slip interface via
BoundingSurfaceobjects andmesh.boundary_slip, without switching movers to consume it yet.Follow-up review feedback has been incorporated in this branch: geometry validation is now strict for radial/plane surfaces, and slip projection is restricted to owned vertices for parallel safety.
🎯 Scope
Systems Affected:
Files Changed:
✅ Testing Status
pixi run underworld-testTest Results:
📊 Key Metrics & Impact
Performance:
Quality:
🔍 Review Checklist (for Reviewers)
Design & Architecture:
Implementation:
Testing & Validation:
Documentation:
boundary_slip()still depends on shared_pinned_maskbehavior; 3D face-only labels (markVertices=False) are tracked separately as follow-up work in the shared helper.developmentdo not yet consume the new API (behavior-preserving step 1).🔗 Dependencies & Related Work
Depends on:
Blocks:
Related:
🖊️ Sign-Off
This PR merge represents formal approval of the architectural review.
Reviewers: Please review the full document at
docs/developer/design/boundary-slip-strategy.mdand approve this PR only when satisfied with the design, implementation, and documentation.📝 Additional Context
Review-driven hardening included in this branch:
BoundingSurfaceconstruction time.boundary_slip().project()to owned slip vertices; keepis_pinnedas full geometric classification for rank consistency.test_degenerate_geometry_raisesto lock geometry validation behavior.