Skip to content
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

Modified order.py #19546

Merged
merged 2 commits into from Jun 17, 2020
Merged

Modified order.py #19546

merged 2 commits into from Jun 17, 2020

Conversation

maurogaravello
Copy link
Contributor

@maurogaravello maurogaravello commented Jun 13, 2020

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.

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

  • series
    • modified order.py to better work with Add objects.

References to other Issues or PRs
Fixes sympy#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
@sympy-bot
Copy link

sympy-bot commented Jun 13, 2020

Hi, I am the SymPy bot (v160). 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.7.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Click here to see the pull request description that was parsed.

<!-- 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 -->

Update

The release notes on the wiki have been updated.

@codecov
Copy link

codecov bot commented Jun 13, 2020

Codecov Report

Merging #19546 into master will increase coverage by 0.093%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##            master    #19546       +/-   ##
=============================================
+ Coverage   75.664%   75.757%   +0.093%     
=============================================
  Files          654       654               
  Lines       169939    170565      +626     
  Branches     40065     40423      +358     
=============================================
+ Hits        128583    129216      +633     
+ Misses       35740     35739        -1     
+ Partials      5616      5610        -6     

@@ -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()
Copy link
Member

@jksuom jksuom Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

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.

Copy link
Contributor Author

@maurogaravello maurogaravello Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@sachin-4099 sachin-4099 Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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() ?

Copy link
Contributor Author

@maurogaravello maurogaravello Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@jksuom jksuom Jun 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like only factor factor will help here.

@jksuom
Copy link
Member

jksuom commented Jun 15, 2020

Sphinx test failed though nothing was changed in the documentation. I'll try to restart it.

@jksuom
Copy link
Member

jksuom commented Jun 15, 2020

The restart button has vanished again.

@sachin-4099
Copy link
Contributor

sachin-4099 commented Jun 15, 2020

Sphinx test failed though nothing was changed in the documentation. I'll try to restart it.

It will: #19551
There is no point restarting it.

@jksuom
Copy link
Member

jksuom commented Jun 16, 2020

Closing and opening to restart the build.

@jksuom jksuom closed this Jun 16, 2020
@jksuom jksuom reopened this Jun 16, 2020
@jksuom
Copy link
Member

jksuom commented Jun 17, 2020

Thanks, ready to be merged.

@jksuom jksuom merged commit 1059dae into sympy:master Jun 17, 2020
3 checks passed
@maurogaravello maurogaravello deleted the issue-19545 branch Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

is_convergent() returns a wrong result
5 participants