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

No-expand() Poly construction per default (step 1) #1047

Merged
merged 3 commits into from
Mar 2, 2022

Conversation

@skirpichev skirpichev added this to the 0.12 milestone Aug 10, 2020
@skirpichev skirpichev added polys documentation enhancement new feature requests (or implementation) labels Aug 10, 2020
@skirpichev
Copy link
Member Author

@user234683, I see no reproducible example in the sympy/sympy#12998. Could you provide one?

I suspect, that this SymPy's issue is fixed by the current PR in the Diofant, but want to be sure.

@user234683
Copy link

This appears to be the polynomial I was using at the time:
f = 6*(-2*e*(15*e - 13)*(c5**4 + 4*c5**3*c6 + 4*c5**3*c7 + 4*c5**3*c8 + 4*c5**3*c9 + 6*c5**2*c6**2 + 12*c5**2*c6*c7 + 12*c5**2*c6*c8 + 12*c5**2*c6*c9 + 6*c5**2*c7**2 + 12*c5**2*c7*c8 + 12*c5**2*c7*c9 + 6*c5**2*c8**2 + 12*c5**2*c8*c9 + 6*c5**2*c9**2 + 4*c5*c6**3 + 12*c5*c6**2*c7 + 12*c5*c6**2*c8 + 12*c5*c6**2*c9 + 12*c5*c6*c7**2 + 24*c5*c6*c7*c8 + 24*c5*c6*c7*c9 + 12*c5*c6*c8**2 + 24*c5*c6*c8*c9 + 12*c5*c6*c9**2 + 4*c5*c7**3 + 12*c5*c7**2*c8 + 12*c5*c7**2*c9 + 12*c5*c7*c8**2 + 24*c5*c7*c8*c9 + 12*c5*c7*c9**2 + 4*c5*c8**3 + 12*c5*c8**2*c9 + 12*c5*c8*c9**2 + 4*c5*c9**3 + c6**4 + 4*c6**3*c7 + 4*c6**3*c8 + 4*c6**3*c9 + 6*c6**2*c7**2 + 12*c6**2*c7*c8 + 12*c6**2*c7*c9 + 6*c6**2*c8**2 + 12*c6**2*c8*c9 + 6*c6**2*c9**2 + 4*c6*c7**3 + 12*c6*c7**2*c8 + 12*c6*c7**2*c9 + 12*c6*c7*c8**2 + 24*c6*c7*c8*c9 + 12*c6*c7*c9**2 + 4*c6*c8**3 + 12*c6*c8**2*c9 + 12*c6*c8*c9**2 + 4*c6*c9**3 + c7**4 + 4*c7**3*c8 + 4*c7**3*c9 + 6*c7**2*c8**2 + 12*c7**2*c8*c9 + 6*c7**2*c9**2 + 4*c7*c8**3 + 12*c7*c8**2*c9 + 12*c7*c8*c9**2 + 4*c7*c9**3 + c8**4 + 4*c8**3*c9 + 6*c8**2*c9**2 + 4*c8*c9**3 + c9**4) + (e + 1)**2*(-c0*c5**2*c8 - 4*c0*c5**2*c9 + 2*c0*c5*c6*c7 + 4*c0*c5*c6*c8 + 4*c0*c5*c6*c9 + 4*c0*c5*c7**2 + 16*c0*c5*c7*c8 + 24*c0*c5*c7*c9 + 16*c0*c5*c8**2 + 50*c0*c5*c8*c9 + 40*c0*c5*c9**2 - c0*c6**3 - 4*c0*c6**2*c7 - 4*c0*c6**2*c8 - 4*c0*c6**2*c9 - 6*c0*c6*c7**2 - 12*c0*c6*c7*c8 - 10*c0*c6*c7*c9 - 5*c0*c6*c8**2 - 4*c0*c6*c8*c9 + 4*c0*c6*c9**2 - 4*c0*c7**3 - 15*c0*c7**2*c8 - 16*c0*c7**2*c9 - 20*c0*c7*c8**2 - 44*c0*c7*c8*c9 - 24*c0*c7*c9**2 - 10*c0*c8**3 - 36*c0*c8**2*c9 - 45*c0*c8*c9**2 - 20*c0*c9**3 - c1*c5**2*c7 - 4*c1*c5**2*c8 - 10*c1*c5**2*c9 + c1*c5*c6**2 + 4*c1*c5*c6*c7 + 4*c1*c5*c6*c8 + 6*c1*c5*c7**2 + 20*c1*c5*c7*c8 + 26*c1*c5*c7*c9 + 19*c1*c5*c8**2 + 56*c1*c5*c8*c9 + 44*c1*c5*c9**2 - c1*c6**2*c9 + 2*c1*c6*c7*c8 + 4*c1*c6*c7*c9 + 4*c1*c6*c8**2 + 16*c1*c6*c8*c9 + 16*c1*c6*c9**2 - c1*c7**3 - 4*c1*c7**2*c8 - 4*c1*c7**2*c9 - 6*c1*c7*c8**2 - 12*c1*c7*c8*c9 - 5*c1*c7*c9**2 - 4*c1*c8**3 - 15*c1*c8**2*c9 - 20*c1*c8*c9**2 - 10*c1*c9**3 - c2*c5**2*c6 - 4*c2*c5**2*c7 - 10*c2*c5**2*c8 - 20*c2*c5**2*c9 - 4*c2*c5*c6*c8 - 14*c2*c5*c6*c9 + 4*c2*c5*c7**2 + 14*c2*c5*c7*c8 + 16*c2*c5*c7*c9 + 16*c2*c5*c8**2 + 48*c2*c5*c8*c9 + 40*c2*c5*c9**2 - c2*c6**2*c8 - 4*c2*c6**2*c9 + c2*c6*c7**2 + 4*c2*c6*c7*c8 + 4*c2*c6*c7*c9 + 6*c2*c6*c8**2 + 20*c2*c6*c8*c9 + 19*c2*c6*c9**2 + 2*c2*c7*c8*c9 + 4*c2*c7*c9**2 - c2*c8**3 - 4*c2*c8**2*c9 - 6*c2*c8*c9**2 - 4*c2*c9**3 + c3*c5**3 - 6*c3*c5**2*c7 - 16*c3*c5**2*c8 - 31*c3*c5**2*c9 - 4*c3*c5*c6*c7 - 14*c3*c5*c6*c8 - 32*c3*c5*c6*c9 + c3*c5*c7**2 + 4*c3*c5*c7*c8 + 10*c3*c5*c8**2 + 32*c3*c5*c8*c9 + 31*c3*c5*c9**2 - c3*c6**2*c7 - 4*c3*c6**2*c8 - 10*c3*c6**2*c9 - 4*c3*c6*c7*c9 + 4*c3*c6*c8**2 + 14*c3*c6*c8*c9 + 16*c3*c6*c9**2 - c3*c7**2*c9 + c3*c7*c8**2 + 4*c3*c7*c8*c9 + 6*c3*c7*c9**2 - c3*c9**3 + 4*c4*c5**3 + 6*c4*c5**2*c6 - 4*c4*c5**2*c7 - 19*c4*c5**2*c8 - 40*c4*c5**2*c9 + 4*c4*c5*c6**2 - 2*c4*c5*c6*c7 - 20*c4*c5*c6*c8 - 48*c4*c5*c6*c9 - 4*c4*c5*c7*c8 - 16*c4*c5*c7*c9 + 4*c4*c5*c8**2 + 14*c4*c5*c8*c9 + 20*c4*c5*c9**2 + c4*c6**3 - 6*c4*c6**2*c8 - 16*c4*c6**2*c9 - 4*c4*c6*c7*c8 - 14*c4*c6*c7*c9 + c4*c6*c8**2 + 4*c4*c6*c8*c9 + 10*c4*c6*c9**2 - c4*c7**2*c8 - 4*c4*c7**2*c9 + 4*c4*c7*c9**2 + c4*c8*c9**2))/(e + 1)**2

