Uh oh!
There was an error while loading. Please reload this page.
Fix the red trunk: inverted inside-test in the fault-network line clip, and a gmsh finalize cascade - #533
Merged
Merged
Conversation
…etected on every LAPACK build crossing_segment missed genuine X crossings whenever a polygon edge was EXACTLY parallel to the plane-plane intersection line: the parallel-edge branch of _line_clip_interval returned "outside" for num < 0, but for an interior point num = nrm.(A - p0) is negative. CI's OpenBLAS returns exact zeros for axis-aligned plane bases, so den == 0.0 hit the inverted branch and the 0851 fault-network tests refused with a bogus near-miss (and the BoxInternalPatch crossing pre-check let a raw tetgen PLC error escape); macOS Accelerate left ~1e-17 jitter in den and took the correct division branch by luck, which is why the tests only failed on CI. Adds a deterministic regression test with axis-aligned patches (den is exactly zero on every platform): a genuine crossing must be found, and a parallel-edge non-crossing must still return None. Underworld development team with AI support from Claude Code
…mesh cannot poison every later one
BoxInternalPatch called gmsh.finalize() only on success; an escaped
mesher exception (e.g. a tetgen PLC error on a crossing patch set) left
the session initialized with the failed model, and the NEXT mesh
constructor in the same process then wrote an invalid .msh that PETSc
rejects with error 79 ("expecting $Nodes"). This is exactly how the CI
crossing-detection miss in test_0851_fault_network_3d took down the
unrelated test_0851_std_reduction_method mesh build two tests later.
generate/write now run under try/finally. TODO(BUG) notes the remaining
fault-session follow-up: patch sets the crossing pre-check cannot
classify (coplanar overlap, edge touching) can still reach tetgen raw.
Underworld development team with AI support from Claude CodeContributor
There was a problem hiding this comment.
Pull request overview
Fixes CI-only failures in the 3-D fault network meshing/tests by correcting an inverted inside-test in the convex line-clipping routine (triggered when an edge is exactly parallel to the plane–plane intersection line), and by ensuring gmsh is finalized even when meshing/write fails to prevent cross-test session poisoning.
Changes:
- Fix
_line_clip_interval()parallel-edge inside/outside classification infault_network_3d.py. - Add a deterministic regression test covering the exact-parallel-edge case.
- Wrap gmsh mesh generation / write in
try/finallyto guaranteegmsh.finalize()on failure.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
tests/test_0851_fault_network_3d.py | Adds a regression test that deterministically exercises the exact-parallel-edge branch and asserts the analytic crossing segment. |
src/underworld3/meshing/fault_network_3d.py | Fixes the parallel-edge “inside” test in the line clip interval computation to avoid missing genuine crossings. |
src/underworld3/meshing/cartesian.py | Adds try/finally around gmsh generate/write so mesher failures don’t leave gmsh initialized and poison later meshes. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1225
to
+1229
| try: | ||
| gmsh.model.mesh.generate(3) | ||
| gmsh.write(uw_filename) | ||
| finally: | ||
| # A mesher failure must not leave the gmsh session |
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Fix the four CI-only test_0851 failures: one inverted sign, one missing finally
All four CI failures (Linux, conda PETSc 3.25.3) trace to a single source
bug plus one missing cleanup — not to geometry constants in the tests.
What was environment-dependent
_line_clip_interval(fault_network_3d.py) had an inverted inside test inits parallel-edge branch: for an interior point,
num = nrm.(A - p0)isNEGATIVE, but the code returned "outside" on
num < 0. The branch onlyfires when an edge is EXACTLY parallel to the clipped line (
den == 0.0,guard 1e-30). CI's OpenBLAS returns exact zeros for the axis-aligned
patch's SVD plane basis, so
den == 0.0and the inverted branch madecrossing_segmentreport "no crossing" for genuinely crossing patches.macOS Accelerate leaves ~1e-17 jitter in
den, dodging the branch intothe sign-correct division path — the dev boxes passed by luck. Verified
by probe: with the module's own code, an exactly axis-aligned crossing
pair returns None pre-fix on macOS too.
Per-test diagnosis
test_preparer_trims_junior_and_records_segmentandtest_network_3d_end_to_end:crossing_segment(Main, Cross)returnedNone on CI, so the preparer fell through to the near-miss ligament
refusal (dmin 0.02233 is the true rim-rim distance of the test pair —
correct data, wrong branch). Fixed by the sign fix.
test_embed_refuses_crossing_patches:BoxInternalPatch's crossingpre-check uses the same
crossing_segment; the miss let the geometryreach gmsh/tetgen, whose raw PLC error escaped. Fixed by the sign fix
(the designed ValueError now fires on every platform).
test_mesh_simple_array_view_std(PETSc error 79): a CASCADE from 2.The escaped PLC exception aborted
BoxInternalPatchaftergmsh.initialize()and beforegmsh.finalize(); the poisoned sessionmade the next
StructuredQuadBox(first test of the next file in thesame
test_08*pytest batch) write an invalid .msh, which PETScrejects: error 79, "expecting
macOS by blinding the crossing check and replaying the sequence.
Fixed by
try/finally gmsh.finalize()around generate/write.The std test itself is sound (no solver is involved; the error was at
mesh file read) and is unchanged.
How the tests now pin the property
New
test_crossing_segment_exact_parallel_edges: axis-aligned patchesmake
denexactly 0.0 on EVERY platform — the CI condition becomesdeterministic everywhere. Asserts the crossing segment analytically
(x=0.42, y=0.5, z in [0.32, 0.68]).
Negative controls
(probe-verified) — the test genuinely detects the bug on all platforms.
outside the square) that must STAY None — guards against a blanket
sign inversion.
test_preparer_refusals(near-miss NotImplementedError, non-convexrefusal) still passes — the refusal arms still fire.
error still escapes but
gmsh.isInitialized() == 0afterwards and thefollow-on
StructuredQuadBoxsucceeds (pre-fix: exact CI error 79).Verification (sequential, worktree amr-dev)
test_0851_fault_network_3d.py test_0851_std_reduction_method.py test_0851_surface_influence_edge.py: 17 passed.test_0845_fault_split.py test_0846_fault_contact.py test_0847_fault_api.py test_0848_fault_split_3d.py: 29 passed(unchanged set, stays green).
Remaining for the fault session
Patch sets the crossing pre-check cannot classify — coplanar overlaps,
patches touching along an edge/point — can still reach tetgen and fail
with a raw PLC error instead of the embed's own message. TODO(BUG) is
placed at the
generate(3)call inBoxInternalPatch. Thefinallymeans such a failure can no longer poison later meshes, but the refusal
message should become the preparer's own.
Underworld development team with AI support from Claude Code