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

[GSoC] Core: Fixes _eval_nseries() of Mul #19369

Merged
merged 8 commits into from
May 24, 2020
Merged

Conversation

sachin-4099
Copy link
Contributor

Fixes: #14068

Brief description of what is fixed or changed

The series expansion of a product is computed as the product of expansions of the factors.

def _eval_nseries(self, x, n, logx):
    from sympy import Order, powsimp
    terms = [t.nseries(x, n=n, logx=logx) for t in self.args]
    res = powsimp(self.func(*terms).expand(), combine='exp', deep=True)
    if res.has(Order):
        res += Order(x**n, x)
    return res

This is correct if the leading term of each series is constant but not in general.

For example, to compute the expansion of f(x)/x**10 at x = 0 to order O(x**10) it is necessary to compute the series of f(x) to order O(x**20), the order O(x**10) does not suffice.

A working strategy could be the following:

  • Compute the order n0 of the leading term of the product as the sum of the orders of the leading terms of the factors.

  • For each factor, compute n - n0 terms of its series expansion (starting from its leading term of order n1 and ending at order n - n0 + n1).

  • Multiply the expansions (truncating at terms of order n).

Release Notes

  • core
    • Fixes _eval_nseries() function of mul.py

@sympy-bot
Copy link

sympy-bot commented May 19, 2020

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

Fixes: #14068

#### Brief description of what is fixed or changed

The series expansion of a product is computed as the product of expansions of the factors.

```
def _eval_nseries(self, x, n, logx):
    from sympy import Order, powsimp
    terms = [t.nseries(x, n=n, logx=logx) for t in self.args]
    res = powsimp(self.func(*terms).expand(), combine='exp', deep=True)
    if res.has(Order):
        res += Order(x**n, x)
    return res
```
This is correct if the leading term of each series is constant but not in general.

For example, to compute the expansion of `f(x)/x**10` at `x = 0` to order `O(x**10)` it is necessary to compute the series of `f(x)` to order `O(x**20)`, the order `O(x**10)` does not suffice.

A working strategy could be the following:

* Compute the order `n0` of the leading term of the product as the sum of the orders of the leading terms of the factors.

* For each factor, compute `n - n0` terms of its series expansion (starting from its leading term of order `n1` and ending at order `n - n0 + n1`).

* Multiply the expansions (truncating at terms of order `n`).

#### Release Notes


<!-- BEGIN RELEASE NOTES -->
* core
  * Fixes `_eval_nseries()` function of `mul.py`
<!-- END RELEASE NOTES -->

Update

The release notes on the wiki have been updated.

@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

Merging #19369 into master will increase coverage by 0.062%.
The diff coverage is 100.000%.

@@              Coverage Diff              @@
##            master    #19369       +/-   ##
=============================================
+ Coverage   75.576%   75.638%   +0.062%     
=============================================
  Files          651       652        +1     
  Lines       169536    169685      +149     
  Branches     40015     40049       +34     
=============================================
+ Hits        128130    128348      +218     
+ Misses       35786     35725       -61     
+ Partials      5620      5612        -8     

@sachin-4099
Copy link
Contributor Author

Are any additional regression tests required? @jksuom

@jksuom jksuom merged commit 31ddf98 into sympy:master May 24, 2020
@oscarbenjamin
Copy link
Collaborator

There is a significant slowdown in

sympy/core/tests/test_power.py::test_issue_6068

I've bisected that to commit 8a06c07 in this #19369 (this PR).

The content of the test is

from sympy import *

x = Symbol('x')
assert sqrt(sin(x)).series(x, 0, 7) == \
    sqrt(x) - x**Rational(5, 2)/12 + x**Rational(9, 2)/1440 - \
    x**Rational(13, 2)/24192 + O(x**7)
assert sqrt(sin(x)).series(x, 0, 9) == \
    sqrt(x) - x**Rational(5, 2)/12 + x**Rational(9, 2)/1440 - \
    x**Rational(13, 2)/24192 - 67*x**Rational(17, 2)/29030400 + O(x**9)
assert sqrt(sin(x**3)).series(x, 0, 19) == \
    x**Rational(3, 2) - x**Rational(15, 2)/12 + x**Rational(27, 2)/1440 + O(x**19)
assert sqrt(sin(x**3)).series(x, 0, 20) == \
    x**Rational(3, 2) - x**Rational(15, 2)/12 + x**Rational(27, 2)/1440 - \
    x**Rational(39, 2)/24192 + O(x**20)

In sympy 1.6 that takes 5 seconds whereas on master it takes 45 seconds.

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.

_eval_nseries of Mul is defective
4 participants