Take slip normals per boundary, not from the deprecated Gamma_P1 (#538) - #659
Open
lmoresi wants to merge 1 commit into
Open
Take slip normals per boundary, not from the deprecated Gamma_P1 (#538)#659lmoresi wants to merge 1 commit into
lmoresi wants to merge 1 commit into
Conversation
`node_redistribution(..., slip_surfaces=True)` slid nodes on Right and Top and left Left and Bottom at EXACTLY zero displacement. With a metric band placed symmetrically about the domain centre: left 0.00000 / right 0.18241, bottom 0.00000 / top 0.18427. A symmetric problem with an asymmetric answer. `_slip_normals` evaluated `mesh.Gamma_P1`. That field is deprecated, and its own docstring says off-kernel evaluation "falls back to a coordinate-based direction" rather than a normal. On a unit box it returns the edge TANGENT on Left and Bottom -- (0, 1) where the outward normal is (-1, 0) -- and drifts up to 40 degrees along Right and Top. A node whose "normal" lies along its own edge has its tangential motion projected out, which is why those two walls could not move at all. Now the normal is assembled PER BOUNDARY. Each boundary's field is supported only on that boundary and zero elsewhere, so summing over the slip set gives each node its own normal, and a node claimed by more than one boundary falls out as a corner. Measured on a unit box: exact on all four walls to 8e-19, all four corners correctly unusable, and the symmetric band now gives left 0.03838 / right 0.03952 and bottom 0.03906 / top 0.03506. Corners are now pinned by CONSTRUCTION (claims != 1) rather than by hoping opposing normals cancel to under the 0.5 magnitude threshold. NO NEW MeshVariable is created, and that constraint is load-bearing rather than tidiness. Creating even ONE extra variable on this path restructures the DM and the parent's static hierarchy stops matching what it recorded, failing test_0764::test_redistribute_then_adapt_composition. Three attempts went wrong before that was understood -- pre-touching the fields earlier, then reusing a single scratch variable -- and it was settled by keeping the creation while reverting to the OLD normals, which still failed. So the creation is the trigger, not the normals. The per-boundary normal is therefore assembled into `_n_proj`, the field `Gamma_P1` already owns; `_update_projected_normals` rebuilds it on every call, so overwriting it is safe. Composite labels (`All_Boundaries`) are never iterated: one would contribute at every boundary node and make every node look like a corner. Cost: one evaluation per label, ~14x the single Gamma_P1 call it replaces (0.0041 s -> 0.0590 s over 5 labels, 64 nodes). The old call was cheap and wrong. Callers that know their slip set should pass it rather than paying for labels that cannot slip. The regression asserts all four walls, not one -- the defect was confined to two of them, and a test on Right or Top alone passes throughout -- plus the corner-pinning contract. 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.
Fixes #538.
The symptom
node_redistribution(..., slip_surfaces=True)slid nodes on Right and Top and left Left and Bottom at exactly zero displacement. With a metric band placed symmetrically about the domain centre:A symmetric problem with an asymmetric answer.
The cause
_slip_normalsevaluatedmesh.Gamma_P1, which is deprecated and whose own docstring says off-kernel evaluation "falls back to a coordinate-based direction" rather than a normal. On a unit box it returns the edge tangent on Left and Bottom —(0, 1)where the outward normal is(-1, 0)— and drifts up to 40 degrees along Right and Top. A node whose "normal" lies along its own edge has its tangential motion projected out, so those two walls could not move at all.The fix
Assemble the normal per boundary. Each boundary's field is supported only on that boundary and zero elsewhere, so summing over the slip set gives every node its own normal, and a node claimed by more than one boundary falls out as a corner.
claims != 1) rather than by hoping opposing normals cancel below a magnitude thresholdAll_Boundaries) are never iterated: one would contribute at every boundary node and make every node look like a cornerThe constraint that took three attempts
No new
MeshVariableis created, and that is load-bearing. Creating even one extra variable on this path restructures the DM, and the parent's static hierarchy stops matching what it recorded —test_0764::test_redistribute_then_adapt_composition. Pre-touching the fields earlier failed; a single reusable scratch variable failed too.What settled it was an experiment rather than more reading: keep the creation, revert to the OLD normals — still failed. So creation is the trigger, not the normals. The per-boundary normal is therefore assembled into
_n_proj, the fieldGamma_P1already owns._update_projected_normalsrebuilds it on every call, so overwriting is safe.Cost, stated plainly
One evaluation per label: ~14x the single
Gamma_P1call it replaces (0.0041 s → 0.0590 s over 5 labels, 64 nodes). The old call was cheap and wrong.boundaries=lets a caller pass just its slip set rather than paying for labels that cannot slip.Tests
The regression asserts all four walls, not one — the defect was confined to two of them, and a test on Right or Top alone passes throughout — plus the corner-pinning contract.
test_0764_node_redistribution: 10 passed (was 8 passed + 1 regression during development)-k "smooth or relax or mmpde or redistrib or bounding"): 105 passedtest_0855_mesh_smoothing_parallelat np=2: greenlevel_1 and tier_a: 1073 passed, 0 failedRelated: #654 stops the mesh factories recommending
Gamma_P1for this purpose. After both land, the only remaining in-tree consumer ofGamma_P1is its own machinery.Underworld development team with AI support from Claude Code