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: refactor propspeed #540

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@matthew-brett
Member

matthew-brett commented Jan 5, 2015

Refactoring propspeed to make me feel better about not understanding or being
able to debug strange error on buildbots.

@matthew-brett matthew-brett force-pushed the matthew-brett:refactor-propspeed branch 2 times, most recently from 1f685be to 583e96a Jan 5, 2015

matthew-brett added some commits Jan 5, 2015

DOC: refactor propspeed docstrings / whitespace
Refactor to docstring format, adjust whitespace to PEP8-ish rules.
RF: remove unused inline functions
Inline functions to get data pointers not being used in this module.

@matthew-brett matthew-brett force-pushed the matthew-brett:refactor-propspeed branch from 583e96a to a3e0b6d Jan 5, 2015

@arokem

This comment has been minimized.

Member

arokem commented Jan 5, 2015

Does this solve that bug on that one buildbot? Otherwise, this refactor should probably wait for 0.9

We'll need to review this, and it's not a trivial amount of work. We should focus on putting out 0.8 first.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jan 5, 2015

I second Ariel's post. Also if you are planning on putting that much amount effort you may consider that the new tracking API made by Bago and Gab should replace EuDX in the future. So, it might be wiser to focus on optimizing and improving that.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jan 5, 2015

Guys - the effort is done. Now I am going to add some checks on top. That will be done soon too.

There's nothing much to review here, just docstring edits and code formatting. Maybe y'all could save time by having a quick look now to confirm this is an improvement.

DOC: more docstring fixes
Fixes to docstrings with better understanding of what the parameters
are.
@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jan 6, 2015

OK - refactoring done. I think it's uncontroversial, and improves the code, making it a little safer against numpy changes.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jan 6, 2015

I will also get travis to build a wheel with these changes on top of the 0.7.1 release to the resulting wheel fixes the buildbot error.

@arokem

This comment has been minimized.

Member

arokem commented Jan 6, 2015

Wait - does this fix that error on your machine? Under 3.3? That was not
clear to me from your previous messages

On Monday, January 5, 2015, Matthew Brett notifications@github.com wrote:

I will also get travis to build a wheel with these changes on top of the
0.7.1 release to the resulting wheel fixes the buildbot error.


Reply to this email directly or view it on GitHub
#540 (comment).

@matthew-brett matthew-brett changed the title from WIP: refactor propspeed to MRG: refactor propspeed Jan 6, 2015

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jan 6, 2015

I built the 0.7.1 wheel with the patches from this PR, and failing test does now pass. Wheel build here: https://travis-ci.org/matthew-brett/dipy-wheels/builds/46025087 . Wheels at http://wheels.scipy.org/ . I propose to upload the fixed (patched) wheel for Python 3.3 to pypi.

@arokem

This comment has been minimized.

Member

arokem commented Jan 6, 2015

I don't understand - you want to upload this instead of 0.7.1? It's
completely different

On Mon, Jan 5, 2015 at 6:43 PM, Matthew Brett notifications@github.com
wrote:

I built the 0.7.1 wheel with the patches from this PR, and failing test
does now pass. Wheel build here:
https://travis-ci.org/matthew-brett/dipy-wheels/builds/46025087 . Wheels
at http://wheels.scipy.org/ . I propose to upload the fixed (patched)
wheel for Python 3.3 to pypi.


Reply to this email directly or view it on GitHub
#540 (comment).

@arokem

This comment has been minimized.

Member

arokem commented Jan 6, 2015

Sorry - that was a brainfart on my part. More power to you!

@arokem

This comment has been minimized.

Member

arokem commented Jan 6, 2015

I guess the answer to my question is a yes? This does fix that bug? In that case, I will review this later tonight.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jan 6, 2015

Yes, if I rebase the changes in this PR on top of 0.7.1 - and rebuild a wheel with that code, then the test failure is fixed for Python 3.3. The wheel is then the same as for 0.7.1 apart from these changes. Actual branch I built the wheel from is : https://github.com/matthew-brett/dipy/tree/tmp . So yes, this appears to fix a bug.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jan 6, 2015

When reviewing, maybe review by commit, because all but the last commit are docstring / formatting cleanups.

RF+TST: use numpy approved data get / flag checks
Check that arrays that should be C-contiguous are in fact C-contiguous
for public-facing functions.

