From 92737609e52fb11ef798f0174f162a79479c0b09 Mon Sep 17 00:00:00 2001 From: lmoresi Date: Mon, 6 Jul 2026 19:01:10 +0100 Subject: [PATCH 1/2] fix(bc): add_rotated_freeslip_bc zero guard compares by value, not sympy 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 --- .../cython/petsc_generic_snes_solvers.pyx | 13 ++++--------- tests/test_0641_wave_c_api_shims.py | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/underworld3/cython/petsc_generic_snes_solvers.pyx b/src/underworld3/cython/petsc_generic_snes_solvers.pyx index 3152b4244..51b17d707 100644 --- a/src/underworld3/cython/petsc_generic_snes_solvers.pyx +++ b/src/underworld3/cython/petsc_generic_snes_solvers.pyx @@ -5400,15 +5400,10 @@ class SNES_Stokes_SaddlePt(SolverBaseClass): raise TypeError( f"add_rotated_freeslip_bc() requires a boundary label string; " f"got {type(boundary).__name__}") - # TODO(BUG): float zero is rejected as "non-zero" by this guard. - # sympy's == is structural, so sympy.sympify(0.0) != 0 evaluates True - # (Float(0.0) is not Integer(0)): the canonical value-first call - # add_rotated_freeslip_bc(0.0, boundary) — the very form this method's - # own deprecation message recommends — raises NotImplementedError, - # while conds=0 (int) and conds=None work. The guard should test - # value semantics, e.g. `sympy.sympify(conds).is_zero is not True`. - # Found by the WE-09 call-site sweep, 2026-07-07 (Wave C shim, #334); issue #336. - if conds is not None and sympy.sympify(conds) != 0: + # Value comparison, not sympy's structural ==: Float(0.0) != Integer(0) + # structurally, but both are zero data. is_zero is True only when sympy + # can PROVE zero, so unprovable symbolic data is rejected too (#336). + if conds is not None and sympy.sympify(conds).is_zero is not True: raise NotImplementedError( "add_rotated_freeslip_bc imposes u.n = 0 only; a non-zero " "wall-normal datum is not implemented (use add_nitsche_bc or " diff --git a/tests/test_0641_wave_c_api_shims.py b/tests/test_0641_wave_c_api_shims.py index 5c8015e4f..60851e884 100644 --- a/tests/test_0641_wave_c_api_shims.py +++ b/tests/test_0641_wave_c_api_shims.py @@ -130,10 +130,24 @@ def test_legacy_positional_normal_is_preserved(self, mesh, stokes): assert _one_deprecation(rec) == 1 assert stokes._rotated_freeslip_bcs[-1] == ("Top", n) + def test_zero_datum_accepted_in_any_numeric_form(self, mesh, stokes): + # Regression (#336): the zero guard must compare by VALUE. sympy's + # structural == treats Float(0.0) != Integer(0), so 0.0 was rejected + # even though the deprecation message itself recommends conds=0. + for zero in (0.0, sympy.Float(0), sympy.S.Zero, sympy.Integer(0)): + with _no_deprecation(): + stokes.add_rotated_freeslip_bc(zero, "Top") + assert stokes._rotated_freeslip_bcs[-1] == ("Top", None) + def test_nonzero_datum_not_implemented(self, mesh, stokes): with pytest.raises(NotImplementedError): stokes.add_rotated_freeslip_bc(1.0, "Top") + def test_symbolic_possibly_nonzero_datum_not_implemented(self, mesh, stokes): + # An expression sympy cannot prove zero must be rejected, not let through. + with pytest.raises(NotImplementedError): + stokes.add_rotated_freeslip_bc(sympy.Symbol("a"), "Top") + class TestConstraintBCValueFirst: def test_new_order_no_warning(self, mesh): From 6a8994c151ee4d18a679e617bd6e0d26b4362f1b Mon Sep 17 00:00:00 2001 From: lmoresi Date: Mon, 6 Jul 2026 21:27:13 +0100 Subject: [PATCH 2/2] fix(bc): shim messages describe the legacy form actually used (Copilot 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 --- .../cython/petsc_generic_snes_solvers.pyx | 21 +++++++++++++------ tests/test_0641_wave_c_api_shims.py | 14 ++++++++++++- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/underworld3/cython/petsc_generic_snes_solvers.pyx b/src/underworld3/cython/petsc_generic_snes_solvers.pyx index 51b17d707..2d6a7f844 100644 --- a/src/underworld3/cython/petsc_generic_snes_solvers.pyx +++ b/src/underworld3/cython/petsc_generic_snes_solvers.pyx @@ -1626,22 +1626,31 @@ class SolverBaseClass(uw_object): Returns the normalized ``(conds, boundary)`` pair; ``boundary`` is required to be a string on exit. """ - legacy = False + legacy_order = False + legacy_alias = False if isinstance(conds, str) and not isinstance(boundary, str): conds, boundary = boundary, conds - legacy = True + legacy_order = True if alias is not None: if conds is not None: raise TypeError( f"{method}() received the boundary datum twice " - f"(positionally and as '{alias_name}='); pass it once, " + f"(as 'conds' and as '{alias_name}='); pass it once, " f"as 'conds'") conds = alias - legacy = True - if legacy: + legacy_alias = True + if legacy_order or legacy_alias: + # Name the legacy form the caller actually used (Copilot review + # of #334: a one-size message misdescribed keyword-only calls). + if legacy_order and legacy_alias: + legacy_form = f"{method}(boundary, {alias_name}=...)" + elif legacy_order: + legacy_form = f"{method}(boundary, conds, ...) positional order" + else: + legacy_form = f"the '{alias_name}=' keyword of {method}()" import warnings warnings.warn( - f"{method}(boundary, {alias_name}=...) is deprecated; " + f"{legacy_form} is deprecated; " f"use {method}(conds, boundary, ...)", DeprecationWarning, stacklevel=3) if not isinstance(boundary, str): diff --git a/tests/test_0641_wave_c_api_shims.py b/tests/test_0641_wave_c_api_shims.py index 60851e884..0ea8ec92a 100644 --- a/tests/test_0641_wave_c_api_shims.py +++ b/tests/test_0641_wave_c_api_shims.py @@ -97,8 +97,20 @@ def test_legacy_and_new_give_identical_bc(self, mesh, stokes): assert bc_old.fn_F == bc_new.fn_F assert bc_old.fn_p == bc_new.fn_p + def test_alias_only_warning_names_the_keyword(self, mesh, stokes): + # Copilot review of #334: the warning must describe the legacy form + # the caller actually used - a keyword-only call is not "positional". + with pytest.warns(DeprecationWarning, match=r"the 'g=' keyword of add_nitsche_bc\(\)") as rec: + stokes.add_nitsche_bc(boundary="Top", g=0.5) + assert _one_deprecation(rec) == 1 + + def test_positional_order_warning_names_the_order(self, mesh, stokes): + with pytest.warns(DeprecationWarning, match=r"positional order is deprecated") as rec: + stokes.add_nitsche_bc("Top", 0.5) + assert _one_deprecation(rec) == 1 + def test_datum_given_twice_is_an_error(self, mesh, stokes): - with pytest.raises(TypeError, match="conds"): + with pytest.raises(TypeError, match="as 'conds' and as 'g='"): stokes.add_nitsche_bc(0.5, "Top", g=0.5) def test_missing_boundary_is_an_error(self, mesh, stokes):