Make Expr.add etc return NotImplemented for non-Expr Basic subclasses #18116

oscarbenjamin · 2019-12-24T12:10:27Z

References to other Issues or PRs

I found that I needed this when attempting to fix #4986 by introducing a new

Brief description of what is fixed or changed

Return NotImplemented from __add__ etc methods in Set and Expr if the sympified other operand is not of the expected type (i.e. Set or Expr). On current master NotImplemented is only returned if _sympify fails. With this PR it will be returned also if _sympify returns an object that is not of the expected type. There are two improvements resulting from this:

This makes it possible to define other non-Expr Basic classes that support arithmetic operations with Expr so if a is non-Expr and b is Expr then type(a) can control both a + b and b + a. On current master Expr will force a into an Add even if it doesn't belong there.

Master:

In [10]: class A(Basic): 
    ...:     def __mul__(self, other): 
    ...:         return -1 
    ...:     def __rmul__(self, other): 
    ...:         return -2 
    ...:          
    ...:                                                                                                                          

In [11]: A() * Symbol('x')                                                                                                        
Out[11]: -1

In [12]: Symbol('x') * A()                                                                                                        
Out[12]: x⋅A()

PR

In [4]: A() * Symbol('x')                                                                                                         
Out[4]: -1

In [5]: Symbol('x') * A()                                                                                                         
Out[5]: -2

This prevents garbage results like creating a Mul of a Set.

Master:

In [13]: Symbol('x') * FiniteSet(1)                                                                                               
Out[13]: x⋅{1}

In [14]: FiniteSet(1) * Symbol('x')                                                                                               
---------------------------------------------------------------------------
TypeError

PR

In [1]: Symbol('x') * FiniteSet(1)                                                                                                
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-1-d040ce0e457f> in <module>
----> 1 Symbol('x') * FiniteSet(1)

TypeError: unsupported operand type(s) for *: 'Symbol' and 'FiniteSet'

In [2]: FiniteSet(1) * Symbol('x')                                                                                                
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-320969da0219> in <module>
----> 1 FiniteSet(1) * Symbol('x')

TypeError: unsupported operand type(s) for *: 'FiniteSet' and 'Symbol'

There was a bug in dsolve due to these garbage results. Because a set will sympify to a FiniteSet the following would happen:

In [1]: Symbol('x') * {1}                                                                                                         
Out[1]: x⋅{1}

In [2]: {1} * Symbol('x')                                                                                                         
Out[2]: x⋅{1}

Under this PR both of those gives TypeError.

Other comments

There were apparently a couple of parts of the codebase depending on this behaviour. I've added a couple of fixes for those. In general I expect that there are lots of bugs to do with this hiding in different places due to e.g. expecting to do something like trigsimp(non-expr) which will sometimes work even though it is really aimed at Expr. We need a docmented way for functions like trigsimp to be able to identify Expr parts of a non-Expr object (like Set, Tuple, Matrix etc).

Release Notes

core
- Expr now uses cooperative dispatch for binary operations so it is possible for non-Expr Basic subclasses to override the behaviour of e.g. a + b where one of a or b is an instance of Expr. This also means that any non-Expr Basic subclasses can not depend on Expr.__add__ to create Add(a, b): if a class is not a subclass of Expr and wants to define binary operations with Expr it must do so explicitly in its own __add__ method. For classes depending on this this is not a backward compatible change.
sets
- Set now uses cooperative dispatch for binary operations so it is possible for non-Set Basic subclasses to override the behaviour of e.g. a + b where one of a or b is an instance of Set. This also means that any non-Set Basic subclasses can not depend on Expr.__add__ to create Union(a, b): if a class is not a subclass of Set and wants to define binary operations with Set it must do so explicitly in its own __add__ method. For classes depending on this this is not a backward compatible change.

sympy-bot · 2019-12-24T12:10:36Z

✅

Hi, I am the SymPy bot (v149). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

