Repository: sympy/sympy
Pull request: Modified order.py
<!-- 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 #19545

#### Brief description of what is fixed or changed

The code `Order(1/n - 3/(3*n +2), (n, oo))` returns `1/n`, which
is not a sharp result.
As a consequence `Sum(1/n - 3/(3*n +2), (n, 1, oo)).is_convergent()`
gives the wrong result `False`.
In the lines 208-214 of `order.py`, if the number of variables are >= 2,
then the expression is expanded and is handled in `expr.is_Add` case.
However in the case of a single variable,
the alghorithm for the other case seems better.
Factorizing the expression forces to use the better alghoritm in the case
of a single variable.

modified:   sympy/concrete/tests/test_sums_products.py
modified:   sympy/series/order.py
modified:   sympy/series/tests/test_order.py

#### Other comments

See also   #18340


#### Release Notes

<!-- Write the release notes for this release below. 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
    - modified order.py to better work with Add objects.
<!-- END RELEASE NOTES -->

Problem statement:
is_convergent() returns a wrong result
The code
```
expr1 = 1/n - 3/(3*n +2)
res1 = Sum(expr1, (n, 1, oo)).is_convergent()
```
returns the wrong result `False`.

The code
```
expr2 = simplify(expr1)
res2 = Sum(expr2, (n, 1, oo)).is_convergent()
```
returns the correct result `True`.

The different behaviour is in line 471 of `summations.py`. In the first case order is `1/n` (correct, but not sharp), while in the second one `n**(-2)`, which is sharp.

Hints from issue discussion:

Commit message: Modified order.py

References to other Issues or PRs
Fixes #19545

Brief description of what is fixed or changed

The code `Order(1/n - 3/(3*n +2), (n, oo))` returns `1/n`, which
is not a sharp result.
As a consequence `Sum(1/n - 3/(3*n +2), (n, 1, oo)).is_convergent()`
gives the wrong result `False`.
In the lines 208-214 of `order.py`, if the number of variables are >= 2,
then the expression is expanded and is handled in `expr.is_Add` case.
However in the case of a single variable,
the alghorithm for the other case seems better.
Factorizing the expression forces to use the better alghoritm in the case
of a single variable.

See also PR#18340

On branch issue-19545
Changes to be committed:
    modified:   sympy/concrete/tests/test_sums_products.py
    modified:   sympy/series/order.py
    modified:   sympy/series/tests/test_order.py

Patch to review:
diff --git a/sympy/concrete/tests/test_sums_products.py b/sympy/concrete/tests/test_sums_products.py
index 258f98acdd63..18a9f850d573 100644
--- a/sympy/concrete/tests/test_sums_products.py
+++ b/sympy/concrete/tests/test_sums_products.py
@@ -1050,6 +1050,8 @@ def test_is_convergent():
     assert Sum(1/(x**3), (x, 1, oo)).is_convergent() is S.true
     assert Sum(1/(x**S.Half), (x, 1, oo)).is_convergent() is S.false
 
+    # issue 19545
+    assert Sum(1/n - 3/(3*n +2), (n, 1, oo)).is_convergent() is S.true
 
 def test_is_absolutely_convergent():
     assert Sum((-1)**n, (n, 1, oo)).is_absolutely_convergent() is S.false
diff --git a/sympy/series/order.py b/sympy/series/order.py
index 362edd4aded4..0320ce599525 100644
--- a/sympy/series/order.py
+++ b/sympy/series/order.py
@@ -212,6 +212,8 @@ def __new__(cls, expr, *args, **kwargs):
                 # x*y (wrong order term!).  That's why we want to deal with
                 # expand()'ed expr (handled in "if expr.is_Add" branch below).
                 expr = expr.expand()
+            else:
+                expr = expr.factor()
 
             old_expr = None
             while old_expr != expr:
