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 is_real for trigonometric and hyperbolic functions #13678
Conversation
if self.args[0].is_real: | ||
return True | ||
|
||
def _eval_is_positive(self): |
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.
In your branch:
>>> sinh(1 + I).is_positive
None
>>> sinh(1 + I).evalf().is_positive
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.
You probably want sinh(1 + I).is_real
to be False instead of None (making is_positive
False too), but I'm not sure it's worth it.
It would be easy to call self.as_real_imag()
and then .is_zero
, but even if we pass deep=False
nested evaluation will lead to runtime exponential in the depth of the expression: 8 seconds for building (sinh^9)(1 + I)
(^
in the sense of nested application) and 24 seconds for .is_real
, instead of 3 milliseconds total with the current PR.
Slowing down to return True instead of None could have been worthwhile because proving something is real often leads to simplifications, but is it worth the slowdown to return False instead of None?
Food for thought: in the current implementation, (a+b).is_real
only returns True when both a and b are real.
>>> (exp(2*I*pi/3) + exp(4*I*pi/3)).is_real
None
if self.args[0].is_real: | ||
return self.args[0].is_positive | ||
|
||
def _eval_is_negative(self): |
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.
similarly for this I guess.
This looks good to me. It fixes a bug even though it does not completely resolve all cases. Thanks! |
Fixes #13677