core
- Expr now uses cooperative dispatch for binary operations so it is possible for non-Expr Basic subclasses to override the behaviour of e.g. a + b where one of a or b is an instance of Expr. This also means that any non-Expr Basic subclasses can not depend on Expr.__add__ to create Add(a, b): if a class is not a subclass of Expr and wants to define binary operations with Expr it must do so explicitly in its own __add__ method. For classes depending on this this is not a backward compatible change. (#18116 by @oscarbenjamin)
sets
- Set now uses cooperative dispatch for binary operations so it is possible for non-Set Basic subclasses to override the behaviour of e.g. a + b where one of a or b is an instance of Set. This also means that any non-Set Basic subclasses can not depend on Expr.__add__ to create Union(a, b): if a class is not a subclass of Set and wants to define binary operations with Set it must do so explicitly in its own __add__ method. For classes depending on this this is not a backward compatible change. (#18116 by @oscarbenjamin)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.6.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests . Please also
write a comment on that issue linking back to this pull request once it is
open. -->

I found that I needed this when attempting to fix #4986 by introducing a new 

#### Brief description of what is fixed or changed

Return `NotImplemented` from `__add__` etc methods in `Set` and `Expr` if the sympified `other` operand is not of the expected type (i.e. `Set` or `Expr`). On current master `NotImplemented` is only returned if `_sympify` fails. With this PR it will be returned also if `_sympify` returns an object that is not of the expected type. There are two improvements resulting from this:

1. This makes it possible to define other non-`Expr` `Basic` classes that support arithmetic operations with `Expr` so if `a` is non-`Expr` and `b` is `Expr` then `type(a)` can control both `a + b` and `b + a`. On current master `Expr` will force `a` into an `Add` even if it doesn't belong there.

Master:
```julia
In [10]: class A(Basic): 
    ...:     def __mul__(self, other): 
    ...:         return -1 
    ...:     def __rmul__(self, other): 
    ...:         return -2 
    ...:          
    ...:                                                                                                                          

In [11]: A() * Symbol('x')                                                                                                        
Out[11]: -1

In [12]: Symbol('x') * A()                                                                                                        
Out[12]: x⋅A()
```

PR
```julia
In [4]: A() * Symbol('x')                                                                                                         
Out[4]: -1

In [5]: Symbol('x') * A()                                                                                                         
Out[5]: -2
```

2. This prevents garbage results like creating a `Mul` of a `Set`.

Master:
```julia
In [13]: Symbol('x') * FiniteSet(1)                                                                                               
Out[13]: x⋅{1}

In [14]: FiniteSet(1) * Symbol('x')                                                                                               
---------------------------------------------------------------------------
TypeError 
```

PR
```julia
In [1]: Symbol('x') * FiniteSet(1)                                                                                                
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-1-d040ce0e457f> in <module>
----> 1 Symbol('x') * FiniteSet(1)

TypeError: unsupported operand type(s) for *: 'Symbol' and 'FiniteSet'

In [2]: FiniteSet(1) * Symbol('x')                                                                                                
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-320969da0219> in <module>
----> 1 FiniteSet(1) * Symbol('x')

TypeError: unsupported operand type(s) for *: 'FiniteSet' and 'Symbol'
```

There was a bug in `dsolve` due to these garbage results. Because a `set` will `sympify` to a `FiniteSet` the following would happen:
```julia
In [1]: Symbol('x') * {1}                                                                                                         
Out[1]: x⋅{1}

In [2]: {1} * Symbol('x')                                                                                                         
Out[2]: x⋅{1}
```
Under this PR both of those gives `TypeError`.

#### Other comments

There were apparently a couple of parts of the codebase depending on this behaviour. I've added a couple of fixes for those. In general I expect that there are lots of bugs to do with this hiding in different places due to e.g. expecting to do something like `trigsimp(non-expr)` which will sometimes work even though it is really aimed at `Expr`. We need a docmented way for functions like `trigsimp` to be able to identify `Expr` parts of a non-`Expr` object (like `Set`, `Tuple`, `Matrix` etc).

#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* core
    * Expr now uses cooperative dispatch for binary operations so it is possible for non-Expr Basic subclasses to override the behaviour of e.g. `a + b` where one of `a` or `b` is an instance of Expr. This also means that any non-Expr Basic subclasses can not depend on `Expr.__add__` to create `Add(a, b)`: if a class is not a subclass of Expr and wants to define binary operations with Expr it must do so explicitly in its own `__add__` method. For classes depending on this this is not a backward compatible change.
* sets
    * Set now uses cooperative dispatch for binary operations so it is possible for non-Set Basic subclasses to override the behaviour of e.g. `a + b` where one of `a` or `b` is an instance of Set. This also means that any non-Set Basic subclasses can not depend on `Expr.__add__` to create `Union(a, b)`: if a class is not a subclass of Set and wants to define binary operations with Set it must do so explicitly in its own `__add__` method. For classes depending on this this is not a backward compatible change.
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

jksuom · 2019-12-25T09:14:18Z

sympy/codegen/array_utils.py

        return obj

+    # XXX: Maybe __mul__ and __rmul__ should have some type checking. These
+    # methods were added because Expr no longer accepts non-Expr in __mul__.


I think that it is possible to avoid calling these methods. This is called from

sympy/sympy/simplify/trigsimp.py

Line 1170 in 270cf68

return coeff*e

with coeff = 1 and e an instance of CoordSys3D (that appears as the second argument of a BaseScalar B.x). There could probably be

return coeff*e if isinstance(e, Expr) else e

That's a better idea. I've fixed this in futrig. However I found that I was actually getting the TypeError from multiplying Tuple there. I guess that there is some exception masking going on because there is an except TypeError: pass in trigsimp - probably that should be removed as it is maybe hiding bugs.

All try ... except code blocks are potential for trouble. We should discourage developers from using try ... except.

Agreed. There are good ways to use try/except but I see so many bad ways of doing it in SymPy code that I think we should discourage all try/except unless absolutely necessary and even then insist that the scope of try/except be limited as much as possible.

Upabjojr · 2019-12-26T10:02:26Z

sympy/codegen/array_utils.py

+        return Mul(self, other)
+
+    def __rmul__(self, other):
+        return Mul(other, self)


I would recommend:

def __mul__(self, other): raise NotImplementedError("Product of N-dim arrays is not uniquely defined. Use another method.")

At least you can specify the error message.

In the future we may decide to convert these objects into an Expr (it's not so far-fetched, you may wish to put these expressions into mathematical formulae?).

Thanks. I've added that but I included a special case for multiplying by 1 since presumably that's okay and that was the case in the test failure.

oscarbenjamin · 2019-12-27T16:05:37Z

I've edited the OP. This is ready for review.

oscarbenjamin · 2019-12-27T18:20:47Z

Actually I'll add more tests first

oscarbenjamin · 2019-12-28T04:28:58Z

This is ready for review

jksuom · 2019-12-28T08:48:48Z

sympy/core/tests/test_expr.py

+    objects when interacting with Expr instances.'''
+    e = Expr()
+    for ne in [NonBasic(), NonExpr()]:
+        ne = NonExpr()


This should probably be removed?

Thanks. That's thrown up some errors. I'll have to fix them...

jksuom · 2019-12-28T09:02:55Z

This looks fine to me though there is one thing that I'd like to change. The boilerplate if not isinstance(... is annoying. I think that the _sympifyit decorator could be enhanced to include that test. Its signature could be something like def _sympifyit(arg, retval=None, cls=None). If retval and cls are both different from None, then the class of the sympified result b here

sympy/sympy/core/decorators.py

Lines 89 to 91 in f7390e5

    
           if not hasattr(b, '_op_priority'): 
        
               b = sympify(b, strict=True) 
        
           return func(a, b)

should be checked before passing it to func(a, b).

oscarbenjamin · 2019-12-28T11:41:44Z

should be checked before passing it to func(a, b).

I did try that like this

from sympy import Basic
from sympy.core.decorators import _sympifyit

class Expr(Basic):
    @_sympifyit('other', NotImplemented, Expr)
    def __add__(self, other):
        return Add(self, other)

The problem is that this gives name Expr not found because the name Expr isn't bound until after the class statement but the decorator is called within the class statement. I thought it was possible to get round this with some kind of magic __class__ variable but it doesn't seem to be. So I'm not sure what the best approach is.

jksuom · 2019-12-28T11:58:47Z

This looks like the problem I had with gaussian-domains (#15396). I had to leave the _parent attribute to its default value None in GaussianInteger and GaussianRational, and reassign it later at global level. Maybe you could have a class attribute _class = None in the code and then set it later.

oscarbenjamin · 2019-12-28T15:29:41Z

I don't see where I'd set the _class attribute. One workaround is to apply the decorators after the class body:

from sympy import Basic
from sympy.core.decorators import _sympifyit

class Expr(Basic):
    def __add__(self, other):
        return Add(self, other)

Expr.__add__ = _sympifyit('other', NotImplemented, Expr)(Expr.__add__)

That could be done as a class decorator:

@sympify_binary_methods()
class Expr(Basic):
    def __add__(self, other):
        return Add(self, other)

jksuom · 2019-12-29T08:05:35Z

I like this approach. It looks promising but I'll take some time to look into it more carefully.

jksuom · 2019-12-29T08:09:03Z

sympy/core/decorators.py

+    In the above it is important that we return NotImplemented when other is
+    not sympifiable and also when the sympified result is not of the expected
+    type. This allows the MyTuple class to be used cooperatively with other
+    classes that overload __add__ and want to do something else in bombination


->combination

oscarbenjamin · 2019-12-29T14:47:07Z

I haven't changed this throughout sympy/core/numbers.py apart from ensuring that NotImplemented is returned when needed because I think that a broader rethink of these methods in Number is needed. Probably all the __add__ etc. methods there should be removed so that it is all handled by Add etc. I think that's the only way to ensure that Add(a, b) and a+b are always consistent.

jksuom · 2019-12-29T17:56:13Z

I think this is ready to be merged.

oscarbenjamin added 4 commits December 24, 2019 12:11

Make Expr.__add__ etc return NotImplemented

7a3eebb

Don't multiply set in ode

22e6b86

Add __mul__ to CodegenArrayContraction

4a22bac

Add __mul__ to CoordSys3D

d37bf69

Abdullahjavednesar requested a review from Upabjojr December 24, 2019 18:02

jksuom reviewed Dec 25, 2019

View reviewed changes

czgdp1807 added the core label Dec 25, 2019

Upabjojr reviewed Dec 26, 2019

View reviewed changes

oscarbenjamin added 5 commits December 26, 2019 17:32

Don't multiply coeff for non-Expr in futrig

e009297

Only allow multiplying 1 in codegen array

e2fc940

Also return NotImplemented in floordiv

17ad49f

Add tests for Expr + non-Expr

2cfb27f

Return NotImplemented in Set.__add__ etc.

8f9d03a

oscarbenjamin changed the title ~~[WIP] Make Expr.__add__ etc return NotImplemented~~ Make Expr.__add__ etc return NotImplemented for non-Expr Basic subclasses Dec 27, 2019

oscarbenjamin added 4 commits December 27, 2019 20:24

Add tests for cooperative operations with Expr

e317234

Only test raising of comparison operators on Py2

1b7e0ad

Test binary operations between Expr and non-Basic

494a184

Add tests for operators between Set and non-Set

7519b99

jksuom reviewed Dec 28, 2019

View reviewed changes

Return NotImplemented in Number.__add__ etc

bec42df

oscarbenjamin added 2 commits December 29, 2019 01:12

Add new decorators for _sympifyit

e8d7b56

Check _op_priority before sympification

5e57135

oscarbenjamin added 3 commits December 29, 2019 01:56

Use sympify_method_args in Set

ba61b5f

Exclude invalid comparison tests on PY2

fdf7166

Exclude invalid comparison tests on PY2

9c67a1d

jksuom reviewed Dec 29, 2019

View reviewed changes

Fix typo in docstring in decorators

aad0e81

jksuom merged commit e245f10 into sympy:master Dec 30, 2019

oscarbenjamin mentioned this pull request Dec 30, 2019

Separate boolean and symbolic relationals #4986

Open

oscarbenjamin mentioned this pull request Jan 8, 2020

Interval arithmetic wrong? #11613

Open

oscarbenjamin deleted the pr_expr_notimplemented branch January 9, 2020 01:55

bjodah mentioned this pull request Apr 25, 2022

Regression(?) when using sympy objects wrapped in numpy scalar array #23421

Closed

Uh oh!

Make Expr.__add__ etc return NotImplemented for non-Expr Basic subclasses #18116

Make Expr.__add__ etc return NotImplemented for non-Expr Basic subclasses #18116

Uh oh!

Conversation

oscarbenjamin commented Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References to other Issues or PRs

Brief description of what is fixed or changed

Other comments

Release Notes

Uh oh!

sympy-bot commented Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oscarbenjamin commented Dec 27, 2019

Uh oh!

oscarbenjamin commented Dec 27, 2019

Uh oh!

oscarbenjamin commented Dec 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jksuom commented Dec 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oscarbenjamin commented Dec 28, 2019

Uh oh!

jksuom commented Dec 28, 2019

Uh oh!

oscarbenjamin commented Dec 28, 2019

Uh oh!

jksuom commented Dec 29, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oscarbenjamin commented Dec 29, 2019

Uh oh!

jksuom commented Dec 29, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Make Expr.add etc return NotImplemented for non-Expr Basic subclasses #18116

Make Expr.add etc return NotImplemented for non-Expr Basic subclasses #18116

oscarbenjamin commented Dec 24, 2019 •

edited

Loading

sympy-bot commented Dec 24, 2019 •

edited

Loading

jksuom commented Dec 28, 2019 •

edited

Loading