Remove the Null_Boundary fake natural BC and the label it was built on - #503
Remove the Null_Boundary fake natural BC and the label it was built on#503lmoresi wants to merge 3 commits into
Conversation
…ated nothing
Every solver with a natural BC quietly gained one it never asked for, on a
label marking every vertex of the mesh, justified by a comment that named no
PETSc version, no error and no test:
a workaround for some problem in the PETSc machinery where we need a
surface integral term somewhere on every process if we have a
contribution from anywhere
Measured on the case that describes — a Neumann condition on one face of a long
flat box, at rank counts where up to 7 of 8 ranks own no facet of it, checked
against a manufactured solution rather than against convergence, because a
silently dropped surface term still converges, just to the wrong field. The
solution is x^2, which P2 reproduces exactly, and the solver tolerance is
tightened to 1e-10, so the error sits at the tolerance and a missing term is
unmissable rather than buried under discretisation error.
The answers are bit-identical to every digit, with and without the workaround,
at np = 1, 2, 3, 4, 6 and 8. A negative control that zeroes the flux term —
what a dropped surface integral amounts to — moves the error from 3.6e-11 to
8.0e-03, so the probe demonstrably can fail. Boundary integrals over the same
geometry are byte-for-byte identical too.
Why there was nothing to fix: natural BCs are registered by
PetscDSAddBoundary_UW against the consolidated UW_Boundaries label, with the
boundary's own value, unconditionally on every rank. There is no rank-local
skip, so the DS boundary list was already identical everywhere. (The
bc_is = bc_label.getStratumIS(value) alongside it is dead — computed, never
read.) And the fake BC could not have contributed anyway: value 666 marks 198
points on a test mesh, all of them VERTICES and none a facet, measured after a
solve, so PETSc never completes it into something integrable.
What it did do was mislead. Reading labelled points to find material interfaces
saw every vertex of every UW3 mesh labelled and silently refused 1114 of 1114
mesh-repair candidates.
test_stokes_natural_bc_constant_no_recompile asserted the workaround's presence
— it was guarding an older bug where _build() re-added the BC on every call,
resetting is_setup and forcing a recompile. That bug class cannot exist once
nothing is manufactured, so the assertion becomes its opposite: a solver's
natural_bcs holds exactly what the user asked for. The test's real subject, that
changing a natural-BC constant does not recompile, is untouched and still passes.
Probes and logs: ~/+Simulations/null_boundary_bc_hack/
Underworld development team with AI support from Claude Code
…misleads Every UW3 mesh carried a label marking EVERY VERTEX in its chart with the reserved value 666. It marks no facet, so it integrates nothing, and its only functional consumer — a fake natural BC the solver manufactured — was removed in the previous commit, measured to change no answer at any rank count. What it cost is legibility. A label's presence on a point says nothing about whether it means a material interface, and this label put a blanket over the whole vertex stratum: a mesh-repair pass that looked for interfaces by reading labelled points saw every vertex labelled and silently declined 1114 of 1114 candidates. Nothing errored; the feature was simply a no-op on every real mesh. Removed from the two injected boundaries enums, from the label-creation block, and from segmented.py's own enum. KEPT DELIBERATELY: the sentinel skips in graph.py, discretisation_mesh.py and adaptivity.py. A caller may still supply a boundaries enum declaring Null_Boundary = 666 (tests/test_1012_stokesImportedDMPlex and several docs/examples do), and a mesh reloaded from an older checkpoint still carries both the label and the enum member. Downstream readers therefore cannot assume it is absent, and _dm_unstack_bcs restores it only when the enum names it, so it becomes a no-op for our meshes and stays correct for a caller's. Backward compatibility measured rather than assumed: a checkpoint written before the removal reads at np=1 and np=3 with area exactly 1.0000000000, keeping its own label and enum member from the file. Old data keeps its labels; new meshes stop manufacturing one. Verification, against a baseline recorded before either commit: all three probe sweeps byte-for-byte identical at np = 1, 2, 3, 4, 6, 8; 93 serial tests pass, including the caller-declared-enum case; 23 parallel tests pass at np=2 and np=4, including test_0766_box_internal_boundary_mpi — the issue-#162 case, where losing these labels once made BoxInternalBoundary's natural BCs contribute nothing. The heavy solver suite matches baseline exactly, down to the same single pre-existing unrelated failure. test_null_boundary_marks_every_vertex becomes test_no_null_boundary_label_is_manufactured, which also pins the distinction from All_Boundaries: the two are not synonyms and only one of them is removable. All_Boundaries (1001) is a real geometric boundary — every exterior FACET, from markBoundaryFaces — and is itself one of the values inside UW_Boundaries, the single consolidated label the DS is pointed at. Measured on a unit box: All_Boundaries 32 edges / 0 verts, Null_Boundary 0 edges / 98 verts. The absence is asserted against the DM's list of label NAMES, not against `getLabel(...) is None`: getLabel returns a non-None DMLabel wrapper with a null handle for a name the DM does not have, so the `is None` form reports every absent label as present. Same family as issue #291. Probes and logs: ~/+Simulations/null_boundary_bc_hack/ Underworld development team with AI support from Claude Code
The removal of the fake Null_Boundary natural BC is pinned by tests; the claim that it was SAFE to remove was pinned by nothing. No test in the repo, before or after, exercises a natural BC on a boundary some rank owns no part of. The only parallel test that creates a natural BC at all is test_0770_submesh_extract_mpi, and there every rank owns part of both natural boundaries (Internal [26,26] at np=2, [12,14,13,13] at np=4). A long flat box (4.0 x 0.25) gets the discriminating case reliably: the partitioner cuts along x, so the short end faces land on one rank each. Measured owned facets of Right: [5,0] at np=2, [0,0,5,0] at np=4, [5,0,0,0,0,0,0,0] at np=8 -- 7 of 8 ranks owning none. Four tests: a vacuity guard that asserts some rank really does own none (without it a partitioner change makes the rest pass for no reason), a BdIntegral case, a scalar Poisson natural BC against the P2-exact manufactured solution T = x^2, and a vector Stokes traction asserted by difference rather than against a stored number. Both negative controls fire. Dropping the scalar flux term moves the relative L2 error from 3.6e-11 to 8.010e-03. Dropping the vector traction makes the two Stokes solves agree to every digit (2.582851221410e-01), which is what a term that never reached the residual looks like. This is a tripwire on current behaviour, not a proof that the PETSc problem the workaround was aimed at is gone -- removing a failed fix does not establish that. The live hazard it guards is the dead `bc_is = bc_label.getStratumIS(value)` sitting beside the DS registration loop, which invites an `if bc_is is None: continue` optimisation that would reintroduce exactly the failure the workaround claimed to prevent. Underworld development team with AI support from Claude Code
Adversarial review —
|
|
|
There was a problem hiding this comment.
Pull request overview
This PR removes the legacy Null_Boundary sentinel label (value 666) and the solver-side “fake” natural BC that was being injected when any natural BCs were present, and adds a parallel regression test to ensure natural BC assembly and boundary integrals behave correctly even when some ranks own no facets of the target boundary.
Changes:
- Remove the solver workaround that auto-added a zero traction natural BC on
Null_Boundary. - Stop manufacturing the
Null_Boundarylabel/member on newly constructed meshes (including segmented meshes). - Add an MPI regression test covering natural BCs and
BdIntegralon boundaries that some ranks do not own.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_1001_poisson_constants.py | Updates the Stokes constant-traction natural-BC test to assert no manufactured Null_Boundary BC; docstring updated. |
| tests/test_0001_meshes.py | Changes mesh test from asserting presence of Null_Boundary to asserting its absence and validating All_Boundaries facet marking. |
| tests/parallel/test_0768_unowned_boundary_natural_bc_mpi.py | Adds MPI “unowned boundary” regression coverage for boundary integrals, scalar Poisson natural BCs, and vector Stokes traction. |
| src/underworld3/meshing/segmented.py | Removes Null_Boundary = 666 from the segmented mesh boundary enum. |
| src/underworld3/discretisation/discretisation_mesh.py | Removes Null_Boundary enum patching and DM label construction from mesh initialization path; updates docstring accordingly. |
| src/underworld3/cython/petsc_generic_snes_solvers.pyx | Removes the solver _build() block that injected a fake natural BC on Null_Boundary. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if uw.mpi.size > 1: | ||
| _nroots, ilocal, iremote = dm.getPointSF().getGraph() | ||
| ghosts = (set(range(0 if iremote is None else len(iremote))) | ||
| if ilocal is None else set(int(p) for p in ilocal)) | ||
|
|
| all_bd = mesh.dm.getLabel("All_Boundaries") | ||
| assert all_bd is not None |
| recompile. That fake BC has since been removed altogether — it was a | ||
| workaround for a PETSc problem that no longer exists, and it integrated | ||
| nothing (value 666 marks vertices, never a facet). So the assertion is now |
|
The dead |
What this removes
Two things, in two commits, plus a test.
866ae55d— the fake natural BC. Whenever a solver had any natural BC, itmanufactured one more on
Null_Boundary, commented as "a workaround for someproblem in the PETSc machinery where we need a surface integral term somewhere
on every process if we have a contribution from anywhere".
feb86401— the label itself.Null_Boundary, value 666, was added to everyUW3 mesh at construction.
2aec7923—tests/parallel/test_0768_unowned_boundary_natural_bc_mpi.py,which pins the case the workaround claimed to need.
The measured basis
Value 666 marks no facet. Measured after a solve on a unit box: 198 points,
all vertices, zero edges. A boundary residual attached to a value that marks no
facet assembles over nothing, so the fake term was identically zero. The source
comment said as much ("integrates nothing over a bunch of points") without
following the thought through.
The DS was already rank-identical. Natural BCs are registered against the
consolidated
UW_Boundarieslabel unconditionally on every rank —PetscDSAddBoundary_UWruns in a loop with no rank-local skip. So the hack couldnot have been fixing a rank-consistency problem. The one thing it could still
have supplied was a locally non-empty stratum on every rank, and that is what the
probes were built to test.
Answers do not move. A long flat box (4.0 x 0.25) makes the partitioner cut
along x, so the short end faces land on one rank each and everyone else owns
none of them. With a natural BC on such a face, results are bit-identical np=1
through np=8 with up to 7 of 8 ranks owning no facet of it: scalar Poisson
against the P2-exact
T = x²attolerance = 1e-10, aBdIntegralsweep(byte-for-byte identical,
diffclean), and Stokes traction and Nitsche free-slipfor the vector path the fake condition
(0,)*u.shape[1]was actually shaped for.Old checkpoints still read. A file written before the removal loads at np=1
and np=3, area exactly
1.0000000000, keeping its ownNull_Boundarylabel andenum member from the file. Old data keeps its labels; new meshes simply stop
manufacturing one. That is also why the sentinel skips stay — a reloaded mesh
still carries
Null_Boundaryin its boundaries enum, as do several caller enumsin
docs/examplesandtest_1012.What the test coverage actually is
Please do not read the parallel-suite pass count as evidence for this change. Of
the parallel tests run, exactly one existing file creates a natural BC at all
(
test_0770_submesh_extract_mpi), and in it every rank owns part of both naturalboundaries —
Internal[26,26] at np=2 and [12,14,13,13] at np=4. Ranks owningnone: zero.
test_1064_rotated_freeslip_paralleluses a strong rotatedconstraint, which never populates
natural_bcs, so it never entered the removedbranch. Those tests are regression cover that nothing else broke. They say
nothing about this.
The new file is the coverage. Owned facets of
Right, ranks owning none: 1 of 2,2 of 3, 3 of 4, 5 of 6, 7 of 8. It carries a vacuity guard, because without
one a partitioner change would make every other test in it pass for no reason.
Both its negative controls were run and fire: dropping the scalar flux term moves
the relative L2 error from 3.6e-11 to 8.010e-03, and dropping the vector traction
makes the two Stokes solves agree to every digit.
It is a tripwire on current behaviour, not a proof that the PETSc problem the
workaround was aimed at is gone. Removing a failed fix does not establish that
the thing it failed to fix has gone away.
Hazard left in place, deliberately
Beside the DS registration loop sits
computed and never read. That dead line invites
if bc_is is None: continueasan apparent optimisation, which would reintroduce precisely the failure the
workaround claimed to guard against — and, before this PR, with every test still
green. It is also a latent issue-#291 segfault that never fires only because it
is never dereferenced. Out of scope here; flagged so it is not lost.
Where this came from
Found while landing the mesh DELETE primitive.
Null_Boundarymarks every vertexof every mesh and
UW_Boundariesre-packs it, so reading labelled points asmaterial interfaces refused 1114 of 1114 mesh-repair candidates, silently. That
fix (read edges, not points) is independent and already landed; this is the
upstream cleanup.
Verification
test_1012_stokesImportedDMPlexand the smoothingtests that skip the sentinel;
test_0766_box_internal_boundary_mpi— the issue-[BUG] The natural boundary condition is not working on the internal boundary #162 case, where losing theselabels once silenced
BoxInternalBoundary's natural BCs;sweep:
test_1065_nitsche_local_h,test_0641_wave_c_api_shims, 48 passed;Full logs and probe scripts under
~/+Simulations/null_boundary_bc_hack/.Underworld development team with AI support from Claude Code