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

Unpythonicity in baseclass.py #12480

Merged
merged 3 commits into from Oct 11, 2017
Merged

Unpythonicity in baseclass.py #12480

merged 3 commits into from Oct 11, 2017

Conversation

abramovat
Copy link

No description provided.

current = components[i]
following = components[i + 1]

for current, following in zip(components, components[1:]):
Copy link
Member

Choose a reason for hiding this comment

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

Should there be components[:-1]?

Copy link
Member

Choose a reason for hiding this comment

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

zip automatically stops on the shortest iterator, so it isn't necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pythonic as well as a little more fast. I am okay with it.

@@ -193,7 +193,7 @@ def guess_generating_function_rational(v, X=Symbol('x')):
if n <= 1: return None
# b) compute the numerator as p
p = [sum(v[i-k]*q[k] for k in range(min(i+1, n)))
for i in range(len(v))] # TODO: maybe better with: len(v)>>1
for i in range(len(v)>>1)]
Copy link
Member

Choose a reason for hiding this comment

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

What does this change do? Also len(v)//2 is more readable than len(v) >> 1.

Copy link
Author

Choose a reason for hiding this comment

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

Let's look at find_simple_recurrence_vector(l). Firstly, here is used >> 1 instead of // 2, and I use the same for uniformity. Secondly, len(find_simple_recurrence_vector(l)) <= (len(l) >> 1). In guess_generating_function_rational we can see that sum(v[i-k] * q[k] for k in range(m)) == 0, where m = n = len(q) <= (len(v) >> 1) , and it is unnecessary to calculate it for i >= (len(v) >> 1), because the sum is 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

Verified this is true for i >= (len(v) >> 1). So I have verified with the current tests.

@gxyd
Copy link
Contributor

gxyd commented Oct 11, 2017

Looks good. Merging this. Thanks @abramovat .

@gxyd gxyd merged commit fe63969 into sympy:master Oct 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants