Fix bare-variable composition under sympy Matrix dispatch (#137) - #154
Merged
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes SymPy operator dispatch so Underworld mesh/swarm variables (via MathematicalMixin) compose correctly in mixed .sym / bare-variable expressions when a SymPy Matrix is on the left-hand side (issue #137).
Changes:
- Add
MathematicalMixin._op_priority = 11.5to force SymPy to delegate mixed operations (e.g.,Matrix * <bare Variable>) to Underworld’s reverse dunder methods. - Add a regression test covering the four mixed-form compositions from #137 and asserting equivalence.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/underworld3/utilities/mathematical_mixin.py |
Sets SymPy _op_priority to ensure reverse-operator dispatch for Matrix-LHS mixed expressions. |
tests/test_0726_bare_variable_composition_137.py |
Regression tests for all four bare/.sym combinations and equivalence after simplification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MathematicalMixin advertises (in CLAUDE.md and
MATHEMATICAL_MIXIN_DESIGN.md) that mesh and swarm variables can be
used directly in sympy arithmetic without explicit .sym access. In
practice it only worked when the bare variable was the *innermost*
operand. As soon as a bare variable appeared on the right of an
already-sympified subexpression — for example as the second factor
inside a sympy.exp() of a product — composition raised:
TypeError: Incompatible classes
<MutableDenseMatrix>, <EnhancedMeshVariable>
Cause
-----
.sym on a scalar variable returns a 1×1 sympy Matrix. SymPy's
Matrix.__mul__ raises TypeError directly instead of returning
NotImplemented, so Python's normal fall-through to the right operand's
__rmul__ never fires. The mixin already had a working __rmul__ — it
just was never being called.
Fix
---
SymPy provides _op_priority as the documented opt-in escape hatch:
any class with _op_priority strictly greater than the LHS's wins
dispatch. Matrix._op_priority is 10.01; Symbol/Mul/Expr are 10.0.
Setting MathematicalMixin._op_priority = 11.5 makes sympy delegate
"Matrix * <bare Variable>" and similar mixed-form expressions to our
existing reverse dunders, which sympify self via .sym and re-do the
operation cleanly as Matrix * Matrix.
This is a one-line class attribute change. The reverse dunders
(__rmul__, __radd__, __rsub__, __rtruediv__, __rpow__) were already
in place and correct; they just needed to be reachable.
Verification
------------
- The four-case reproducer from issue #137 (A_sym_both, B_bareC_symT,
C_symC_bareT, D_bare_both): pre-fix C and D raise TypeError,
post-fix all four return ImmutableDenseMatrix and simplify to
identical expressions.
- tests/test_0726_bare_variable_composition_137.py covers all four
forms plus an equivalence assertion. 5/5 pass in 2.3s.
- pytest -m "level_1 and tier_a" on amr-dev: 56 passed, 3 skipped,
0 failed. No regressions.
Closes #137.
Underworld development team with AI support from Claude Code
…xpression regression) The previous commit set MathematicalMixin._op_priority = 11.5 to win sympy dispatch over Matrix (10.01) for the bare-variable composition case. UWexpression inherits from MathematicalMixin (first in MRO), so that change inadvertently bumped UWexpression's priority too — and UWexpression has its OWN __rtruediv__ (and __rmul__, etc.) for sympy interop. Its __rmul__ explicitly handles MutableDenseMatrix; its __rtruediv__ does not, falling through to Symbol.__rtruediv__ which fails on MatrixBase. Result: solvers that compute Matrix / UWexpression (e.g. SNES_Diffusion's F0 expression `self.DuDt.bdf(0) / self.delta_t` at solvers.py:2499) broke with TypeError on every transient/Darcy/AdvDiff path. CI surfaced this on test_1005, test_1006, test_1100, test_1110. Fix: explicitly set UWexpression._op_priority = 10.0 to opt out of the mixin's bump. UWexpression already handles Matrix dispatch correctly through its own dunders (and via sympy's standard machinery for fall-through cases); it doesn't need the priority bump that pure MathematicalMixin classes (EnhancedMeshVariable, SwarmVariable) need. Verified: - #137 4-case reproducer still passes (the original fix is intact). - Previously-failing tests all pass: test_1005_TransientDarcyCartesian, test_1006_RichardsCartesian, test_1100_AdvDiffCartesian, test_1110_advDiffAnnulus — 8/8 pass. - pytest -m "level_1 and tier_a" on amr-dev: 56 passed, 3 skipped, 0 failed. Underworld development team with AI support from Claude Code
lmoresi
force-pushed
the
bugfix/mixin-bare-var-composition-137
branch
from
May 1, 2026 03:32
f11872a to
aba85fd
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
One-line class attribute on `MathematicalMixin` plus a regression test.
`MathematicalMixin._op_priority = 11.5` makes sympy delegate `Matrix * ` (and similar mixed-form expressions) to the mixin's existing reverse dunders.
Closes #137.
Root cause
`MathematicalMixin` advertises (in `CLAUDE.md` and `MATHEMATICAL_MIXIN_DESIGN.md`) that mesh and swarm variables can be used directly in sympy arithmetic without explicit `.sym` access. In practice it only worked when the bare variable was the innermost operand. As soon as a bare variable appeared on the right of an already-sympified subexpression, composition raised `TypeError: Incompatible classes`.
`.sym` on a scalar variable returns a 1×1 sympy `Matrix`. SymPy's `Matrix.mul` raises `TypeError` directly instead of returning `NotImplemented`, so Python's normal fall-through to the right operand's `rmul` never fires. The mixin already had a working `rmul` — it just was never being called.
Fix
SymPy provides `_op_priority` as the documented opt-in escape hatch: any class with `_op_priority` strictly greater than the LHS's wins dispatch. `Matrix._op_priority` is 10.01; `Symbol`/`Mul`/`Expr` are 10.0. Setting `MathematicalMixin._op_priority = 11.5` makes sympy delegate the operation to our reverse dunder, which sympifies `self` via `.sym` and re-runs cleanly as `Matrix * Matrix`.
The reverse dunders (`rmul`, `radd`, `rsub`, `rtruediv`, `rpow`) were already in place and correct.
Verification
The four-case reproducer from #137:
All four forms simplify to identical expressions.
Underworld development team with AI support from Claude Code