Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Comparing two operations that contain log sometimes leads to TypeError exception #22020

Closed
jonatanschroeder opened this issue Sep 4, 2021 · 10 comments · Fixed by #22024
Closed

Comments

@jonatanschroeder
Copy link

This originally comes from PrairieLearn/PrairieLearn#4793, but it seems the problem is in sympy. We weren't able to narrow it down much further, other than to identify that this is specific to log (other functions like square root don't seem to have this problem).

In essence, two expressions can lead to an exception when being compared if the log expression is multiplied by a variable instead of a fixed coefficient.

Python 3.8.5 (default, May 27 2021, 13:30:53)
[GCC 9.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from sympy.parsing.sympy_parser import parse_expr
>>> x = parse_expr("log((2*V/3-V)/C)/-(R+r)*C")
>>> y = parse_expr("log((2*V/3-V)/C)/-(R+r)*2")
>>> x.equals(y)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/jonatan/.local/lib/python3.8/site-packages/sympy/core/expr.py", line 751, in equals
    constant = diff.is_constant(simplify=False, failing_number=True)
  File "/home/jonatan/.local/lib/python3.8/site-packages/sympy/core/expr.py", line 674, in is_constant
    if b is not None and b is not S.NaN and b.equals(a) is False:
  File "/home/jonatan/.local/lib/python3.8/site-packages/sympy/core/expr.py", line 741, in equals
    diff = factor_terms(simplify(self - other), radical=True)
  File "/home/jonatan/.local/lib/python3.8/site-packages/sympy/simplify/simplify.py", line 635, in simplify
    _e = cancel(expr)
  File "/home/jonatan/.local/lib/python3.8/site-packages/sympy/polys/polytools.py", line 6688, in cancel
    f = factor_terms(f, radical=True)
  File "/home/jonatan/.local/lib/python3.8/site-packages/sympy/core/exprtools.py", line 1274, in factor_terms
    return do(expr)
  File "/home/jonatan/.local/lib/python3.8/site-packages/sympy/core/exprtools.py", line 1271, in do
    rv = _keep_coeff(cont, p, clear=clear, sign=sign)
  File "/home/jonatan/.local/lib/python3.8/site-packages/sympy/core/mul.py", line 2069, in _keep_coeff
    if r == int(r):
  File "/home/jonatan/.local/lib/python3.8/site-packages/sympy/core/expr.py", line 332, in __int__
    raise TypeError("can't convert %s to int" % r)
TypeError: can't convert oo to int
>>>
@oscargus
Copy link
Contributor

oscargus commented Sep 4, 2021

The problem comes from

sympy/sympy/core/expr.py

Lines 661 to 669 in 5331d05

try:
a = expr.subs(list(zip(free, [0]*len(free))),
simultaneous=True)
if a is S.NaN:
# evaluation may succeed when substitution fails
a = expr._random(None, 0, 0, 0, 0)
except ZeroDivisionError:
a = None
if a is not None and a is not S.NaN:

where all free symbols are replaced by zeros on line 666 and numerically evaluated,which results in -oo. This later breaks the checking. I assume that checking that a is not infinite at line 669 will fix your problem, but not sure if it will break something else...

@oscargus oscargus added the core label Sep 4, 2021
@echuber2
Copy link

echuber2 commented Sep 5, 2021

This doesn't happen in Sympy 1.6.2 but does in 1.8. Is it not a regression then?

@echuber2
Copy link

echuber2 commented Sep 5, 2021

To elaborate, in 1.6.2 instead of getting an error, the equals call returns None:

>>> import sympy
>>> from sympy.parsing.sympy_parser import parse_expr
>>> x = parse_expr("log((2*V/3-V)/C)/-(R+r)*C")
>>> x
C*log(-V/(3*C))/(-R - r)
>>> y = parse_expr("log((2*V/3-V)/C)/-(R+r)*2")
>>> y
2*log(-V/(3*C))/(-R - r)
>>> x.equals(y)
>>> print(x.equals(y))
None

skirpichev added a commit to skirpichev/diofant that referenced this issue Sep 5, 2021
@oscargus
Copy link
Contributor

oscargus commented Sep 5, 2021

Is it not a regression then?

I'd say it is, yes. This code has not been touched in the last seven years, so probably something with the numerical evaluation has changed so that oo is returned rather than throwing a ZeroDivisionError.

@oscargus
Copy link
Contributor

oscargus commented Sep 5, 2021

Added a fix in #22022. Hopefully it doesn't break anything else.

None is sort of correct here as sympy cannot really determine if the expressions are the same or not. (I assume that we do not want to return False as they can be identical if C is 2, but not really sure about the logic.) And it should definitely not throw an error.

@oscarbenjamin
Copy link
Contributor

I bisected this to aef0304 from #20885. Maybe that should be changed or reverted.

@oscargus
Copy link
Contributor

oscargus commented Sep 5, 2021

Maybe that should be changed or reverted.

I've realized that it still makes sense to have these checks. If

sympy/sympy/core/expr.py

Lines 662 to 663 in c181470

a = expr.subs(list(zip(free, [0]*len(free))),
simultaneous=True)

returns oo, I think the same thing will happen. So I wouldn't fully blame #20885 here.

@oscargus
Copy link
Contributor

oscargus commented Sep 5, 2021

One could of course discuss how is_constant should work in general. It is not obvious why

sympy/sympy/core/expr.py

Lines 678 to 679 in c181470

if b is not None and b is not S.NaN and b.equals(a) is False:
return False
uses equals as there should be no more free symbols in it (or rather one should probably have a look at how that case is dealt with in equals).

@oscargus
Copy link
Contributor

oscargus commented Sep 5, 2021

The underlying problem is:

In [7]: simplify(-log(3)/2 + I*pi/2 -oo)
Traceback (most recent call last):

  File "C:\Users\Oscar\AppData\Local\Temp/ipykernel_14780/1626714943.py", line 1, in <module>
    simplify(-log(3)/2 + I*pi/2 -oo)

  File "C:\Users\Oscar\sympy\sympy\simplify\simplify.py", line 635, in simplify
    _e = cancel(expr)

  File "C:\Users\Oscar\sympy\sympy\polys\polytools.py", line 6682, in cancel
    f = factor_terms(f, radical=True)

  File "C:\Users\Oscar\sympy\sympy\core\exprtools.py", line 1274, in factor_terms
    return do(expr)

  File "C:\Users\Oscar\sympy\sympy\core\exprtools.py", line 1271, in do
    rv = _keep_coeff(cont, p, clear=clear, sign=sign)

  File "C:\Users\Oscar\sympy\sympy\core\mul.py", line 2115, in _keep_coeff
    if any(c == int(c) for c, _ in args):

  File "C:\Users\Oscar\sympy\sympy\core\mul.py", line 2115, in <genexpr>
    if any(c == int(c) for c, _ in args):

  File "C:\Users\Oscar\sympy\sympy\core\expr.py", line 333, in __int__
    raise TypeError("can't convert %s to int" % r)

TypeError: can't convert -oo to int

@oscargus
Copy link
Contributor

oscargus commented Sep 5, 2021

Better fix in #22024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants