modify Basic.__eq__ to detect dissimilar Numbers

[smichr]

References to other Issues or PRs

closes #16916

Brief description of what is fixed or changed

Float(2) == 2 but the use of Float in an Expr will test as unequal x**2.0 != x**2.

Other comments

Release Notes

• core
  • numbers still compare the same as they do in Python (Float(1) == 1) except when they appear in an Expression, e.g. x**2.0 != x**2
  • bugfix to disambiguate was made
• sets
  • FiniteSet(1) == FiniteSet(1.0)

[sympy-bot]

✅

Hi, I am the SymPy bot (v147). 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

  • numbers still compare the same as they do in Python (Float(1) == 1) except when they appear in an Expression, e.g. x**2.0 != x**2 (#16924 by @smichr)

  • bugfix to disambiguate was made (#16924 by @smichr)

• sets

  • FiniteSet(1) == FiniteSet(1.0) (#16924 by @smichr)

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

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. -->

closes #16916 

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

`Float(2) == 2` but the use of Float in an Expr will test as unequal `x**2.0 != x**2`.

#### Other comments


#### 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
    - numbers still compare the same as they do in Python (`Float(1) == 1`) except when they appear in an Expression, e.g. `x**2.0 != x**2`
    - bugfix to `disambiguate` was made
- sets
    - `FiniteSet(1) == FiniteSet(1.0)`
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

[jksuom]

I think that taking types into account is a good idea. I wonder if that could be applied to all objects, not only numbers. So maybe something like this:

elif type(a) != type(b):
    return False

[codecov]

Codecov Report

Merging #16924 into master will increase coverage by 0.052%.
The diff coverage is 92.105%.

@@              Coverage Diff              @@
##            master    #16924       +/-   ##
=============================================
+ Coverage   73.922%   73.975%   +0.052%     
=============================================
  Files          620       620               
  Lines       160262    160369      +107     
  Branches     37606     37632       +26     
=============================================
+ Hits        118469    118633      +164     
+ Misses       36305     36249       -56     
+ Partials      5488      5487        -1

[oscarbenjamin]

This breaks the transitivity of __eq__ for Tuple:

In [5]: Tuple(2) == Tuple(2.0)                                                                                                    
Out[5]: False

In [6]: Tuple(2) == (2,) == Tuple(2.0)                                                                                            
Out[6]: True

In [7]: Tuple(2) == (2,) == (2.0,) == Tuple(2.0)                                                                                  
Out[7]: True

[oscarbenjamin]

And for FiniteSet

In [15]: FiniteSet(2) == {2} == {2.0} == FiniteSet(2.0)                                                                           
Out[15]: True

In [16]: FiniteSet(2) == FiniteSet(2.0)                                                                                           
Out[16]: False

[jksuom]

Maybe it is Expr.__eq__ that should be modified.

[jksuom]

This looks promising. FiniteSet(2) == FiniteSet(2.0) still fails (as it does in current master) but that should change when hash has been fixed.

[smichr]

still fails (as it does in current master)

I just tried excluding numbers from the type test. Since FiniteSet is not an Expr, it should pass compare the items via number/number (Expr/Expr) comparison. I am thinking this will come to this routine, skip the type test and go to the hashable content test where a==b will pass.

So outside of an Expr, Float(1) == 1; within and Expr, it will fail, e.g. pi + 1 != pi + Float(1) (I hope).

[smichr]

The first time the following is solved, the right answer is obtained in Py2. The next time, it is not:

>>> f,g=symbols('f g',cls=Function); eqn = Eq(Derivative(f(x), x), Derivative(g(x), x))
>>> dsolve(eqn,f(x))
Eq(f(x), C1 + g(x))
>>> f,g=symbols('f g',cls=Function); eqn = Eq(Derivative(f(x), x), Derivative(g(x), x))
Eq(f(x), C1 + Integral(diffx(g(x)), x))

[smichr]

Does the diffx class in ode.py need to define __hash__? @asmeurer

[oscarbenjamin]

The next time, it is not

I can't reproduce that. Python 2.7 and SymPy master:

In [1]: f,g=symbols('f g',cls=Function); eqn = Eq(Derivative(f(x), x), Derivative(g(x), x))

In [2]: dsolve(eqn,f(x))
Out[2]: f(x) = C₁ + g(x)

In [3]: f,g=symbols('f g',cls=Function); eqn = Eq(Derivative(f(x), x), Derivative(g(x), x))

In [4]: dsolve(eqn,f(x))
Out[4]: f(x) = C₁ + g(x)

In [5]: dsolve(eqn,f(x))
Out[5]: f(x) = C₁ + g(x)

[smichr]

Yes: not a problem in master, but a problem here. I don't know how to fix it and have just put the test in an if PY3: check. But that means the problem is still there and can show up on some other object...I just don't know how to tell who has this problem and why.

[jksuom]

It seems that the local class diffx:

sympy/sympy/solvers/ode.py

Lines 4106 to 4129 in ab7dbc8

	class diffx(Function):
	def inverse(self):
	# We mustn't use integrate here because fx has been replaced by _t
	# in the equation so integrals will not be correct while solve is
	# still working.
	return lambda expr: Integral(expr, var) + constant()
	# Replace derivatives wrt the independent variable with diffx
	def replace(eq, var):
	def expand_diffx(*args):
	differand, diffs = args[0], args[1:]
	toreplace = differand
	for v, n in diffs:
	for _ in range(n):
	if v == var:
	toreplace = diffx(toreplace)
	else:
	toreplace = Derivative(toreplace, v)
	return toreplace
	return eq.replace(Derivative, expand_diffx)
	# Restore derivatives in solution afterwards
	def unreplace(eq, var):
	return eq.replace(diffx, lambda e: Derivative(e, var))

is first introduced in replace and then removed in unreplace to make integration of a derivative possible. That is what happens in current master but in this PR replace and unreplace see different versions of diffx.

[oscarbenjamin]

in this PR replace and unreplace see different versions of diffx.

That makes sense. The diffx class is in the cached expression during a second call to dsolve which tried to unreplace a new diffx class. The simple solution is for diffx to be defined at module scope instead of in a function

[smichr]

simple solution is for diffx to be defined at module scope instead of in a function

I used to keep track of such details with common blocks and such with Fortran, but this level of thinking is not something I usually need for Python. Really appreciate the help on this.

[jksuom]

This seems to be working well.

[smichr]

A way to make Finite(2.0) == FiniteSet(2) is to just use the sorted args as the hashable content instead of a frozenset of the same. Although frozenset([1]) == frozenset([1.]), frozenset([S(1)]) != frozenset([S(1.)]).

[smichr]

If all looks well, please feel free to commit. I have nothing further to add.

This eliminates the irritating assymmetry of the former x**2.==x**2 but x**1. != x**1 -- now both (though for slightly different reasons) compare as unequal like 1.0*x != 1*x and 2.0*x != 2*x. Other than that, the impact has been much less than anticipated. I'm glad about that...and that my "mnemesis" ode.py was faced with a little help from friends. :-)

[smichr]

Not sure where this error is coming from:

Warning, treated as error:
duplicate object description of sympy.holonomic.holonomic, other instance in modules/holonomic/convert, use :noindex: for one of them
Makefile:49: recipe for target 'html' failed
make: *** [html] Error 2

[smichr]

@sylee957 , do you have an idea of how to fix the Sphinx error?

[sylee957]

I think the build had started failing from the master after new update to sphinx
I've opened up an issue in #16945

[oscarbenjamin]

I don't understand why this is different between Python 2 and 3.

I added the diffx class and I now realise (seeing the work you've done) that it is fragile in the face of caching. That needs to be fixed and you've added a fix for Python 2 but I'm now worried about how safe it is for Python 3 as well. I'll look into it... (don't let that hold up this PR though)

[asmeurer]

I've seen issues before with classes defined inside of functions. In this case, the class really does have to be defined inside the function, because it is different for each function call. Perhaps it would be better to store var and constant in the diffx.args.

If I had to guess, the Python 2-ness of this points to the accursed class registry and the __cmp__ (see #13059).

[asmeurer]

Yep:

$ cat test.py
from sympy.core.core import all_classes

from sympy import Function, symbols, dsolve, Derivative, Eq

x = symbols('x')
f, g = symbols('f g', cls=Function)

eqn = Eq(Derivative(f(x), x), Derivative(g(x), x))

dsolve(eqn, f(x), hint='nth_algebraic')
dsolve(eqn, f(x), hint='nth_algebraic')

diffxs = [i for i in all_classes if 'diffx' in str(i)]
print(diffxs[0] == diffxs[1])
$ python3 test.py
False
$ python2 test.py
True

Because Python 2 uses the ill conceived __cmp__ method of BasicMeta (__cmp__ does nothing in Python 3).

[jksuom]

This looks fine to me. The diffx issue is puzzling but I think that the implementation was questionable in view of caching. This PR only happened to bring that forth.

[asmeurer]

Did you check if this introduces any performance regressions?