Skip to content

fix: symbolic sign, Max/Min, and numeric copysign in the SymPy backend#721

Draft
henryiii wants to merge 2 commits into
mainfrom
fix/sympy-backend
Draft

fix: symbolic sign, Max/Min, and numeric copysign in the SymPy backend#721
henryiii wants to merge 2 commits into
mainfrom
fix/sympy-backend

Conversation

@henryiii

Copy link
Copy Markdown
Member

🤖 AI text below 🤖

Part of #711.

Fixes the SymPy backend's _lib shim and related issues:

sign raised on symbolic input

_lib.sign delegated to numpy.sign, which cannot evaluate a symbolic relational. Repro (before):

>>> r, a = sympy.Symbol("r", positive=True), sympy.Symbol("a", real=True)
>>> vector.VectorSympy2D(rho=r, phi=sympy.Symbol("phi", real=True)).scale(a)
TypeError: cannot determine truth value of Relational: a < 0

Now uses sympy.sign for symbolic input (numpy for plain numbers).

maximum/minimum returned the first symbolic argument unconditionally

The two methods were byte-identical, so symbolic clamps were silently dropped — e.g. deltaangle's acos domain clamp and the t2/Mt2 floors. They now return sympy.Max/sympy.Min for symbolic input, and fall back to numpy.maximum/numpy.minimum for plain numbers (so nan from mixed sympy/object operations propagates instead of raising inside sympy.Max).

The exact-expression assertions in the sympy mirror tests were updated to the new honest forms (e.g. acos(Min(1, ...)), sqrt(Max(0, tau**2 + ...))). No numeric .subs(...).evalf() expectation changed.

copysign — documented, deliberately not fully fixed

copysign(val1, val2) returning val1 assumes the sign carrier is non-negative (true for timelike vectors). I attempted the faithful Abs(val1) * sign(val2) (and a Piecewise variant matching numpy's copysign(x, 0) == +x): both are mathematically right, but tau/t round-trips then produce nested Piecewise expressions that crash sympy's piecewise_fold/simplify_logic with ValueError: The argument 'nan' is not comparable, and degrade every tau-coordinate expression with unsimplifiable sign(...) factors. The shortcut is now explicitly commented (with a pointer to #711), and the numeric path (copysign(3.0, -2.0)) is handled correctly via numpy.copysign. A proper symbolic encoding of signed tau is left as a follow-up decision.

Other

  • Added the missing BSD-3 copyright headers to src/vector/backends/sympy.py and src/vector/_pytree.py.
  • Fixed an operator-precedence bug in two _wrap_result branch conditions (the isinstance/issubclass guards bound to only one arm of the or; latent today, aligned with the correct parenthesization in numpy.py).
  • New regression tests: symbolic scale, symbolic Max/Min, numeric copysign.

Tests: full suite 814 passed, 3 skipped (unrelated optional deps); prek -a clean.

🤖 Generated with Claude Code

@codecov

codecov Bot commented Jun 11, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.17%. Comparing base (d9ef1db) to head (1ef85a9).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #721   +/-   ##
=======================================
  Coverage   87.16%   87.17%           
=======================================
  Files          96       96           
  Lines       11199    11205    +6     
=======================================
+ Hits         9762     9768    +6     
  Misses       1437     1437           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…y backend

The _lib shim's sign() delegated to numpy.sign, which raises on symbolic
input (breaking scale() with a symbolic factor on rho/phi vectors), and
maximum/minimum returned the first symbolic argument unconditionally,
silently dropping domain clamps (acos inputs, Mt2/t2 floors). They now
use sympy.Max/sympy.Min/sympy.sign for symbolic input and fall back to
the numpy equivalents for plain numbers so that nan propagates instead
of raising inside sympy.

copysign keeps the assume-non-negative-carrier shortcut for symbolic
input (a faithful Abs(x)*sign(y) translation makes sympy's simplifier
choke on the nested Piecewise expressions produced by tau/t round
trips), but now documents the assumption and handles plain numbers
correctly. Affected exact-expression test assertions were updated to
the new honest Max/Min forms; numeric expectations are unchanged.

Also adds the missing BSD-3 headers to sympy.py and _pytree.py and
fixes an operator-precedence bug in two _wrap_result branch conditions
(latent; the guards bound to only one arm of the or).

Assisted-by: ClaudeCode:claude-fable-5
@henryiii henryiii force-pushed the fix/sympy-backend branch from 9a9ef8f to 6c232cc Compare June 16, 2026 04:04
Sign up for free to 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.

1 participant