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 atan2(a, a) #1808
base: master
Are you sure you want to change the base?
Fix atan2(a, a) #1808
Conversation
@@ -2218,7 +2218,7 @@ TEST_CASE("Atan2: functions", "[functions]") | |||
REQUIRE(eq(*r1, *r2)); | |||
|
|||
r1 = atan2(add(one, sqrt(i2)), im1); | |||
r2 = div(mul(pi, i3), integer(-8)); | |||
r2 = div(mul(pi, i5), integer(8)); | |||
REQUIRE(eq(*r1, *r2)); | |||
|
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.
We should have a test for atan2(x, x)
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.
And also atan2(x, 0)
etc to cover symbolic cases where the numerator or denominator could potentially be 0 or complex.
} else { | ||
return div(pi, index); | ||
return sub(div(pi, index), mul(div(pi, i2), sub(sign(den), one))); |
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.
Are complex values of num
and den
handled by this case?
else | ||
return div(pi, i2); | ||
} | ||
return mul(div(pi, i2), sign(num)); |
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 don't think this is correct. If we do atan2(x, 0)
this will become pi/2 * sign(x)
, but for x = 0 this is should be NaN
. Sympy keeps this case unevaluated as atan2(x, 0)
.
No description provided.