@skirpichev
Copy link
Member Author

skirpichev commented Aug 11, 2020

Thank you.

Well, I have these timings on this branch:

In [9]: %timeit degree(f, c5)
996 ms ± 2.91 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

In [10]: %timeit Poly(f).degree(c5)
178 ms ± 860 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

In [11]: %timeit Poly(f, c5).degree(c5)
998 ms ± 11.8 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

In [12]: %timeit degree(f, gen=c5)
179 ms ± 205 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

It seems apparent, that univariate polynomial construction (over variable c5) noticeably slower here (5x). You have reported 27x difference. For f_s = f.subs({e: 1}) is't about 2.2x:

In [18]: %timeit degree(f_s, c5)
105 ms ± 66.3 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

In [19]: %timeit Poly(f_s).degree(c5)
46.1 ms ± 95.4 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

In [20]: %timeit Poly(f_s, c5).degree(c5)
104 ms ± 115 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

In [21]: %timeit degree(f_s, gen=c5)
46.5 ms ± 75.6 µs per loop (mean ± std. dev. of 7 runs, 10 loops each)

@user234683
Copy link

Oh it appears I had fixed the issue already for degree but not degree_list: sympy/sympy#12999

@skirpichev
Copy link
Member Author

Hmm, I'm not sure if that was a fix. You break function API a little, making it inconsistent with other polynomial functions, while don't fix the real issue: noticeable slowdown of Poly(f, c5) vs Poly(f). C.f. SymPy:

In [4]: %timeit Poly(f)
228 ms ± 2.98 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

In [5]: %timeit Poly(f, c5)
6.9 s ± 117 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

vs the current Diofant (on this branch):

In [3]: %timeit Poly(f)
176 ms ± 502 µs per loop (mean ± std. dev. of 7 runs, 1 loop each)

In [4]: %timeit Poly(f, c5)
979 ms ± 2.17 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

@user234683
Copy link

user234683 commented Aug 12, 2020

It was a fix for the issue as it pertained to degree rather than Poly, so yes Poly will still have this issue. In fact, the fix was to switch degree from using e.g. Poly(f, c5) to Poly(f) due to the massive speed difference between them which your current pull is fixing.
EDIT: My bad; it seems I was confused and thought this was just a fix for the degree issue in Sympy. I hadn't realized this was a fork. Kudos on the fork, by the way; I looked over it. Hope it catches on.

@skirpichev skirpichev force-pushed the recursive-Poly-from-expr branch 4 times, most recently from f310b45 to c17c7a2 Compare September 9, 2020 05:33
@skirpichev skirpichev modified the milestones: 0.12, 0.13 Jan 7, 2021
@diofant diofant deleted a comment from github-actions bot Nov 3, 2021
@skirpichev skirpichev modified the milestones: 0.13, 1.0, 0.14 Nov 6, 2021
@skirpichev skirpichev changed the title No-expand() Poly construction per default No-expand() Poly construction per default (step 1) Mar 1, 2022
@skirpichev skirpichev marked this pull request as ready for review March 1, 2022 17:51
@skirpichev skirpichev merged commit 72da687 into diofant:master Mar 2, 2022
@skirpichev skirpichev deleted the recursive-Poly-from-expr branch March 2, 2022 02:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation enhancement new feature requests (or implementation) maintainability polys
Development

Successfully merging this pull request may close these issues.

2 participants