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

Make list_cy more idiomatic #3

Merged
merged 1 commit into from May 22, 2021
Merged

Make list_cy more idiomatic #3

merged 1 commit into from May 22, 2021

Conversation

boothby
Copy link
Contributor

@boothby boothby commented Apr 19, 2021

This is a more idiomatic implementation of cy_list, which avoids several performance pitfalls. On my machine, this results in about a 2x improvement in performance. This PR is incompatible with #2; maybe they should be separate tests.

@00sapo
Copy link
Owner

00sapo commented Apr 19, 2021

I had already tried to use cdef list internal_list and list in the signature of make_list, but slowed down the code for me... That's the point: Cython is completely unpredictable

@bconstanzo
Copy link

bconstanzo commented Apr 19, 2021

Chiming in here, I tried basically the same optimization/cleanup that @boothby did, on my machine I got almost an order of magnitude speedup:

In [9]: %timeit list_cy.iterate_list(a_list)
1000000.0007792843
1000000.0007792843
1000000.0007792843
1000000.0007792843
1000000.0007792843
1000000.0007792843
1000000.0007792843
1000000.0007792843
385 ms ± 6.15 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

In [10]: %timeit list_cyo.iterate_list(a_list)
1000000.0007792843
1000000.0007792843
1000000.0007792843
1000000.0007792843
1000000.0007792843
1000000.0007792843
1000000.0007792843
1000000.0007792843
2.71 s ± 182 ms per loop (mean ± std. dev. of 7 runs, 1 loop each)

Where list_cy is the optimized module and list_cyo is the code as you have right now in the repo. Give this a double check, it certainly improves times for cython.

@mcabbott
Copy link

FWIW, the equivalent change to the Julia version would also be more idiomatic, and much quicker:

make_list() = [fill(0.1, 10^4) for _ in 1:10^4];

Similar changes could also be made to iterate_list, but almost all the time is spent in make_list.

@boothby
Copy link
Contributor Author

boothby commented Apr 19, 2021

I noticed that the majority of the time was spent in make_list when I did the implementation for #2, too. @00sapo it might be interesting to time the make_list and iterate_list calls separately.

count += internal_list[j]
cpdef float iterate_list(list a_list):
cdef double count = 0, d
cdef int i

Choose a reason for hiding this comment

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

i is unused here?

new_list.append(0.01)
a_list.append(new_list)
return a_list
return [[0.01]*(10**4) for _ in range(10**4)]

Choose a reason for hiding this comment

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

i, j are also unused here

@00sapo 00sapo merged commit 0e413e4 into 00sapo:main May 22, 2021
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