Repository: sympy/sympy
Pull request: Fixed bug related to limits of abs
<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### References to other Issues or PRs
<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234" (see
https://tinyurl.com/auto-closing for more information). Also, please
write a comment on that issue linking back to this pull request once it is
open. -->

Fixes #26513 
#### Brief description of what is fixed or changed
sign function in gruntz gave erroneous results for negative logarithmic arguments. Refactored `set_signs` in limits to let pass expressions returning NotImplementedError for gruntz to operate directly on abs functions . Made this separate PR for this issue as suggested in  #26749.

#### Other comments

#### Release Notes

<!-- Write the release notes for this release below between the BEGIN and END
statements. The basic format is a bulleted list with the name of the subpackage
and the release note for this PR. For example:

* solvers
  * Added a new solver for logarithmic equations.

* functions
  * Fixed a bug with log of integers. Formerly, `log(-x)` incorrectly gave `-log(x)`.

* physics.units
  * Corrected a semantical error in the conversion between volt and statvolt which
    reported the volt as being larger than the statvolt.

or if no release note(s) should be included use:

NO ENTRY

See https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more
information on how to write release notes. The bot will check your release
notes automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* series
  * Fixed `sign` in gruntz for better handling of terms having negative bases in power expressions with symbolic exponents.
<!-- END RELEASE NOTES -->


Problem statement:
Wrong limit result for Abs((-n/(n+1))**n)
```python
import sympy

from sympy import S, Symbol, limit, oo

print('sympy version:', sympy.__version__)

n = Symbol('n', real=True)

expr = (n/(n+1))**n
print(limit(expr, n, oo))  # Success

expr = (-n/(n+1))**n
print(limit(expr, n, oo))  # Wrong

expr = Abs((-n/(n+1))**n)
print(limit(expr, n, oo))  # Wrong
```

The result:
```python
sympy version: 1.12
exp(-1)
0
0
```

Hints from issue discussion:
Thanks for raising this. We do perform poorly when it comes to limits (bi-directional, or at infinities too ... you can easily find similar errors for `-oo` too ) involving the abs function. 
It seems that the result for the third limit (the one with abs) is being returned correctly by gruntz but not for the second one
```
In [6]: gruntz((-n/(n+1))**n, n, oo)
Out[6]: 0

In [7]: gruntz(abs((-n/(n+1))**n), n, oo)
Out[7]: exp(-1)

```
though for the second one(negative one) we get value 0 though the expression (-n/(n+1))**n is not well defined at infinity, it seems the problem stems from the following lines:
https://github.com/sympy/sympy/blob/2565eb35e255f0fee2267f20e9065b0f5eedf562/sympy/series/limits.py#L266-L286
since gruntz returns wrong value for (-n/(n+1))**n i.e 0 the computation for Abs one also gets messed up.

Ok these are my thoughts here 
1) So technically the gruntz is responsible for all 3 outputs here, which means instead of limit we can straight up call gruntz and if we get it to work, we are done.
2) Talking about the 3rd limit

So the code flow tries to solve `limit(Abs((-n/(n+1))**n), n, oo)` through 2 approaches 
i) calcualte `limit((-n/(n+1))**n), n, oo)` and then make a decision
ii) calculate `gruntz(Abs((-n/(n+1))**n), n, oo)`

Now as we can see, we don't solve `limit((-n/(n+1))**n), n, oo)` correctly and return `0` for the same, hence the case adding Abs goes wrong too

3) So now we need to solve `limit((-n/(n+1))**n), n, oo)` which further depends on `gruntz((-n/(n+1))**n), n, oo)` which currently returns `0`.

4) Hence `gruntz((-n/(n+1))**n), n, oo)` is the whole issue here. Once that is solved we can backtrack and all other limits would work by themselves. Now we return 0 here but the expression is not well defined at infinity. I think we just need to return self here.
```
>>> limit((-n/(n+1))**n), n, oo)
Limit((-x/(x + 1))**x, x, oo)
```
>Hence gruntz((-n/(n+1))**n), n, oo) is the whole issue here. Once that is solved we can backtrack and all other limits would work by themselves. Now we return 0 here but the expression is not well defined at infinity. I think we just need to return self here.

Yes exactly!
It seems that the following lines are the cause of the problem 
https://github.com/sympy/sympy/blob/3377fbd8311512529c6d014fd60055d8aab5274b/sympy/series/gruntz.py#L417-L418
the function sign(e,x) computes the sign of e as x tends to infinity.
In master sign(log(-n/(n+1), n) computes to -1 which is wrong and should have been I.
It seems the above lines were added with the context that taylor series expansion of `log(x+1)` around 0  is `  x -x**2/2+ ...' but since here we are trying to find the sign of expression as  x tends to oo so maybe this conditional isn't the right thing to do here.
Removing the conditional altogether solves the issue as sign of log expressions are calculated correctly using coefficient obtained from mrv_leadterm, here
https://github.com/sympy/sympy/blob/3377fbd8311512529c6d014fd60055d8aab5274b/sympy/series/gruntz.py#L421-L422

> the function sign(e,x) computes the sign of e as x tends to infinity.
> In master sign(log(-n/(n+1), n) computes to -1 which is wrong and should have been I.

Is this `sign` function supposed to be able to handle non-real inputs?

It gets this wrong:
```python
In [12]: from sympy.series.gruntz import sign

In [13]: sign(exp(I*x), x)
Out[13]: 1
```
Is it that somewhere higher up such non-real expressions should be rejected?
Yeah, since scenarios which will require computation of `sign(exp(I*x), x)` will be simply arise from things like `limit(exp(I*x), x,oo)`and in `limitinf` we use the `sign()` on the coefficient(using mrv_leadterm) of rewritten(rewrite in mrv_leadterm) term where it will raise error, 
https://github.com/sympy/sympy/blob/da2d56142230563bcfe50c564980e7d07fcc0cc9/sympy/series/gruntz.py#L646-L648
even in case of nested exponentiation or power tower scenarios, routines in `mrv` ensure that we dont end up getting exp or logs in exponents. Nevertheless we will never come to the point of having to use this sign function on exponents of any symbolic expression directly I believe.

Commit message: added and modified tests

Signed-off-by: arnabnandikgp <arnabnandi2002@gmail.com>

Patch to review:
diff --git a/sympy/series/gruntz.py b/sympy/series/gruntz.py
index 0bf3bf3e50b1..cad37a988757 100644
--- a/sympy/series/gruntz.py
+++ b/sympy/series/gruntz.py
@@ -411,7 +411,7 @@ def sign(e, x):
             return 1
         if e.exp.is_Integer:
             return s**e.exp
-    elif isinstance(e, log):
+    elif isinstance(e, log) and e.args[0].is_positive:
         return sign(e.args[0] - 1, x)
 
     # if all else fails, do it the hard way
diff --git a/sympy/series/limits.py b/sympy/series/limits.py
index b3976e551227..041996fdb13d 100644
--- a/sympy/series/limits.py
+++ b/sympy/series/limits.py
@@ -273,16 +273,20 @@ def set_signs(expr):
             arg_flag = isinstance(expr, arg)
             sign_flag = isinstance(expr, sign)
             if abs_flag or sign_flag or arg_flag:
-                sig = limit(expr.args[0], z, z0, dir)
-                if sig.is_zero:
-                    sig = limit(1/expr.args[0], z, z0, dir)
-                if sig.is_extended_real:
-                    if (sig < 0) == True:
-                        return (-expr.args[0] if abs_flag else
-                                S.NegativeOne if sign_flag else S.Pi)
-                    elif (sig > 0) == True:
-                        return (expr.args[0] if abs_flag else
-                                S.One if sign_flag else S.Zero)
+                try:
+                    sig = limit(expr.args[0], z, z0, dir)
+                    if sig.is_zero:
+                        sig = limit(1/expr.args[0], z, z0, dir)
+                except NotImplementedError:
+                    pass
+                else:
+                    if sig.is_extended_real:
+                        if (sig < 0) == True:
+                            return (-expr.args[0] if abs_flag else
+                                    S.NegativeOne if sign_flag else S.Pi)
+                        elif (sig > 0) == True:
+                            return (expr.args[0] if abs_flag else
+                                    S.One if sign_flag else S.Zero)
             return expr
 
         if e.has(Float):
diff --git a/sympy/series/tests/test_limits.py b/sympy/series/tests/test_limits.py
index 4f2760f8e82a..a2dce871b1e7 100644
--- a/sympy/series/tests/test_limits.py
+++ b/sympy/series/tests/test_limits.py
@@ -1414,6 +1414,12 @@ def test_issue_26250():
     assert limit(e1/e2, x, 0) == -S(1)/8
 
 
+def test_issue_26513():
+    assert limit(abs((-x/(x+1))**x), x ,oo) == exp(-1)
+    assert limit((x/(x + 1))**x, x, oo) == exp(-1)
+    raises (NotImplementedError, lambda: limit((-x/(x+1))**x, x, oo))
+
+
 def test_issue_26916():
     assert limit(Ei(x)*exp(-x), x, +oo) == 0
     assert limit(Ei(x)*exp(-x), x, -oo) == 0


Code review comments:
diff_hunk: @@ -273,16 +273,20 @@ def set_signs(expr):
             arg_flag = isinstance(expr, arg)
             sign_flag = isinstance(expr, sign)
             if abs_flag or sign_flag or arg_flag:
-                sig = limit(expr.args[0], z, z0, dir)
-                if sig.is_zero:
-                    sig = limit(1/expr.args[0], z, z0, dir)
-                if sig.is_extended_real:
-                    if (sig < 0) == True:
-                        return (-expr.args[0] if abs_flag else
-                                S.NegativeOne if sign_flag else S.Pi)
-                    elif (sig > 0) == True:
-                        return (expr.args[0] if abs_flag else
-                                S.One if sign_flag else S.Zero)
+                try:
+                    sig = limit(expr.args[0], z, z0, dir)
+                    if sig.is_zero:
+                        sig = limit(1/expr.args[0], z, z0, dir)
+                except NotImplementedError:
+                    pass
path: sympy/series/limits.py
text: @user1:
Just a doubt. Are we sure we want to use pass here ?
Do we have any other options and like what result do we get if we don't have a try except block here ?