Uh oh!
There was an error while loading. Please reload this page.
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.
lmoresi
commented
Jul 6, 2026
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
418e690 to
6a8994cCompareUh oh!
There was an error while loading. Please reload this page.
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