fix(bc): rotated free-slip zero-datum guard compares by value (#336)#339
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes a Wave C API-shim regression in SNES_Stokes_SaddlePt.add_rotated_freeslip_bc where the “zero datum” guard used SymPy structural comparison and incorrectly rejected conds=0.0 (and other numeric-zero representations) in the canonical value-first call order.
Changes:
- Update the zero-datum guard to use value semantics via
sympy.sympify(conds).is_zero is not True, accepting all provable numeric zeros while rejecting unprovable symbolic data. - Add regression coverage ensuring numeric-zero forms are accepted without deprecation warnings and symbolic data still raises
NotImplementedError.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/test_0641_wave_c_api_shims.py | Adds regression tests covering numeric-zero acceptance and symbolic rejection for add_rotated_freeslip_bc in value-first order. |
| src/underworld3/cython/petsc_generic_snes_solvers.pyx | Fixes the rotated free-slip zero-datum guard to compare by value (is_zero) rather than structural equality. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Second commit added: the two Copilot review comments on #334 (shim TypeError said "positionally" on keyword calls; DeprecationWarning always described the legacy form as Underworld development team with AI support from Claude Code |
…mpy structure (#336) sympy's == is structural, so sympify(0.0) != Integer(0) is True and the canonical value-first call add_rotated_freeslip_bc(0.0, boundary) - the exact form the deprecation message recommends - raised NotImplementedError while conds=0 (int) worked. The guard now uses is_zero, which is True only when sympy can prove the datum is zero: every numeric zero form is accepted, and unprovable symbolic data is still rejected. Regression tests: 0.0 / Float(0) / S.Zero / Integer(0) all accepted warning-free; Symbol('a') still raises. Underworld development team with AI support from Claude Code
…t review of #334) The _value_first_bc_args TypeError claimed the datum was passed 'positionally' even on keyword-only calls, and the DeprecationWarning always described the legacy call as method(boundary, g=...) even when the shimmed form was the positional method(boundary, conds, ...). Each message now names the form the caller used; contract tests pin all three legacy shapes. Underworld development team with AI support from Claude Code
lmoresi
force-pushed
the
bugfix/rotated-zero-conds
branch
from
418e690 to
6a8994c
Compare
Closes #336.
The bug (found by Wave E's WE-09 sweep, filed as #336)
The Wave C shim's zero-datum guard in
SNES_Stokes_SaddlePt.add_rotated_freeslip_bcusedsympy.sympify(conds) != 0— a structural comparison, soFloat(0.0) != Integer(0)is True. The canonical value-first calladd_rotated_freeslip_bc(0.0, boundary)— the exact form the method's own deprecation message recommends — raisedNotImplementedError, whileconds=0(int) worked. Legacy boundary-first calls were unaffected (they passconds=None); the bug bit exactly the users adopting the migration #334 asks for.The fix
sympy.sympify(conds).is_zero is not True— value semantics.is_zerois True only when sympy can prove zero, so every numeric zero form (0.0,Float(0),S.Zero,Integer(0)) is accepted and an unprovable symbolic datum (Symbol('a')) is still rejected.Verification
test_0641(zero forms accepted warning-free; symbolic still raises) — shown failing against the unmodified build first (1 failed / 6 passed), 42/42 after.level_1 and tier_a: 371 passed / 10 skipped / 2 xfailed / 1 xpassed (baseline 369 + the 2 new tests; nothing lost). Cleanrm -rf build/pyx rebuild.Sequencing note
PR #338 (Wave E) carries a
TODO(BUG)marker on this same guard line — merge #338 first; I will refresh this branch over it (removing the marker) so the diff stays one logical change.Underworld development team with AI support from Claude Code