-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Fast series
method for ring series
#9775
Conversation
I benchmarked some expressions here. The benchmarked expressions are mostly random. Is there a standard set of series for proper benchmarking?
In cases in which |
Can you point me to which |
Also, some tests fail. |
I had to change |
I see. The benchmarks are good enough. I am fine with this. Let's finish this up and get it in. |
1. rs_min_pow finds out the index of the required gen 2 Disable expand so that exp(a+b) is not expanded to exp(a)*exp(b)
def test_rs_series(): | ||
x, a, b, c = symbols('x, a, b, c') | ||
|
||
assert (rs_series(a, a, 5)).as_expr() == a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You do not need a parenthesis around rs_series
:
rs_series(a, a, 5).as_expr()
else: | ||
raise NotImplementedError | ||
|
||
def rs_series(expr, a, prec): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a higher level function, it needs a docstring with documentation and a few doctests showing how to use it.
I left some comments. In general this looks good, let's fix the issues I pointed out and merge. |
I've made the changes. |
@certik Anything else to be added here? |
I think it looks good, thanks! |
Fast `series` method for ring series
Thanks for the review! |
This PR implements a
series_fast
method that takes an arbitraryBasic
expression and expands it using sparse ring manipulations. We tried usingEX
domain for all expansion in #9614, but that proved to be too slow for our liking. This PR starts expanding over theQQ
domain and keeps adding generators as and when required. Thus, we always work on the simplest (almost) possible domain, resulting in considerable speed-up.The timings are extremely encouraging:
Because of the way we add generators, there is a constant overhead in expanding a given expression(irrespective of the asked order), depending on the complexity of the expression. So, for small orders,
series_fast
(yes, we need a new name!) might seem slow, but soon far outperformsseries
for even medium sized orders:Here
series_fast
is actually slower thanseries
for 10 terms, but once we expand till the 100th order:Clearly, the performance advantage will grow larger with larger orders.
There is a lot of scope for optimising the code (my priority was to make it work). It also needs to be played with to find bugs. However, I feel this is the way to go.
cc @certik @pernici @jksuom @thilinarmtb
ToDo
Fix wrong expansion of
sin(a**2 - a + 2)**5*cos(a**3 - a/2)
sin(x + a + b)
sin(x**QQ(1,2))
._parallel_dict_from_expr_if_gens
etc need to be modified