Repository: sympy/sympy
Pull request: calculus : Fixes is_increasing() cases which undergo infinite recursive loop through periodicity()
<!-- 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 #23401

#### Brief description of what is fixed or changed
The `if` routine condition has been modified. The coefficient `coeff` seems to be a Float, hence earlier it did not work as expected.

On master - 
```
>>> expr = (p + 1)/(-1.0e-3*p**2 + 0.1*p + 0.1)
>>> is_increasing(expr,Interval(1,2),p)
# No result, internally evalution of infinite recursion.
```
On current branch - 
```
>>> expr = "(p + 1)/(-1.0e-3*p**2 + 0.1*p + 0.1)"
>>> is_increasing(expr,Interval(1,2))
True
>>> is_increasing(expr,Interval(1,2),p)
True
```

#### Other comments
The failing example from the issue has been used as the unit test and some other relevant examples have been tested !

#### 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.

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 -->
NO ENTRY
<!-- END RELEASE NOTES -->


Problem statement:
is_increasing() function can lead to periodicity() infinite recursive loop
**Environment:**
Windows 10
Python 3.9
Sympy 1.10.1
Running in VsCode Jupyter Notebook

**Explanation:**
Some expressions can cause the is_increasing() function to lead to an infinite recursive loop via the periodicity() function.  Below is the simplest code and expression I could find to cause this error (although there may be simpler).  I have no idea how or why this is happening.  The simplest function has an asymptote at -1 and is not periodic.

```
import sympy
var = sympy.symbols("p")
# original expression and simplest that still causes error
#expr = "(p + 1.5713046)/(-7.89819399268874e-8*p**2 + 0.008879019*p + 0.112832144)"
expr = "(p + 1)/(-1.0e-3*p**2 + 0.1*p + 0.1)"

print(sympy.is_increasing(expr, sympy.Interval(1, 2), var))
```

