fix(meshing): loose semantics on _test_if_points_in_cells_internal — accept on-face queries#207
Open
lmoresi wants to merge 1 commit into
Open
fix(meshing): loose semantics on _test_if_points_in_cells_internal — accept on-face queries#207lmoresi wants to merge 1 commit into
lmoresi wants to merge 1 commit into
Conversation
…accept on-face queries
`_test_if_points_in_cells_internal` used a strict `> 0` test on the
squared-distance difference between mirrored inner/outer control points
placed ±1e-3 along each face normal. A query exactly on a cell face has
zero distance difference and was rejected. That meant mesh vertices —
which by definition sit on the faces of every cell containing them in
their closure — failed the in-cell test for every candidate cell, and
`_get_closest_local_cells_internal` returned -1 for them.
For evaluation use cases (the dominant caller), this is wrong: the FE
basis at a shared vertex / face is consistent across the adjacent cells
(it's a DM consistency requirement of FE assembly), so "any cell whose
closure contains the point" is the right semantics. Picking one specific
adjacent cell and returning its id lets downstream code evaluate
correctly.
Add a `strict` keyword (default False) to `_test_if_points_in_cells_internal`:
- strict=False (new default): accept diff >= -1e-12. A point on a
face counts as inside the cell. The -1e-12 tolerance is well below
the 1e-3 control-point offset (test resolution) and well above
64-bit float roundoff. Use this for FE-evaluation hints,
points-in-domain queries, and similar "is this point in or on
this cell" checks.
- strict=True: preserve the previous `> 0` behaviour. Use this when
uniqueness matters — e.g. a strict-ownership partitioning scheme
where each point should be claimed by exactly one cell, and a
shared-face point being claimed by all adjacent cells would be a
bug.
Behavioural consequences:
- `_get_closest_local_cells_internal` now returns a valid cell id for
every vertex query (was returning -1 for ~43% of vertices on a
structured quad, ~71% on a 3D simplex — all of them on cell
boundaries by definition).
- `points_in_domain` (via the public `test_if_points_in_cells`
wrapper) reports boundary-vertex queries as "in the domain"
instead of "outside". This matches user intuition.
- Existing strict-mode behaviour is preserved by passing
`strict=True` explicitly.
New tests in `tests/test_0820_in_cell_test_loose_semantics.py` lock
the contract: loose default accepts vertices on 2D/3D simplex and 2D
quad meshes, strict explicit still rejects on-face queries, and the
returned cells genuinely contain the queries in their closure.
This change unblocks the bypass design in
`feature/dminterp-bypass-element-check` (PR #203), which needs an
authoritative per-rank cell-id source — `_get_closest_local_cells_internal`
with loose semantics gives exactly that.
Underworld development team with AI support from Claude Code (claude.com/claude-code)
6 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adjusts mesh cell-containment semantics so queries exactly on a cell face/edge/vertex are treated as “inside” by default. This fixes cases where mesh vertices (which lie on the closure of adjacent cells) were being rejected by _test_if_points_in_cells_internal, causing _get_closest_local_cells_internal to return -1 for valid vertex queries—impacting downstream evaluation and domain queries.
Changes:
- Added a
strict: bool = Falsekwarg toMesh._test_if_points_in_cells_internal()and to the publicMesh.test_if_points_in_cells()wrapper; loose mode accepts on-face queries via a small negative tolerance. - Updated the internal in-cell test implementation to branch between strict (
> 0) and loose (>= -1e-12) comparisons. - Added regression tests covering loose-default acceptance for vertex queries and strict-mode rejection, plus a closure-containment sanity check.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/underworld3/discretisation/discretisation_mesh.py |
Adds strict kwarg and implements loose vs strict face-containment thresholding; forwards the kwarg through the public wrapper. |
tests/test_0820_in_cell_test_loose_semantics.py |
Adds regression tests to lock in loose-default behavior and verify strict-mode distinction and closure containment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -3247,6 +3247,20 @@ | |||
| Coordinate array in any physical unit system (will be auto-converted) | |||
| @@ -3260,15 +3274,23 @@ | |||
| inside = numpy.ones_like(cells, dtype=bool) | |||
Comment on lines
3577
to
+3584
| points : array-like | ||
| Coordinate array in any physical unit system (will be auto-converted) | ||
| cells : array-like | ||
| Cell indices to test | ||
| strict : bool, default False | ||
| If True, points exactly on a cell face are reported as NOT in the | ||
| cell (useful when uniqueness matters). If False, points on the | ||
| closure of a cell count as in it (natural for FE evaluation). |
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.
Summary
Mesh._test_if_points_in_cells_internalused a strict> 0test on the squared-distance difference between mirrored inner/outer control points placed ±1e-3 along each face normal. A query exactly on a cell face has zero distance difference and was rejected. That meant mesh vertices — which by definition sit on the faces of every cell containing them in their closure — failed the in-cell test for every candidate cell, and_get_closest_local_cells_internalreturned-1for them.For evaluation use cases this is wrong: the FE basis at a shared vertex / face is consistent across the adjacent cells (DM consistency requirement of FE assembly), so "any cell whose closure contains the point" is the right semantics.
What's in the patch
src/underworld3/discretisation/discretisation_mesh.py— addstrict: bool = Falsekwarg to_test_if_points_in_cells_internal. Loose default accepts diff>= -1e-12; strict preserves the previous> 0. The publictest_if_points_in_cellswrapper forwards the kwarg.tests/test_0820_in_cell_test_loose_semantics.py— 5 tests locking the contract (loose default accepts vertices on 2D/3D simplex and 2D quad meshes; strict explicit still rejects on-face queries; returned cells genuinely contain the query in their closure).Net: +131 / -12 lines.
Empirical impact (probed locally before/after)
>0)>=-1e-12)Strict vs loose — when to use which
strict=True): callers that need uniqueness — strict-ownership partitioning where a shared-face point being claimed by all adjacent cells would be a bug.The strict→loose default change is observable for one caller:
points_in_domainnow reports boundary-vertex queries as "in the domain" instead of "outside". This matches user intuition (a point on the boundary of a closed domain IS in the closed domain).Why this matters
_get_closest_local_cells_internalis now an authoritative cell-id source — returns a cell whose closure contains the query, or -1 if no local cell does. That's the missing piece for PR #203 (feature/dminterp-bypass-element-check): the bypass needs a trustworthy hint and this provides it.Multi-rank ownership: a vertex on a partition seam will be claimed by both adjacent ranks under loose semantics. For FE evaluation this is harmless (the basis values agree at shared DOFs by DM consistency), so the
MPI_Allreduce(MIN, foundProcs)tie-break is correct. If a future use case needs strict ownership, callers can passstrict=True.Test plan
tests/test_0820_in_cell_test_loose_semantics.py— 5/5 passtests/test_0000_imports.py + test_0001_meshes.py + test_0004_pointwise_fns.py + test_0800_unit_aware_functions.py— 32/32 pass (no regressions)Underworld development team with AI support from Claude Code