Skip to content

Fix: ufuncs with scalar-first arg now work on Variables (#486)#1201

Open
gaoflow wants to merge 2 commits into
usnistgov:masterfrom
gaoflow:fix-arithmetic-base-class-486
Open

Fix: ufuncs with scalar-first arg now work on Variables (#486)#1201
gaoflow wants to merge 2 commits into
usnistgov:masterfrom
gaoflow:fix-arithmetic-base-class-486

Conversation

@gaoflow

@gaoflow gaoflow commented May 28, 2026

Copy link
Copy Markdown

Fix: ufuncs with a scalar first arg now work on Variables

Fixes #486.

Problem

numpy ufuncs reach Variable subclasses via __array_priority__ = 100.0 and Variable.__array_wrap__, which preserves argument order: when the first argument is a scalar and the second is a Variable, __array_wrap__ wraps the scalar as a _Constant and calls _Constant._BinaryOperatorVariable(..., other=var). The result's class is then resolved by

baseClass = self._getArithmeticBaseClass(other)   # self = _Constant, other = var

…which returns None because neither isinstance(_Constant, CellVariable) nor isinstance(CellVariable, _Constant) holds. baseClass = None short-circuits _BinaryOperatorVariable to return NotImplemented, so every scalar-first ufunc call breaks:

>>> from fipy import Grid1D, CellVariable
>>> from fipy.tools import numerix
>>> import numpy as np
>>> mesh = Grid1D(nx=5)
>>> var = CellVariable(mesh=mesh, value=2.)

# All NotImplemented before this patch:
>>> numerix.fmax(0.1, var)
NotImplemented
>>> np.add(0.1, var)
NotImplemented
>>> np.subtract(0.1, var)
NotImplemented
>>> np.power(0.1, var)
NotImplemented
>>> np.minimum(0.1, var)
NotImplemented

# All work, even though the dispatch path is the same:
>>> numerix.fmax(var, 0.1)        # works
>>> 0.1 + var                      # works (goes through __radd__, not __array_wrap__)

The issue (#486, opened by @guyer) titles this "fmax with non-CellVariable" but the bug is much broader: every numpy ufunc with a scalar in the first slot — np.add, np.subtract, np.multiply, np.divide, np.power, np.minimum, np.maximum, np.fmin, np.fmax, … — fails the same way.

Fix

Apply the case @guyer sketched in the issue body:

elif isinstance(self, _Constant):
    return other._getArithmeticBaseClass()

When self is a bare _Constant wrapping a scalar/array, the result's class is dictated by other. The previously-handled cases (isinstance(self, other._getArithmeticBaseClass()) and isinstance(other, _Constant)) keep their original behaviour; this only fires when self itself is the _Constant.

Verification

All previously-broken ufuncs now produce CellVariable-typed results, with the correct non-commutative semantics preserved:

call before after
fmax(0.1, var=2.) NotImplemented [2, 2, 2, 2, 2]
fmin(0.1, var=2.) NotImplemented [0.1, 0.1, 0.1, 0.1, 0.1]
add(0.1, var=2.) NotImplemented [2.1, 2.1, 2.1, 2.1, 2.1]
subtract(0.1, var=2.) NotImplemented [-1.9, -1.9, -1.9, -1.9, -1.9]
divide(0.1, var=2.) NotImplemented [0.05, 0.05, 0.05, 0.05, 0.05]
power(0.1, var=2.) NotImplemented [0.01, 0.01, 0.01, 0.01, 0.01]

A doctest is added inline in Variable._getArithmeticBaseClass that exercises the _Constant._getArithmeticBaseClass(CellVariable) resolution and the numerix.fmax(scalar, var) == numerix.fmax(var, scalar) symmetry.

Test impact

$ python -c "import unittest, fipy.variables.test as t; \
              r = unittest.TextTestRunner(verbosity=0).run(t._suite()); \
              print(r.testsRun, len(r.failures))"
# baseline (master @ d8c21fd26):  112 27
# with this patch:                113 27

113 vs 112 = +1 doctest (the new one). 27 vs 27 = same baseline of pre-existing numpy-2.x repr drift in fipy/variables/* — none of those touch _getArithmeticBaseClass.

Scope

Only fipy/variables/variable.py: a four-line elif plus a doctest. No public API change.

…snistgov#486)

numpy ufuncs reach Variable subclasses via __array_priority__ and
__array_wrap__, which preserves argument order: when a scalar is the
first argument and a Variable the second, __array_wrap__ wraps the
scalar as a _Constant and calls _Constant._BinaryOperatorVariable(...,
other=var).  The result class is then resolved by
_Constant._getArithmeticBaseClass(var), which returned None because
neither isinstance(_Constant, CellVariable) nor isinstance(CellVariable,
_Constant) holds.  baseClass=None makes _BinaryOperatorVariable return
NotImplemented, so e.g.

    numerix.fmax(0.1, var)           # NotImplemented
    np.add(0.1, var)                 # NotImplemented
    np.subtract(0.1, var)            # NotImplemented
    np.power(0.1, var)               # NotImplemented
    np.minimum(0.1, var)             # NotImplemented

even though the symmetric cases (var-first) all work, and even though
the plain Python operators (0.1 + var, etc.) work via __radd__.

Add the case @guyer described in the issue:

    elif isinstance(self, _Constant):
        return other._getArithmeticBaseClass()

When self is a bare _Constant wrapping a scalar/array, the result's
class is dictated by other.  Verified that all the ufuncs listed above
now return CellVariable-typed results, with the expected non-commutative
arithmetic (0.1 - var == -(var - 0.1), 0.1 / var == 1/(var/0.1),
0.1**var == 1/(...) * etc.).  Doctest added in
_getArithmeticBaseClass.

Local: 112 tests / 27 failures, identical to master baseline (all
pre-existing numpy-2.x repr drift).
@codacy-production

codacy-production Bot commented May 28, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric Results
Complexity 0
Duplication 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

The custom wordlist accepts the plural 'ufuncs' (line 106) but not the
singular 'ufunc'.  Reword without altering the doctest.
guyer added a commit that referenced this pull request Jun 16, 2026
* Add policy on AI contributions

Inspired by #1179, #1200, #1201, #1202, #1203, #1205, #1206, #1209, #1210

* Ignore extractive issues and PRs in changelog

https://llvm.org/docs/AIToolPolicy.html#extractive-contributions
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.

fmax with non-CellVariable returns NotImplemented.

1 participant