Skip to content

Rotated and block constraints are exclusive; remove four dead label lookups - #590

Merged
lmoresi merged 2 commits into
developmentfrom
bugfix/rotated-guard-deadcode
Aug 17, 2026
Merged

Rotated and block constraints are exclusive; remove four dead label lookups#590
lmoresi merged 2 commits into
developmentfrom
bugfix/rotated-guard-deadcode

Conversation

@lmoresi

@lmoresilmoresi commented Aug 16, 2026

Copy link
Copy Markdown
Member

Closes#464. Closes#506.

Two independent fixes in the same boundary-condition code, both small and both
in petsc_generic_snes_solvers.pyx.

#464 — the two constraint mechanisms are now exclusive

add_rotated_freeslip_bc, add_fault_bc and add_constraint_bc could all be
called on one solver and nothing checked.

The rotated driver builds its own index-set fieldsplit over exactly two fields
(rotated_bc._solve_rotated_iterative), and build_rotation addresses velocity
and pressure by field number. A block constraint registers a multiplier field of
its own; those DOFs are in neither index set, so the preconditioner is built over
a strict subset of the operator's rows with nothing said about it. Both
mechanisms impose the same wall-normal condition, so the combination is a
configuration error rather than a case to support — supporting it would need a
third split for the multipliers and a build_rotation that knows about them.

_reject_mixed_constraint_mechanisms on SNES_Stokes_SaddlePt refuses whichever
call comes second, naming what is already registered and what was refused. The
fault path is covered too, since add_fault_bc reaches the same rotated driver.

The same check runs as the first statement of solve(). The registration checks
only cover what goes through the solver's own methods, and fault_contact writes
_fault_contact_faults directly rather than through add_fault_bc; the dispatch
is where both lists are read together. Registration gives the better message — it
knows which call was refused — and the dispatch gives the guarantee. Running it
first means an unsupported pair costs nothing before it is refused.

tests/test_0062_rotated_constraint_exclusivity.py covers both orderings, and
both single-mechanism controls: several rotated boundaries on one solver, and
several block constraints on one solver, are each still allowed. Without those a
guard that refused everything would pass.

The negative control is what shows there was something to fix. Neutering
_reject_mixed_constraint_mechanisms and repeating the first case:

guard neutered: accepted 1 rotated bc(s) and 1 multiplier(s) on one solver — no error

#506 — four dead label lookups

bc_label=self.dm.getLabel(boundary)
bc_is=bc_label.getStratumIS(value)

bc_is was assigned at four sites and read at none; bc_label at those sites
existed only to produce it. Nine lines removed. The issue's reason for wanting
them gone is that a stratum IS sitting beside a collective DS registration
invites a rank-local skip — getStratumIS returns nothing on a rank that holds
no part of the stratum, and the obvious next edit is to continue on that.

Verified

  • Full ./uw test: 1498 passed, 32 skipped, 2 xfailed — the branch baseline
    plus the five new tests.
  • No test, example or document calls both mechanisms on one solver;
    test_0641_wave_c_api_shims.py uses each on a different solver.

Underworld development team with AI support from Claude Code

…p dead label lookups
Two independent fixes in the same boundary-condition code.
#464. add_rotated_freeslip_bc, add_fault_bc and add_constraint_bc could all
be called on the same solver and nothing checked. The rotated driver builds
its own index-set fieldsplit over velocity and pressure and build_rotation
addresses them by field number, so a block constraint's multiplier DOFs are
in neither index set and the preconditioner is built over a strict subset of
the operator's rows. The two impose the same wall-normal condition by
different means, so _reject_mixed_constraint_mechanisms refuses the
combination at whichever call comes second, naming both.
test_0062 covers both orderings and, as controls, that several rotated
boundaries and several block constraints are each still allowed. With the
guard neutered the solver accepts one rotated boundary condition and one
multiplier together and says nothing, which is what the issue reports.
#506. bc_is = bc_label.getStratumIS(value) was assigned at four sites in
petsc_generic_snes_solvers.pyx and read at none; bc_label at those sites
existed only to produce it. A stratum IS beside a collective DS registration
invites a rank-local skip, which is why the issue asks for it to go.
Closes#464, #506.
Underworld development team with AI support from Claude Code
CopilotAI lite review requested due to automatic review settings August 16, 2026 12:20

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@lmoresi

Copy link
Copy Markdown
MemberAuthor

Adversarial review

Reviewed at e956fcc. Three findings, one of them worth a decision before merge.

1. The guard is registration-time only, so it can be bypassed. It fires from
add_rotated_freeslip_bc, add_fault_bc and add_constraint_bc. Anything that
appends to _rotated_freeslip_bcs, _fault_contact_faults or _multipliers
without going through those methods is unguarded, and fault_contact.py writes
solver._fault_contact_faults directly (line ~108) rather than through the
solver API. Today add_fault_bc is the only caller, so the guard holds; it holds
by convention, not by construction. The construction-safe version puts the same
check in the solve dispatch, where both lists are read together
(petsc_generic_snes_solvers.pyx ~9364). We would rather it were there as well
as at registration — registration gives the better message, the dispatch gives
the guarantee.

2. RuntimeError may be the wrong class. This is a caller passing an
unsupported combination of configuration, which is closer to TypeError /
ValueError than to a runtime fault; add_rotated_freeslip_bc raises
TypeError a few lines later for a bad datum shape. We chose RuntimeError
because the objection is about solver state rather than about the arguments of
this call, but the tests match on the message, so the class is easy to change if
the Charter reading is the other one.

3. Removing the dead lookups changes nothing observable, which is also the
limit of the evidence.
bc_is had no reader, so deletion is safe by
inspection, and the suite is green — but a green suite is not evidence about a
line that never did anything. The claim this PR can support is "assigned at four
sites, read at none"; the claim it cannot support is that getStratumIS was
free. It allocates an IS per natural BC per setup, and we did not measure that.

Checked and clean: nothing in tests/, docs/ or the examples calls both
mechanisms on one solver, so no existing configuration is refused by this change
test_0641_wave_c_api_shims.py matched the search but uses each mechanism on
a different solver, in a different test.

Underworld development team with AI support from Claude Code

The registration checks only cover what goes through the solver's own
methods, and fault_contact writes _fault_contact_faults directly rather than
through add_fault_bc. The dispatch is where both lists are read together, so
it carries the check as well: registration gives the better message, the
dispatch gives the guarantee.
It runs as the first statement of solve(), before any setup touches either
list, so an unsupported pair costs nothing before it is refused.
Underworld development team with AI support from Claude Code
@lmoresi

Copy link
Copy Markdown
MemberAuthor

Finding 1 above is addressed at aa2b8d8 rather than left as a note: _reject_mixed_constraint_mechanisms("solve") now runs as the first statement of solve(), so the pair is refused even when registration was bypassed. test_0062 gained a case that appends to _multipliers directly and asserts the solve refuses it.

Findings 2 and 3 stand as written.

Underworld development team with AI support from Claude Code

@lmoresi
lmoresi merged commit a24c558 into developmentAug 17, 2026
2 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@lmoresi