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
Fix powsimp issues #9183, #10095, and KeyError #13131
Conversation
Fixed powsimp raising ValueError: expecting ints or fractions ~ when base is float.
Fixed Pow.as_numer_denom() returning nan when either numerator or denominator is 1.
Added tests for sympy#9183 and sympy#10095 fixed by 'Fix Pow.as_numer_denom() when exp is infinite' and 'Fix powsimp raising ValueError when base is float'.
Fixed test case for sympy#10095.
I don't think automatic evaluation should use evalf at all. That can be slow in general and automatic stuff should be fast (see also https://github.com/sympy/sympy/wiki/Automatic-Simplification). Those examples you gave all work because the power can be implemented directly on the Number object itself without any actual numeric evaluation (for instance, Inside a simplification function like powsimp however it makes sense. |
Fixed powsimp raising KeyError in common_b[b] = common_b[b] + e, when one base has Float and another has its Rational(Integer) counterpart. This seems to be caused by sympy#11707.
@asmeurer I've been mistaken assumptions like .is_positive do not use evalf. Added another commit. |
sympy/simplify/powsimp.py
Outdated
if (b and b.is_Number and not all(ei.is_Number for ei in e) and \ | ||
coeff is not S.One and | ||
b not in (S.One, S.NegativeOne)): | ||
if (b and b.is_Number and b.is_rational and not all( |
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.
If you need an actual rational number you should use is_Rational or isinstance(b, Rational). is_rational will also be True for things like Symbol('x', rational=True)
.
I have just the one comment. +1 to merge once it is fixed. |
Changed is_Number and is_rational to is_Rational
@asmeurer Fixed 😄 |
nan
#10095Thing(s) to consider: Should we automatically evaluate k**oo to zero where k is in (-1, 1), i.e.,(1/(2*E))**oo
?