diff --git a/sympy/series/tests/test_order.py b/sympy/series/tests/test_order.py
index f98247fd511b..76f433bcecbc 100644
--- a/sympy/series/tests/test_order.py
+++ b/sympy/series/tests/test_order.py
@@ -376,6 +376,8 @@ def test_order_at_infinity():
     assert Order(exp(x), (x, oo)).expr == Order(2*exp(x), (x, oo)).expr == exp(x)
     assert Order(y**x, (x, oo)).expr == Order(2*y**x, (x, oo)).expr == exp(log(y)*x)
 
+    # issue 19545
+    assert Order(1/x - 3/(3*x + 2), (x, oo)).expr == x**(-2)
 
 def test_mixing_order_at_zero_and_infinity():
     assert (Order(x, (x, 0)) + Order(x, (x, oo))).is_Add


Code review comments:
diff_hunk: @@ -212,6 +212,8 @@ def __new__(cls, expr, *args, **kwargs):
                 # x*y (wrong order term!).  That's why we want to deal with
                 # expand()'ed expr (handled in "if expr.is_Add" branch below).
                 expr = expr.expand()
+            else:
+                expr = expr.factor()
path: sympy/series/order.py
text: @user1:
It seems that this aims to transform the sum  `1/x - 3/(3*x + 2)` into a product, so it looks like it should only be applied to instances of `Add`. The code would probably be better placed here: https://github.com/sympy/sympy/blob/f7318dbfef2bf61e5ed8bc55f3e374880ec6ea67/sympy/series/order.py#L199-L201

It is possible that `expand_multinomial` could be removed and replaced by this. You could also test something like
```
                num, den = expr.as_numer_denom()
                expr = num/den
```
instead of `expr.factor()` which may be more expensive.

@author:
> It seems that this aims to transform the sum 1/x - 3/(3*x + 2) into a product, so it looks like it should only be applied to instances of Add. The code would probably be better placed here:
> 
> sympy/sympy/series/order.py
> 
> Lines 199 to 201 in f7318db
>  if expr.is_Add: 
>      from sympy import expand_multinomial 
>      expr = expand_multinomial(expr) 

Right. I changed the location of `factor`.

> It is possible that expand_multinomial could be removed

Yes, I run the tests and they pass also without `expand_multinomial`.

> You could also test something like
> 
>                 num, den = expr.as_numer_denom()
>                 expr = num/den
> 
> instead of expr.factor() which may be more expensive.
> 

Putting this instead of `expr.factor()` causes 2 failures in tests. The tests which failure are:

- `test_wester.py` at lines 2088-2090.
- `test_trigonometric.py` at llines 1224-1225.



@user2:
> Putting this instead of `expr.factor()` causes 2 failures in tests. The tests which failure are:
> 
> * `test_wester.py` at lines 2088-2090.
> * `test_trigonometric.py` at llines 1224-1225.

Can you tell what is `expr` in these cases using `num/den` and when you use `expr.factor()` ?



@author:
For `test_wester`.

```
expr = -exp(-_p**2/(_p + 1) + _p)*exp(_p**2/(_p + 1) - _p)/2 - exp(-_p**2/(_p + 1) + _p)*exp(_p**2/(_p + 1) - _p)/(2*_p)
expr.factor() = -(_p + 1)/(2*_p)
num = -_p*exp(-_p**2/(_p + 1) + _p)*exp(_p**2/(_p + 1) - _p) - exp(-_p**2/(_p + 1) + _p)*exp(_p**2/(_p + 1) - _p)
den = 2*_p
```
In this case the test does  not give an error, but it never finishes.

For `test_trigonometric`.
```
expr = -4*_Dummy_69 + (_Dummy_69 + 1)**4 - 4*(_Dummy_69 + 1)**3 + 6*(_Dummy_69 + 1)**2 - 3
expr.factor() = _Dummy_69**4
num = -4*_Dummy_69 + (_Dummy_69 + 1)**4 - 4*(_Dummy_69 + 1)**3 + 6*(_Dummy_69 + 1)**2 - 3
den = 1
```




@user1:
It looks like only `factor` factor will help here.