Use PyArray_DATA to get array data pointer, to adapt to newer numpy API.

Add tests for modified functions.

@matthew-brett matthew-brett force-pushed the matthew-brett:refactor-propspeed branch from 4463886 to a388a24 Jan 6, 2015

@arokem

This comment has been minimized.

arokem commented on dipy/tracking/propspeed.pyx in a388a24 Jan 6, 2015

Is this the part that deals directly with the bug that was detected on that python 3.3 bot? Just trying to understand.

@arokem

This comment has been minimized.

arokem commented on dipy/tracking/propspeed.pyx in a388a24 Jan 6, 2015

Or is this the crucial bit?

This comment has been minimized.

Owner

matthew-brett replied Jan 6, 2015

I'm afraid I don't know which bit is the crucial bit. I suspect the PyArray_DATA changes are the crucial ones, but I didn't test which of these makes a difference.

DOC+RF: add more comments to tests
Explain tests for non-contiguous arrays in propspeed.
cnp.ndarray[double, ndim=1] result):
''' trilinear interpolation (isotropic voxel size)
def map_coordinates_trilinear_iso(
cnp.ndarray[double, ndim=3] data,

This comment has been minimized.

@Garyfallidis

Garyfallidis Jan 6, 2015

Member

We usually align parameters below the first parameter.

This comment has been minimized.

@matthew-brett

matthew-brett Jan 6, 2015

Member

​That would cause a line longer than 78 characters, hence the change.

This comment has been minimized.

@Garyfallidis

Garyfallidis Jan 6, 2015

Member

Yeah, I see that data_strides goes a bit out. But look below too. There are other places that this is not the case.

_trilinear_interpolation_iso(&ps[i*3],<double *>w,<cnp.npy_intp *>index)
rs[i]=0
_trilinear_interpolation_iso(
&ps[i * 3],

This comment has been minimized.

@Garyfallidis

Garyfallidis Jan 6, 2015

Member

indentation

''' interpolate in 3d volumes given point X
cdef void _trilinear_interpolation_iso(
double *X,
double *W,

This comment has been minimized.

@Garyfallidis

Garyfallidis Jan 6, 2015

Member

same as above

odf_vertices : array, shape(N,3), float, sampling directions on the sphere
cdef cnp.npy_intp _nearest_direction(
double* dx,

This comment has been minimized.

@Garyfallidis

Garyfallidis Jan 6, 2015

Member

same as above.

cnp.npy_intp *qa_shape,cnp.npy_intp* strides,\
double *direction,double total_weight) nogil:
cdef cnp.npy_intp _propagation_direction(
double *point,

This comment has been minimized.

@Garyfallidis

Garyfallidis Jan 6, 2015

Member

same as above

double new_direction[3] #new propagation direction
double w[8],qa_tmp[PEAK_NO],ind_tmp[PEAK_NO]
cnp.npy_intp index[24],i,j,m,xyz[4]
double total_w=0 # total weighting useful for interpolation

This comment has been minimized.

@Garyfallidis
@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jan 6, 2015

Okay, apart from a few pep8 problems. I think this is ready. I used it also with some real data and the tracking looks good. Matthew here is using cnp.PyArray_DATA to access numpy arrays. We may want to use this trick in other places too.

@arokem

This comment has been minimized.

Member

arokem commented Jan 6, 2015

Writing up a new PR to address the PEP8 issues. Should be up in just a
minute.

On Tue, Jan 6, 2015 at 8:47 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Okay, apart from a few pep8 problems. I think this is ready. I tried also
with some real data and the tracking looks good. Matthew here is using
cnp.PyArray_DATA to access numpy arrays. We may want to use this trick in
other places too.


Reply to this email directly or view it on GitHub
#540 (comment).

@arokem arokem referenced this pull request Jan 6, 2015

Merged

Refactor propspeed - updated #544

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jan 6, 2015

Closing in favor of #544

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jan 6, 2015

Guys - OK to upload the patched wheel to pypi?

@arokem

This comment has been minimized.

Member

arokem commented Jan 6, 2015

Seems fine to me.

On Tue, Jan 6, 2015 at 10:09 AM, Matthew Brett notifications@github.com
wrote:

Guys - OK to upload the patched wheel to pypi?


Reply to this email directly or view it on GitHub
#540 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment