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
Fixed issue with simplifying oo + imaginary #22024
Conversation
✅ Hi, I am the SymPy bot (v161). 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: This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.10. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
Benchmark results from GitHub Actions Lower numbers are good, higher numbers are bad. A ratio less than 1 Significantly changed benchmark results (PR vs master) Significantly changed benchmark results (master vs previous release) Full benchmark results can be found as artifacts in GitHub Actions |
sympy/core/mul.py
Outdated
@@ -2112,7 +2112,7 @@ def _keep_coeff(coeff, factors, clear=True, sign=False): | |||
if not clear and coeff.is_Rational and coeff.q != 1: | |||
args = [i.as_coeff_Mul() for i in factors.args] | |||
args = [(_keep_coeff(c, coeff), m) for c, m in args] | |||
if any(c == int(c) for c, _ in args): | |||
if any(not c.is_infinite and c == int(c) for c, _ in args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The condition should probably be something like c.is_Number
as int(c)
is not implemented for finite complex numbers:
In [23]: I.is_infinite
Out[23]: False
In [24]: int(I)
---------------------------------------------------------------------------
TypeError Traceback (most recent call last)
TypeError: can't convert complex to int
In [25]: I.is_Number
Out[25]: False
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, I only noted the oo
error, but that clearly makes sense. (For some reason the code doesn't come here for e.g. oo + I
, only for oo+I/2
etc, and apparently not for many other cases with complex numbers...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
However, as oo
is a Number, I am not sure exactly what it should be. Right now it works and it seems like c
rarely (if ever) an imaginary/complex number.
Also, c
should by sympified as far as I can tell, so not obvious what the purpose of c == int(c)
is? Check that c
is an integer or something else? Maybe is_Integer
is enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried is_Integer
only and so far so good. Pushing to run the complete test set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also,
c
should by sympified as far as I can tell, so not obvious what the purpose ofc == int(c)
is
Yes, c
was generated from as_coeff_Mul
so it will be sympified. Even I am not sure why the int
was used.
References to other Issues or PRs
Closes #22020
Brief description of what is fixed or changed
Simplifying expressions of the type
oo + I/2
used to throw an error. Not anymore.Other comments
To be honest, I do not fully understand the logic behind the code I added, but it seems to work and avoids the
int(c)
that was the cause of all evil here.Release Notes