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

MAINT: Cython code maintenance #5319

Merged
merged 5 commits into from Oct 10, 2015

Conversation

Projects
None yet
5 participants
@larsmans
Copy link
Contributor

larsmans commented Oct 6, 2015

Optimizing the Cython code for size and fixing buglets along the way. Should reduce the installation footprint by at least a few hundred kB and give some marginal speedups.

larsmans added some commits Oct 6, 2015

MAINT: interpolate: optimize interpnd Cython code
Optimized for code size, not speed, though it should become
(marginally) faster.
MAINT: cluster: fix and optimize _hierarchy.pyx
* C int corresponds to np.intc, not necessarily np.int32
* Removed some dead code and optimized for code size
MAINT: special: optimize _ellip_harm for code size
This one needs some more work; it's memory allocation and cleanup
is hard to follow.
MAINT: cluster: fixes to _vq
* intc not int32 for C int
* _vq would not propagate exceptions due to void
* don't rely on C99 function sqrtf

@larsmans larsmans force-pushed the larsmans:cython-maint branch from 8d36184 to fb9cea3 Oct 7, 2015

@ev-br ev-br added the maintenance label Oct 7, 2015

@@ -44,7 +47,7 @@ def givens_elimination(double [:, ::1] S, double [:] v, double [:] diag):
if diag[i] == 0:
continue

diag_row[:] = 0
diag_row[i+1:] = 0

This comment has been minimized.

Copy link
@rgommers

rgommers Oct 10, 2015

Member

It's not obvious to me that this change is correct.

The code currently starts by setting diag_row to all zeros except for element i. Then in the inner loop it touches only elements i..n. The comment diag_row[j] is implicitly 0 now is not very clear. The new and old code are only equivalent is diag_row[i] really is reset to 0 within dlartg it looks like to me.

@nmayorov you want to have a look at this one?

This comment has been minimized.

Copy link
@nmayorov

nmayorov Oct 10, 2015

Contributor

In the loop for j in range(i, n) only diag_row[j:] is used and modified. The is idea is to sequentially zero out j-th element of diag_row (starting with j=i), but as this element is never used anymore I wrote diag_row[j] is implicitly 0 now, i.e. we don't have to set it to 0.

So this optimization looks correct for me. And tests are passing too.

The new and old code are only equivalent is diag_row[i] really is reset to 0

Can you explain why it seems to you like this? Maybe I'm missing something.

This comment has been minimized.

Copy link
@rgommers

rgommers Oct 10, 2015

Member

The new and old code are only equivalent is diag_row[i] really is reset to 0

Can you explain why it seems to you like this? Maybe I'm missing something.

Probably because I'm reading too fast, but the current code does:

diag_row[:] = 0
diag_row[i] = diag[i]

in each iteration, so will contain [0, 0, 0, 0.23432, 0, 0, 0] (for i==3). The new code only resets elements i+1:, so (for n==4) could contain [0.11, 0.22, 0.33, 0.23432, 0, 0, 0].

The above isn't true if the comment about implicit 0 really resets element j to identically 0, which I didn't verify.

This comment has been minimized.

Copy link
@nmayorov

nmayorov Oct 10, 2015

Contributor

in each iteration, so will contain [0, 0, 0, 0.23432, 0, 0, 0](for i==3). The new code only resets elements i+1:, so (for n==4) could contain [0.11, 0.22, 0.33, 0.23432, 0, 0, 0]

Yes, but [0.11, 0.22, 0.33] won't be used in any way. Does it make sense to you now?

This comment has been minimized.

Copy link
@rgommers

rgommers Oct 10, 2015

Member

Ah allright, makes sense.

This comment has been minimized.

Copy link
@rgommers
@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Oct 10, 2015

Everything else in this PR looks good to me.

@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Oct 10, 2015

@richardtsai you may be interested in seeing the fixes to _vq and _hierarchy. The intc vs int32 in particular (see https://github.com/scikit-learn/scikit-learn/wiki/C-integer-types:-the-missing-manual)

@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Oct 10, 2015

@larsmans what do you think about adding your C integer guide in a more visible place like the Numpy user guide? It's a very helpful piece of text.

@rgommers rgommers added this to the 0.17.0 milestone Oct 10, 2015

rgommers added a commit that referenced this pull request Oct 10, 2015

Merge pull request #5319 from larsmans/cython-maint
MAINT: Cython code maintenance

@rgommers rgommers merged commit cf9ea6a into scipy:master Oct 10, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Oct 10, 2015

Okay, in it goes. Thanks @larsmans!

@richardtsai

This comment has been minimized.

Copy link
Contributor

richardtsai commented Oct 11, 2015

looks good. BTW, it seems that cluster lacks a proper benchmark. Maybe we should have one so that we can track these kinds of optimization more easily.

@rgommers

This comment has been minimized.

Copy link
Member

rgommers commented Oct 11, 2015

Indeed. Ideally we'd like to have benchmarks for all important functionality, and we're moving towards requiring benchmarks for major performance improvements (but not for small optimizations like the ones in this PR).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.