And here is the full error report (stopped with CTRL-C so the python kernel doesn't crash):
```
---------------------------------------------------------------------------
KeyboardInterrupt                         Traceback (most recent call last)
~\AppData\Local\Temp/ipykernel_28340/1436643031.py in <module>
      5 expr = "(p + 1)/(-1.0e-3*p**2 + 0.1*p + 0.1)"
      6 
----> 7 print(sympy.is_increasing(expr, sympy.Interval(1,2), var))

~\AppData\Roaming\Python\Python39\site-packages\sympy\calculus\singularities.py in is_increasing(expression, interval, symbol)
    199 
    200     """
--> 201     return monotonicity_helper(expression, lambda x: x >= 0, interval, symbol)
    202 
    203 

~\AppData\Roaming\Python\Python39\site-packages\sympy\calculus\singularities.py in monotonicity_helper(expression, predicate, interval, symbol)
    155     variable = symbol or (free.pop() if free else Symbol('x'))
    156     derivative = expression.diff(variable)
--> 157     predicate_interval = solveset(predicate(derivative), variable, S.Reals)
    158     return interval.is_subset(predicate_interval)
    159 

~\AppData\Roaming\Python\Python39\site-packages\sympy\solvers\solveset.py in solveset(f, symbol, domain)
   2214     if symbol not in _rc:
   2215         x = _rc[0] if domain.is_subset(S.Reals) else _rc[1]
-> 2216         rv = solveset(f.xreplace({symbol: x}), x, domain)
   2217         # try to use the original symbol if possible
   2218         try:

~\AppData\Roaming\Python\Python39\site-packages\sympy\solvers\solveset.py in solveset(f, symbol, domain)
   2238     f = piecewise_fold(f)
   2239 
-> 2240     return _solveset(f, symbol, domain, _check=True)
   2241 
   2242 

~\AppData\Roaming\Python\Python39\site-packages\sympy\solvers\solveset.py in _solveset(f, symbol, domain, _check)
   1044         from .inequalities import solve_univariate_inequality
   1045         try:
-> 1046             result = solve_univariate_inequality(
   1047             f, symbol, domain=domain, relational=False)
   1048         except NotImplementedError:

~\AppData\Roaming\Python\Python39\site-packages\sympy\solvers\inequalities.py in solve_univariate_inequality(expr, gen, relational, domain, continuous)
    489     else:
    490         e = expr.lhs - expr.rhs
--> 491         period = periodicity(e, gen)
    492         if period == S.Zero:
    493             e = expand_mul(e)

~\AppData\Roaming\Python\Python39\site-packages\sympy\calculus\util.py in periodicity(f, symbol, check)
    486 
    487         else:
--> 488             period = _periodicity(g.args, symbol)
    489 
    490     elif f.is_Add:

~\AppData\Roaming\Python\Python39\site-packages\sympy\calculus\util.py in _periodicity(args, symbol)
    558     periods = []
    559     for f in args:
--> 560         period = periodicity(f, symbol)
    561         if period is None:
    562             return None

~\AppData\Roaming\Python\Python39\site-packages\sympy\calculus\util.py in periodicity(f, symbol, check)
    483         coeff, g = f.as_independent(symbol, as_Add=False)
    484         if isinstance(g, TrigonometricFunction) or coeff is not S.One:
--> 485             period = periodicity(g, symbol)
    486 
    487         else:

~\AppData\Roaming\Python\Python39\site-packages\sympy\calculus\util.py in periodicity(f, symbol, check)
    483         coeff, g = f.as_independent(symbol, as_Add=False)
    484         if isinstance(g, TrigonometricFunction) or coeff is not S.One:
--> 485             period = periodicity(g, symbol)
    486 
    487         else:

    ...
```

Any ideas about what is going on here would be appreciated as I would like to use the is_increasing() function to check arbitrary expressions but that obviously can't happen with this bug around.  Thanks

Hints from issue discussion:
This seems to be the fix:
```diff
diff --git a/sympy/calculus/util.py b/sympy/calculus/util.py
index 4f6ada07f1..ed8c4e2fbd 100644
--- a/sympy/calculus/util.py
+++ b/sympy/calculus/util.py
@@ -480,9 +480,8 @@ def _check(orig_f, period):
 
     elif f.is_Mul:
         coeff, g = f.as_independent(symbol, as_Add=False)
-        if isinstance(g, TrigonometricFunction) or coeff is not S.One:
+        if isinstance(g, TrigonometricFunction) or coeff is not S.One and not (coeff.is_Float and coeff == 1.0):
             period = periodicity(g, symbol)
-
         else:
             period = _periodicity(g.args, symbol)
 ```
Looks like `coeff` in this case is a Float so testing `is  not S.One` does the wrong thing.

Commit message: Fix_23401

Patch to review:
diff --git a/sympy/calculus/tests/test_singularities.py b/sympy/calculus/tests/test_singularities.py
index 08036b5a1184..4d34e4a7c597 100644
--- a/sympy/calculus/tests/test_singularities.py
+++ b/sympy/calculus/tests/test_singularities.py
@@ -93,3 +93,9 @@ def test_is_monotonic():
     assert not is_monotonic(-x**2, S.Reals)
     assert is_monotonic(x**2 + y + 1, Interval(1, 2), x)
     raises(NotImplementedError, lambda: is_monotonic(x**2 + y + 1))
+
+
+def test_issue():
+    x = Symbol('x')
+    expr = (x + 1)/(-1.0e-3*x**2 + 0.1*x + 0.1)
+    assert is_increasing(expr, Interval(1,2), x)
diff --git a/sympy/calculus/util.py b/sympy/calculus/util.py
index 4f6ada07f123..32c9ace12750 100644
--- a/sympy/calculus/util.py
+++ b/sympy/calculus/util.py
@@ -480,7 +480,7 @@ def _check(orig_f, period):
 
     elif f.is_Mul:
         coeff, g = f.as_independent(symbol, as_Add=False)
-        if isinstance(g, TrigonometricFunction) or coeff is not S.One:
+        if isinstance(g, TrigonometricFunction) or coeff is not S.One and not (coeff.is_Float and coeff == 1.0):
             period = periodicity(g, symbol)
 
         else:


Code review comments:
diff_hunk: @@ -480,7 +480,7 @@ def _check(orig_f, period):
 
     elif f.is_Mul:
         coeff, g = f.as_independent(symbol, as_Add=False)
-        if isinstance(g, TrigonometricFunction) or coeff is not S.One:
+        if isinstance(g, TrigonometricFunction) or coeff is not S.One and not (coeff.is_Float and coeff == 1.0):
path: sympy/calculus/util.py
text: @user1:
We should really make a function for testing this sort of thing.

Looking at `git grep 'is S.One'` I see a lot of other places in the codebase that potentially have the same problem.

@user2:
I think it better to use `coeff != 1` here.

@user1:
Using `!=` does work but I would rather have a function specifically for testing integers and integer-valued floats (see #20033)

@user2:
A bit off-topic and I believe people have talked it a lot, but I don't understand why Sym(bolic)Py should support calculations mixing numeric and symbolic.

@user1:
In practice an analysis-oriented CAS that doesn't support approximate numerics is not very useful because most real problems cannot be solved symbolically.

@author:
Yes the `'is not S.One'` line is a part of multiple `if` routines in the code. Should I make a private function and check everytime ?

@author:
Also kindly look into the `CircleCI` test fail .All other tests have passed .

@user3:
> Also kindly look into the `CircleCI` test fail .All other tests have passed .

You have to rebase your code with latest master, so that latest commits into the master are available on your local machine. You can do that by:
```
git checkout master
git fetch upstream
git merge upstream/master
git checkout fix_23041
git rebase master
```

@user1:
The CircleCI check doesn't matter so nothing needs to be done about that.

I think maybe what is needed is a function that can check for when two expressions are equal except for possible differences where one expression has a float and the other has an int. For this particular case though to fix this particular issue we could just use `coeff != 1` for now to keep it simple.

@author:
Thanks @user2 for the commits.I have been busy with travel ,hence could not respond to the desired suggestions. Thanks again !