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

MRG: some Cython refactorings #533

Merged
merged 6 commits into from Jul 30, 2015

Conversation

Projects
None yet
4 participants
@matthew-brett
Member

matthew-brett commented Dec 30, 2014

Including start of review of uses of int and size_t for things that refer to
array sizes / shapes.

@matthew-brett matthew-brett force-pushed the matthew-brett:cython-review branch 2 times, most recently from 21174ee to 75ca096 Dec 30, 2014

@@ -203,16 +223,15 @@ def search_descending(cnp.ndarray[cnp.float_t, ndim=1, mode='c'] a,
i : int
The greatest index such that ``all(a[:i] >= relative_threshold *

This comment has been minimized.

@omarocegueda

omarocegueda Feb 11, 2015

Contributor

The first thing I wondered when I read the doc string was "what happens if no such index i exist?, how am I going to catch that case?", this line answers the question but it was a little cryptic for me at the beginning. To help the inexperienced reader (like me), can we add something like "If no such index 'i' exists, the length of a is returned instead." in the description of the function? (Is that statement correct?).

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Feb 26, 2015

Hi guys, I don't have further comments on this PR (I was a bit concern about performance but I even observed a small speed up =) ), I think it is ok to be merged (the documentation suggestion is not so important). I just have a question: I used this PR as "guidelines" to refactor another .pyx, how are we going to proceed here? are we going to make several PRs doing this kind of refactorings or should we make PRs on top of this one and merge until we have finish refactoring everything? (if we are going to do several PRs, we should merge this one and keep going, no?)

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Feb 26, 2015

Omar - I suggest that we merge this one (if you are happy with my last doc edits) then submit more PRs with refactoring. I actually have some more refactorings on top of these, but I wanted to keep this PR stable so y'all could review it and sign off without it getting confusing with more changes on top.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Feb 26, 2015

I wonder whether we should make a little Cython style guide?

@arokem arokem referenced this pull request Mar 2, 2015

Open

Make a cython style guide #583

@stefanv stefanv modified the milestone: 0.9 Mar 4, 2015

@arokem

This comment has been minimized.

Member

arokem commented Apr 30, 2015

Any reasons this hasn't been merged?

@matthew-brett matthew-brett changed the title from WIP: some Cython refactorings to MRG: some Cython refactorings Apr 30, 2015

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Apr 30, 2015

No reason from my side.

r"""
Wrapper for _solve_2d_symmetric_positive_definite (see documentation above)
Wrapper for _solve_2d_symmetric_positive_definite

This comment has been minimized.

@arokem

arokem Apr 30, 2015

Member

Since this is the public interface, it's confusing to call it a "wrapper". Can we change that to say what the 'wrapped' function does?

This comment has been minimized.

@arokem

arokem Apr 30, 2015

Member

I know you didn't write this in this PR, but you did bring it to my attention ;-)

This comment has been minimized.

@matthew-brett

matthew-brett Jul 30, 2015

Member

I edited the docstring.

@@ -91,12 +113,35 @@ cdef int _solve_3d_symmetric_positive_definite(double[:] g, double[:] y, double
return 0
def solve_3d_symmetric_positive_definite(double[:] g, double[:] y, double tau,
double[:] out):
def solve_3d_symmetric_positive_definite(g, y, double tau):
r"""
Wrapper for _solve_3d_symmetric_positive_definite (see documentation above)

This comment has been minimized.

@arokem

arokem Apr 30, 2015

Member

Same applies here (and you would probably want to edit it in a corresponding manner anyway)

This comment has been minimized.

@arokem

arokem Apr 30, 2015

Member

And by "same" I mean that a public interface wrapping a function should probably just state what the "wrapped" function does.

@@ -54,10 +54,11 @@ cdef void splitoffset(float *offset, size_t *index, size_t shape) nogil:
def trilinear_interp(cnp.ndarray[cnp.float32_t, ndim=4, mode='strided'] data,
cnp.ndarray[cnp.float_t, ndim=1, mode='strided'] index,
cnp.ndarray[cnp.float_t, ndim=1, mode='c'] voxel_size):
"""Interpolates data at index
""" Interpolates vector from 4D `data` at 3D point given by `index`

This comment has been minimized.

@arokem

arokem Apr 30, 2015

Member

Maybe put a newline in there, instead of that space (at the very beginning of this line). If you feel like it, you can put newlines at the beginning of many docstrings in these files...

@@ -278,7 +316,7 @@ def local_maxima(cnp.ndarray odf, cnp.ndarray edges):
@cython.wraparound(False)
@cython.boundscheck(False)
cdef void _cosort(double[::1] A, cnp.npy_intp[::1] B) nogil:
"""Sorts A inplace and applies the same reording to B"""
""" Sorts `A` in-place and applies the same reordering to `B`"""

This comment has been minimized.

@arokem

arokem Apr 30, 2015

Member

Newline instead of space

This comment has been minimized.

@matthew-brett
@arokem

This comment has been minimized.

Member

arokem commented Apr 30, 2015

OK - I had just a couple of tiny comments here. Let me know when you want me to press that button.

@arokem

This comment has been minimized.

Member

arokem commented Jun 24, 2015

@matthew-brett - could you please rebase this, so I can merge? Sorry that this has taken so long.

matthew-brett added some commits Dec 30, 2014

RF: refactor sumsqdiff to use C arrays
Use C arrays when possible in sumsqdiff calculations.

Make symmetric solvers robust to input arguments, and return output
rather than writing in-place; writing in-place only likely useful at C /
Cython level.
DOC: rewrite docstrings / fix whitespace
Rewrite some docstrings, clean out some extra and trailing whitespace.
RF: refactor remove_similar_vertices
Rename some variables, refactor return variable logic, add comments.

Logic almost the same, except for raising error for vertices list of
exactly length 2**16 instead of > length 2**16.
RF: fix typo in variable name; use npy_intp
Fix typo in 'relative_threshold'.  Use npy_intp for index into array
rather than size_t.
DOC: expand docstring for search_descending
From comments by Omar on PR.
DOC: fix docstrings from review by Ariel
Fill out docstrings for public interface functions, remove leading
spaces in some docstrings.

@matthew-brett matthew-brett force-pushed the matthew-brett:cython-review branch from 6f3044a to 478c88a Jul 30, 2015

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jul 30, 2015

I rebased, ready to merge, I believe.

@arokem

This comment has been minimized.

Member

arokem commented Jul 30, 2015

Thanks Matthew! I will let Travis finish his thing and then press that button.

BTW - the whole Travis thing will be faster once this two-line PR is merged: #676

arokem added a commit that referenced this pull request Jul 30, 2015

Merge pull request #533 from matthew-brett/cython-review
MRG: some Cython refactorings

@arokem arokem merged commit 10eb504 into nipy:master Jul 30, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment