-
-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Solved incorrect left limit result for atan at inf #25763
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
Conversation
✅ Hi, I am the SymPy bot. 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.13. 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) | Change | Before [8059df73] <sympy-1.12^0> | After [060f97a8] | Ratio | Benchmark (Parameter) |
|----------|------------------------------------|---------------------|---------|----------------------------------------------------------------------|
| - | 88.3±0.8ms | 58.5±0.2ms | 0.66 | integrate.TimeIntegrationRisch02.time_doit(10) |
| + | 21.9±0.1μs | 39.0±0.1μs | 1.78 | integrate.TimeIntegrationRisch03.time_doit(1) |
| - | 6.98±0.02ms | 3.84±0.02ms | 0.55 | logic.LogicSuite.time_load_file |
| - | 2.12±0.01ms | 660±2μs | 0.31 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(3, 'sparse') |
| - | 10.4±0.06ms | 1.97±0.01ms | 0.19 | polys.TimePREM_LinearDenseQuadraticGCD.time_op(5, 'sparse') |
| - | 367±0.9μs | 82.7±0.3μs | 0.23 | polys.TimePREM_QuadraticNonMonicGCD.time_op(1, 'sparse') |
| - | 4.89±0.02ms | 364±0.7μs | 0.07 | polys.TimePREM_QuadraticNonMonicGCD.time_op(3, 'sparse') |
| - | 10.6±0.06ms | 1.10±0ms | 0.1 | polys.TimePREM_QuadraticNonMonicGCD.time_op(5, 'sparse') |
| - | 6.26±0.02ms | 3.99±0.01ms | 0.64 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(2, 'sparse') |
| - | 27.2±0.1ms | 12.2±0.01ms | 0.45 | polys.TimeSUBRESULTANTS_LinearDenseQuadraticGCD.time_op(3, 'sparse') |
| - | 6.93±0.04ms | 1.18±0ms | 0.17 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(1, 'sparse') |
| - | 16.4±0.05ms | 9.23±0.03ms | 0.56 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(2, 'sparse') |
| - | 211±0.4ms | 71.4±0.1ms | 0.34 | polys.TimeSUBRESULTANTS_QuadraticNonMonicGCD.time_op(3, 'sparse') |
| - | 6.39±0.01ms | 537±1μs | 0.08 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(3, 'sparse') |
| - | 27.6±0.04ms | 863±3μs | 0.03 | polys.TimeSUBRESULTANTS_SparseGCDHighDegree.time_op(5, 'sparse') |
| - | 612±3μs | 203±2μs | 0.33 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(1, 'sparse') |
| - | 6.49±0.01ms | 205±0.8μs | 0.03 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(3, 'sparse') |
| - | 17.0±0.04ms | 208±0.9μs | 0.01 | polys.TimeSUBRESULTANTS_SparseNonMonicQuadratic.time_op(5, 'sparse') |
| - | 162±0.7μs | 89.4±0.3μs | 0.55 | solve.TimeMatrixOperations.time_rref(3, 0) |
| - | 311±0.5μs | 109±0.8μs | 0.35 | solve.TimeMatrixOperations.time_rref(4, 0) |
| - | 32.0±0.2ms | 13.6±0.08ms | 0.42 | solve.TimeSolveLinSys189x49.time_solve_lin_sys |
Full benchmark results can be found as artifacts in GitHub Actions |
@@ -2160,3 +2160,7 @@ def test_as_real_imag(): | |||
def test_issue_18746(): | |||
e3 = cos(S.Pi*(x/4 + 1/4)) | |||
assert e3.period() == 8 | |||
|
|||
|
|||
def test_atan_inf_left(): |
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 do believe that we would like to see some sort of PoleError
here but I would like to test out more cases (think of cases that could go wrong) . the acot
function is related to the atan
function hence there is a possiblity of cases going wrong there too.
>>> atan(x).rewrite(acot)
acot(1/x)
Also could you please change the name of the function (use test_issue_issue_number
like other functions) and place it in sorted order
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 add the test here https://github.com/sympy/sympy/blob/master/sympy/series/tests/test_limits.py
@@ -2677,6 +2677,8 @@ def taylor_term(n, x, *previous_terms): | |||
def _eval_as_leading_term(self, x, logx=None, cdir=0): # atan | |||
arg = self.args[0] | |||
x0 = arg.subs(x, 0).cancel() | |||
if x0 is S.NaN: | |||
raise PoleError() | |||
if x0.is_zero: |
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 another thing being we would encounter this case in all inverse trigonometric functions (while calculating limits for asin(exp(x))
, acsc(exp(x))
). Just addressing an individual case might not help us overall, could you try tackling all cases that go wrong.
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.
nan
typically results when a substitution gives 0/0 (e.g. arg = sin(x)/x
). It may be advisable to first try to compute x0
as a limit before raising PoleError
.
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.
e.g. limit(acos(sin(x)/x),x,oo)
? @borgesaugusto, what does that do in this PR?
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.
With the "/ x" gives this result:
limit(acos(sin(x)),x,oo) -> I* log(I* AccumBounds(-1, 1) + AccumBounds(0, 1)) + pi/2
and
limit(acos(sin(x)/x),x,oo) gives:
"sympy.core.function.PoleError: Cannot expand sin(1/_w) around 0" in "sympy/functions/elementary/trigonometric.py" line 436" (The txt with the trace), ending with a NotImplementedError.
I added the check to all the inverse trigonometric functions (except atan2) and moved the test to the suggested file (incorporating the asserts for the new functions and changing the name). |
This seems like a positive step forward. Thanks, this is in. |
References to other Issues or PRs
Fixes #25582
Brief description of what is fixed or changed
As suggested by anutosh491 PoleError is raised in case the value is nan, which solves the bug. A test was added to check for this case.
Release Notes