Fixes a few buggy _eval_as_leading_term methods
#21253
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.9. Click here to see the pull request description that was parsed.
Update The release notes on the wiki have been updated. |
6d39d72 to
1da36d0
Compare
1da36d0 to
7fca4a6
Compare
d29d587 to
748294e
Compare
|
The two failing tests are for a similar reason to #21334, i.e. expressions which can be simplified to 0 on a call to diff --git a/sympy/core/add.py b/sympy/core/add.py
index 1da38afed6..7a52597102 100644
--- a/sympy/core/add.py
+++ b/sympy/core/add.py
@@ -1011,15 +1011,7 @@ def _eval_as_leading_term(self, x, cdir=0):
new_expr = new_expr.simplify()
iszero = new_expr.is_zero
if iszero is True:
- # simple leading term analysis gave us cancelled terms but we have to send
- # back a term, so compute the leading term (via series)
- n0 = min.getn()
- res = Order(1)
- incr = S.One
- while res.is_Order:
- res = old._eval_nseries(x, n=n0+incr, logx=None, cdir=cdir).cancel().powsimp().trigsimp()
- incr *= 2
- return res.as_leading_term(x, cdir=cdir)
+ return old.simplify().compute_leading_term(x)
elif new_expr is S.NaN:
return old.func._from_args(infinite) |
|
I would rather avoid using |
c39ec35 to
a87f972
Compare
|
It is difficult to get the suggestion in #21253 (comment) to work. The commit I pushed partially fixes #21334, in that there is no longer a diff --git a/sympy/core/add.py b/sympy/core/add.py
index 404e5aa9f0..bd0f528970 100644
--- a/sympy/core/add.py
+++ b/sympy/core/add.py
@@ -1006,14 +1006,21 @@ def _eval_as_leading_term(self, x, cdir=0):
return expr
new_expr = new_expr.trigsimp().factor()
- if not new_expr:
+ iszero = new_expr.is_zero
+ if iszero is None:
+ check = new_expr.simplify()
+ iszero = check.is_zero
+ if iszero is True:
# simple leading term analysis gave us cancelled terms but we have to send
# back a term, so compute the leading term (via series)
+ re = (self - new_expr).expand()
+ if re.is_Order:
+ return re
n0 = min.getn()
res = Order(1)
incr = S.One
while res.is_Order:
- res = old._eval_nseries(x, n=n0+incr, logx=None, cdir=cdir).cancel().trigsimp()
+ res = old._eval_nseries(x, n=n0+incr, logx=None, cdir=cdir).cancel().powsimp().trigsimp()
incr *= 2
return res.as_leading_term(x, cdir=cdir)
However, with this, This limit has #21334 as an intermediate step, and having the example in the issue output |
sympy/core/add.py
Outdated
| new_expr = new_expr.simplify() | ||
| iszero = new_expr.is_zero | ||
| if iszero is True: | ||
| new_expr = new_expr.trigsimp().factor() |
There was a problem hiding this comment.
I would recommend new_expr = factor_terms(new_expr.trigsimp().cancel())
There was a problem hiding this comment.
An issue with this -
In [1]: a, b, c, x = symbols('a b c x', positive=True)
In [2]: limit((a + 1)*x - sqrt((a + 1)**2*x**2 + b*x + c), x, oo)
Out[2]:
⎛ ______________ ⎞
⎜ ╱ 2 ⎟
∞⋅sign⎝a - ╲╱ a + 2⋅a + 1 + 1⎠(Currently outputs -b/(2*a + 2))
There was a problem hiding this comment.
This highlights the importance of getting the expression into form before it enters the limit code. If cse were used on the expression before beginning and the assumptions copied to the substituted variable, the correct result is obtained in master:
>>> cse(eq)
([(x0, a + 1)], [x*x0 - sqrt(b*x + c + x**2*x0**2)])
>>> r, e = _
>>> x0=r[0][0]
>>> _x0 = Symbol('x0',**r[0][1]._assumptions)
>>> limit(e[0].subs(x0, _x0), x, oo).subs(_x0, r[0][1])
-b/(2*(a + 1))The core workings should not have to deal with all the different ways to write expressions more than necessary. Even a wide-acting call like simplify should be used with great reservation (and preferably not at all).
There was a problem hiding this comment.
My exams are over and I will resume contributing now.
I think this should be the place where the expression changes to a suitable form -
Lines 683 to 686 in f174489
cse on e0? I only see cse being used in one other place in the codebase (apart from printing and codegen), which is at sympy/sympy/solvers/ode/ode.py
Line 2681 in f174489
cse be used in library code or is there any other preferred alternative?
| @@ -422,7 +422,7 @@ def test_issue_5740(): | |||
| def test_issue_6366(): | |||
| n = Symbol('n', integer=True, positive=True) | |||
| r = (n + 1)*x**(n + 1)/(x**(n + 1) - 1) - x/(x - 1) | |||
| assert limit(r, x, 1) == n/2 | |||
| assert limit(r, x, 1).cancel() == n/2 | |||
There was a problem hiding this comment.
with the suggestion I made, cancel is not necessary here
125876a to
3c23245
Compare
Added non-integer power of positive Add expression simplification
3c23245 to
cfb8b76
Compare
506dabe to
721a509
Compare
|
This looks good.Thanks! |
References to other Issues or PRs
Closes #11465
Closes #14563
Fixes #20697
Fixes #21176
Fixes #21177
Fixes #21245
Fixes #21550
Brief description of what is fixed or changed
Add._eval_as_leading_term- If the initially obtained leading term contained irrational or imaginary terms,togetherwas insufficient to check whether the term was 0 or not, as it only cancels rational coefficients._mexpandalong withfactoris being used instead now to fix this.Pow._eval_as_leading_term- In a few cases where g is 0, but cannot be caught byg.is_zero(returns None),NotImplementedErrorwas being raised. In these cases however, it is indeed possible to compute the leading term, so this case has been added._mexpandat the last return is to make get a simplified answer (in cases like #20697), if cancelling of terms is possible.tan._eval_as_leading_termandcot._eval_as_leading_term- Both methods were incomplete, and did not return the correct answer for angles outside their principle domains -(Before)
Release Notes
AddandPownow simplify irrational and imaginary expressionstanandcotnow work for angles outside their principle domain