Place a tip vertex from the face that holds it, not from the mesh at large - #545
Place a tip vertex from the face that holds it, not from the mesh at large#545lmoresi wants to merge 1 commit into
Conversation
…large pull_vertex_onto moved the nearest vertex in the whole mesh onto each target. That vertex need not belong to the cell containing the target: it can be the apex of a neighbouring cell, and moving it leaves the containing cell with no corner on the line. cut_along_lines then refuses the tip with "entered but not left" — for a tip that does have a vertex on it. The candidates are now the vertices of the smallest face whose closure contains the target: the three corners of the cell holding it, the two ends of the edge it lies on, or the vertex it coincides with. The face is read off the barycentric coordinates of one containing cell, which is what makes it right. Taking the union of the corners of every containing cell is not enough — a point on an edge lies in two cells and the union offers each cell's opposite apex, which covers one cell and not the other. That was the first attempt at this fix and it did not clear the measured case. Measured inside a ribbon zone of width 0.02: the target sat on an edge whose ends were at exactly 1.000e-02, being the ribbon's own edge vertices, and an apex won by 0.4% at 9.963e-03. Regular vertex rows at half-width make such near-ties the norm rather than bad luck, which is why this showed up in a zone and not on a plain mesh. The reduction now leads with an outside-flag so a rank holding no part of the containing cell cannot win with its own nearer vertex; all ranks flagging outside means the target is off the mesh, and the fallback is the previous nearest-anywhere behaviour. Closes #542. The regression test flattens the mesh first, because on a well-shaped mesh an edge midpoint's nearest vertex is one of that edge's own ends and the defect never appears; a negative control asserts the mesh really does present the case. Against the old rule the invariant fails at 38 of 71 midpoints tested. Underworld development team with AI support from Claude Code
Adversarial review1. The rule is geometric, and the first version of it was wrong. Restricting candidates to the corners of the containing cells is the obvious fix and it does not work: a point on an edge lies in two cells, and the union of their corners still offers each cell's opposite apex. On the measured case that apex still won on distance, and the cut still refused. The shipped rule reads the minimal face off the barycentric coordinates of one cell, which is both cheaper and correct. We are flagging this because "restrict to the containing cell" is what a reviewer would propose, and it is not sufficient. 2. The barycentric tolerance is a real choice. 3. Parallel is argued, not measured, for the case that matters. 4. The fallback is unchanged behaviour, deliberately. A target outside the mesh still takes the nearest vertex anywhere. That is the pre-existing contract and callers may rely on it, but it means a target just outside the boundary gets no containment guarantee at all — the guarantee this PR adds is conditional on the target being inside. 5. What the fix does not do. It guarantees every cell holding the target ends up with a corner on it. It does not guarantee the resulting mesh is well shaped: the chosen vertex can be the farther of two edge ends, and moving it that distance can flatten a cell. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a robustness issue in pull_vertex_onto() that could move a vertex from a neighbouring cell (rather than from the cell/edge that actually contains the target point), causing cut_along_lines() to reject otherwise-valid terminating tips (issue #542). The change makes vertex selection depend on the smallest containing face (triangle/edge/vertex) inferred from barycentric coordinates, and adds a regression test that reliably exposes the prior failure mode.
Changes:
- Add
_minimal_face_vertices()to identify the smallest mesh face whose closure contains a target point (via barycentric coordinates). - Update
pull_vertex_onto()to select candidate vertices from the containing face (with an MPI-safe fallback to global nearest when outside the mesh). - Add a regression test exercising edge-midpoint targets on a deliberately “flattened” mesh to ensure every containing cell ends up with a corner on the target.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/underworld3/utilities/line_cut.py |
Adds containing-face detection and updates pull_vertex_onto() to restrict candidate vertices accordingly. |
tests/test_0844_line_cut.py |
Adds a regression test to ensure pulled vertices cover all cells that contain the target. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| face = _minimal_face_vertices(X, cell_verts, t) | ||
| held = len(face) > 0 | ||
| candidates = face if held else np.arange(len(X)) | ||
| d = np.linalg.norm(X[candidates, :2] - t, axis=1) | ||
| # Reduced as (outside, distance, x, y) so both decisions ride one | ||
| # reduction: tuples compare lexicographically, so MIN prefers ANY rank | ||
| # that holds a containing cell over every rank that does not, then the | ||
| # closest corner, then the one lowest in coordinate order. The | ||
| # outside-flag has to lead: a rank with no part of the containing cell | ||
| # proposes its own nearest vertex, which can easily be closer than the | ||
| # right answer, and a reduction on distance alone would take it. | ||
| # | ||
| # All ranks flagging 1 means the target lies outside the mesh, and the | ||
| # fallback is the nearest vertex anywhere — the behaviour before the | ||
| # containment rule, kept because a target off the mesh still has to | ||
| # land somewhere. | ||
| local = ((0 if held else 1, float(d.min()), | ||
| *X[candidates[int(d.argmin())], :2]) | ||
| if d.size else (1, np.inf, np.inf, np.inf)) | ||
| _outside, _dist, tx, ty = uw.mpi.comm.allreduce(local, op=MPI.MIN) |
Closes #542.
pull_vertex_ontomoved the nearest vertex in the whole mesh onto each target. That vertex need not belong to the cell containing the target — it can be the apex of a neighbouring cell — and moving it leaves the containing cell with no corner on the line.cut_along_linesthen refuses the tip with "entered but not left", for a tip that does have a vertex on it.The measured case
Inside a ribbon zone of width 0.02, a fault trace starting at
(0.65, 0.88):The target sits exactly on the edge 2014–2015, whose ends are the ribbon's own edge vertices at exactly the half-width. Regular vertex rows at
±w/2make near-ties like this the norm inside a zone rather than bad luck, which is why it showed up there and not on a plain mesh — a tip swept across a cell of an ordinary box succeeded at 7 of 7 positions.The fix
Candidates are the vertices of the smallest face whose closure contains the target: the three corners of the cell holding it, the two ends of the edge it lies on, or the vertex it coincides with. The face is read off the barycentric coordinates of one containing cell.
Reading it from one cell is what makes it right. Taking the union of the corners of every containing cell is not enough: a point on an edge lies in two cells, and the union then offers each cell's opposite apex, which covers one cell and not the other. That was the first attempt here and it did not clear the measured case — the apex still won on distance.
In parallel the reduction leads with an outside-flag, so a rank holding no part of the containing cell cannot win with its own nearer vertex. All ranks flagging outside means the target lies off the mesh, and the fallback is the previous nearest-anywhere behaviour.
Why it matters
This is what limited the hybrid fault self-selection sweep to a subset of separations. With the fix,
selfselect.pyat DX=0.30 gets past the cut; it previously failed there while passing at 0.50 and 0.70.Tests
test_the_pulled_vertex_covers_every_cell_holding_the_targetasserts the invariant the cut actually needs — every cell whose closure contains the target ends up with a corner exactly on it — over interior edge midpoints.The mesh is flattened in y first, and that is load-bearing rather than cosmetic: on a well-shaped mesh an edge midpoint's nearest vertex is one of that edge's own ends, the defect never appears, and the test would pass without testing anything. A negative control (
exposed > 0) asserts the mesh really does present the case; the first version of this test, on an unflattened mesh, duly failed that control. Against the old rule the invariant fails at 38 of 71 midpoints tested.test_0844,test_0845,test_0846,test_0850_faults,test_0850_fault_network_toolkit,test_0853pass (102 passed, 1 pre-existing skip);ptest_0844andptest_0845pass at np=2 (20 passed).Underworld development team with AI support from Claude Code