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

Cythonized version of streamlines' resample() and length() functions. #382

Merged
merged 12 commits into from Jul 8, 2014

Conversation

Projects
None yet
5 participants
@MarcCote
Contributor

MarcCote commented Jun 23, 2014

I made a cythonized version of the resampling and length functions found in tracking/metrics.py. As suggested by @Garyfallidis, I put these new functions in core/streamlinespeed.pyx accessible through core/streamline.py.

I made a minor change to the Makefile and setup.py to add the compilation of the new cython file. Also, regarding the language use for compiling Cython, I changed it to C++ instead of C since I needed vector from C++ std and also it was not breaking anything except one thing that I also fixed (see commit logs). If you don't want to change the compiler for Cython, I can set it only to compile core/streamlinespeed.pyx instead.

MarcCote added some commits Jun 18, 2014

Added streamline.py in dipy/core and cythonized resample() and length()
streamline.py will contain everything related to streamlines representation.
Right now, two functions are moved there from tracking/metrics.py: resample() and length().

streamlinespeed.pyx contains the Cython part of streamline.py's functions.

setup.py has been changed to compile Cython with C++ instead of C since streamlinespeed.pyx
 uses C++'s vector to represent list of numpy array (i.e. list of streamlines).

reconst/quick_squash.pyx needed to be modified as compiling it with C++ failed. So, instead
of declaring `dtypes` as an memoryview of object, it is now declared as a memoryview of
numpy.dtype .
Added docstring and more unit tests for resample() and length().
Fixed missing parenthesis in metrics.py
assert_raises(AttributeError, dipystreamline.length, streamline)
def speedup_resample():

This comment has been minimized.

@matthew-brett

matthew-brett Jun 23, 2014

Member

Can you put these guys into a bench_streamline routine or similar? See e.g dipy/reconst/benchmarks

This comment has been minimized.

@MarcCote

MarcCote Jun 23, 2014

Contributor

Done. See dipy/core/benchmarks

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jun 25, 2014

@MarcCote Travis is failing... on Python 3

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jun 25, 2014

@Garyfallidis I fixed it, thanks.

@arokem

This comment has been minimized.

Member

arokem commented Jul 2, 2014

Hey @MarcCote - I am trying to compile this on my machine and I am getting the following error:

dipy/tracking/distances.cpp:10602:19: error: use of undeclared identifier
      'isnan'; did you mean 'std::isnan'?
    __pyx_t_12 = (dpy_isnan(__pyx_v_tmp) != 0);
                  ^
src/dpy_math.h:9:19: note: expanded from macro 'dpy_isnan'
#define dpy_isnan npy_isnan
                  ^
/Users/arokem/anaconda/lib/python2.7/site-    packages/numpy/core/include/numpy/npy_math.h:170:34: note: 
      expanded from macro 'npy_isnan'
            #define npy_isnan(x) isnan(x)
                             ^
/usr/include/c++/4.2.1/cmath:551:5: note: 'std::isnan' declared here
    isnan(_Tp __f) { return ::__gnu_cxx::__capture_isnan(__f); }
    ^
2 warnings and 1 error generated.
error: command 'gcc' failed with exit status 1

Any ideas? Maybe you could tell me how dpy_math.h is supposed to work?

Thanks!

@MrBago

This comment has been minimized.

Contributor

MrBago commented Jul 2, 2014

I'm also having trouble compiling this, though I haven't spent much time on it yet. Also I've always used a != a to check for nans, maybe that could be a workaround here?

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jul 2, 2014

@arokem and @MrBago what OS are you using? I'm running on Linux so it might explained why Travis and I are able to compile but not you.

If it is a problem, I guess we could just specify streamlinespeed.pyx to be compiled in C++ and leave the others .pyx be compiled in C. I'll look if I get some warning when I compiled distances.pyx

@arokem

This comment has been minimized.

Member

arokem commented Jul 2, 2014

Sorry - should have included that information - Mac OS 10.9.3. I'll try
what you suggested (C++/C).

On Wed, Jul 2, 2014 at 1:17 PM, Marc-Alexandre Côté <
notifications@github.com> wrote:

@arokem https://github.com/arokem and @MrBago
https://github.com/MrBago what OS are you using? I'm running on Linux
so it might explained why Travis and I are able to compile but not you.

If it is a problem, I guess we could just specify streamlinespeed.pyx to
be compiled in C++ and leave the others .pyx be compiled in C. I'll look if
I get some warning when I compiled distances.pyx


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

@arokem

This comment has been minimized.

Member

arokem commented Jul 2, 2014

Yep. This change in the setup:

arokem@562bbab

seems to compile fine for me, and tests all run and pass. I'll take a look
at the rest of it as well.

On Wed, Jul 2, 2014 at 1:18 PM, Ariel Rokem arokem@gmail.com wrote:

Sorry - should have included that information - Mac OS 10.9.3. I'll try
what you suggested (C++/C).

On Wed, Jul 2, 2014 at 1:17 PM, Marc-Alexandre Côté <
notifications@github.com> wrote:

@arokem https://github.com/arokem and @MrBago
https://github.com/MrBago what OS are you using? I'm running on Linux
so it might explained why Travis and I are able to compile but not you.

If it is a problem, I guess we could just specify streamlinespeed.pyx to
be compiled in C++ and leave the others .pyx be compiled in C. I'll look if
I get some warning when I compiled distances.pyx


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

from nose.tools import assert_true, assert_false, assert_equal, assert_almost_equal
from numpy.testing import assert_array_equal, assert_array_almost_equal, assert_raises
import dipy.core.streamline as dipystreamline

This comment has been minimized.

@arokem

arokem Jul 2, 2014

Member

I am not sure this belongs in dipy.core. How about putting it in dipy.tracking?

This comment has been minimized.

@MarcCote

MarcCote Jul 2, 2014

Contributor

I let @Garyfallidis answer this question.

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 3, 2014

Member

The idea here is that we can have in dipy.core.streamline different ways to represent streamlines. Tracking for me is more about creating streamlines rather than the streamlines themselves. This is in the same way of thinking as we did with gradients and spheres etc. Streamlines don't need a class by themselves but you could have a streamline represented in a discrete way or continuous way or resample a streamline (as you can interpolate with RBFs a sphere) etc. Makes sense @arokem ?

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 3, 2014

Member

@emanuele has also a way to represent a streamline as vectors using other streamlines. This would also go to this streamline.py file in dipy.core.

This comment has been minimized.

@arokem

arokem Jul 3, 2014

Member

OK - I am just worried about making core a dumping ground for stuff that
doesn't clearly belong anywhere else. I think that core should be
reserved for stuff that is required by all the other modules. Maybe this is
the case here. What is the typical use-case for resampling tracks?
Segmentation? Maybe this should go under segment?

On Wed, Jul 2, 2014 at 7:06 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

In dipy/core/tests/test_streamline.py:

@@ -0,0 +1,279 @@
+from future import print_function
+
+import unittest
+import numpy as np
+
+from nose.tools import assert_true, assert_false, assert_equal, assert_almost_equal
+from numpy.testing import assert_array_equal, assert_array_almost_equal, assert_raises
+
+import dipy.core.streamline as dipystreamline

The idea here is that we can have in dipy.core.streamline different ways
to represent streamlines. Tracking for me is more about creating
streamlines rather than the streamlines themselves.


Reply to this email directly or view it on GitHub
https://github.com/nipy/dipy/pull/382/files#r14494402.

This comment has been minimized.

@MrBago

MrBago Jul 3, 2014

Contributor

I agreed, core should be reserved for things that are used by many other
modules, ie gradients.

Bago
On Jul 3, 2014 9:36 AM, "Ariel Rokem" notifications@github.com wrote:

In dipy/core/tests/test_streamline.py:

@@ -0,0 +1,279 @@
+from future import print_function
+
+import unittest
+import numpy as np
+
+from nose.tools import assert_true, assert_false, assert_equal, assert_almost_equal
+from numpy.testing import assert_array_equal, assert_array_almost_equal, assert_raises
+
+import dipy.core.streamline as dipystreamline

OK - I am just worried about making core a dumping ground for stuff that
doesn't clearly belong anywhere else. I think that core should be
reserved for stuff that is required by all the other modules. Maybe this is
the case here. What is the typical use-case for resampling tracks?
Segmentation? Maybe this should go under segment?
… <#146fd15d9666b174_>
On Wed, Jul 2, 2014 at 7:06 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote: In dipy/core/tests/test_streamline.py: >
@@ -0,0 +1,279 @@ > +from future import print_function > + > +import
unittest > +import numpy as np > + > +from nose.tools import assert_true,
assert_false, assert_equal, assert_almost_equal > +from numpy.testing
import assert_array_equal, assert_array_almost_equal, assert_raises > + >
+import dipy.core.streamline as dipystreamline The idea here is that we can
have in dipy.core.streamline different ways to represent streamlines.
Tracking for me is more about creating streamlines rather than the
streamlines themselves. — Reply to this email directly or view it on GitHub
https://github.com/nipy/dipy/pull/382/files#r14494402.


Reply to this email directly or view it on GitHub
https://github.com/nipy/dipy/pull/382/files#r14523956.

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 3, 2014

Member

Okay, I see you both don't like my suggestion. I have no strong position on that. So, @MarcCote please move your functions inside dipy.tracking

This comment has been minimized.

@MarcCote

MarcCote Jul 3, 2014

Contributor

Before I moved it, I will add that streamlines are/will be used by the tracking, segment and viz modules. Which seems to me as many modules. If you plan to do bundle analysis and connectomics, they will be useful in those modules too. Your call.

len1 = cumlen[ind]
Ds = distance-len0
Lambda = Ds/(len1-len0)
return Lambda*xyz[ind]+(1-Lambda)*xyz[ind-1]

This comment has been minimized.

@arokem

arokem Jul 2, 2014

Member

PEP8: space around operators

cumlen = np.zeros(xyz.shape[0])
cumlen[1:] = length_python(xyz, along=True)
step = cumlen[-1]/(n_pols-1)

This comment has been minimized.

@arokem

arokem Jul 2, 2014

Member

PEP8

This comment has been minimized.

@MarcCote

MarcCote Jul 2, 2014

Contributor

What's wrong with this line?

This comment has been minimized.

@arokem

arokem Jul 2, 2014

Member

No big deal - just spaces around operators : cumlen[-1] / (n_pols - 1)

On Wed, Jul 2, 2014 at 3:26 PM, Marc-Alexandre Côté <
notifications@github.com> wrote:

In dipy/core/tests/test_streamline.py:

  • return np.sum(dists)

+def resample_python(xyz, n_pols=3):

  • def _extrap(xyz, cumlen, distance):
  •    ''' Helper function for extrapolate '''
    
  •    ind = np.where((cumlen-distance) > 0)[0][0]
    
  •    len0 = cumlen[ind-1]
    
  •    len1 = cumlen[ind]
    
  •    Ds = distance-len0
    
  •    Lambda = Ds/(len1-len0)
    
  •    return Lambda_xyz[ind]+(1-Lambda)_xyz[ind-1]
    
  • cumlen = np.zeros(xyz.shape[0])
  • cumlen[1:] = length_python(xyz, along=True)
  • step = cumlen[-1]/(n_pols-1)

What's wrong with this line?


Reply to this email directly or view it on GitHub
https://github.com/nipy/dipy/pull/382/files#r14487820.

step = cumlen[-1]/(n_pols-1)
ar = np.arange(0, cumlen[-1], step)
if np.abs(ar[-1]-cumlen[-1]) < np.finfo('f4').eps:

This comment has been minimized.

@arokem

arokem Jul 2, 2014

Member

PEP8

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jul 2, 2014

I applied commit d606735 proposed by @arokem.

free(arclengths)
def resample(streamlines, nb_points=3):

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 3, 2014

Member

Please rename to:
set_number_of_points

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 3, 2014

Also it will be used in the bundle module which will provide tools to do tractometry, AFQ and other bundle-based statistics. This is coming with my bundle registration PR.

@cython.cdivision(True)
@cython.boundscheck(False)
cdef void _set_number_of_points(Streamlines streamlines, Streamlines out) nogil:
cdef unsigned int N

This comment has been minimized.

@Garyfallidis

Garyfallidis Jul 3, 2014

Member

You can use one cdef: for all of these variables if you want. I am sure you know how.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 3, 2014

But, anyway this PR is very good Marc-Alex and the coverage is good too. Please move your functions in dipy.tracking and then we could maybe move these to dipy.core later but at the moment it is true that their is no need for that as there is already the dipy.tracking module being used for similar things. Keep it up!

@arokem

This comment has been minimized.

Member

arokem commented Jul 3, 2014

+1 - really nice!

On Thu, Jul 3, 2014 at 1:59 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

But, anyway this PR is very good Marc-Alex and the coverage is good too.
Please move your functions in dipy.tracking and then we could maybe move
these to dipy.core later but at the moment it is true that their is no need
for that as there is already the dipy.tracking module being used for
similar things. Keep it up!


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

MarcCote added some commits Jul 4, 2014

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jul 4, 2014

Ok, I moved it to dipy.tracking.

@arokem

This comment has been minimized.

Member

arokem commented Jul 4, 2014

Cool. Let's give people another couple of days to take a look if they want. If no one objects, I can merge this on Monday.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 8, 2014

@arokem is away for vacation and most likely forgot to merge this. If everyone is fine would like to go ahead and merge this.

arokem added a commit that referenced this pull request Jul 8, 2014

Merge pull request #382 from MarcCote/resample_cython
Cythonized version of streamlines' resample() and length() functions.

@arokem arokem merged commit 6e22849 into nipy:master Jul 8, 2014

1 check passed

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

This comment has been minimized.

Member

arokem commented Jul 8, 2014

Beat you to it :-)

Thanks @MarcCote!

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 8, 2014

LOL! :)

@MarcCote MarcCote deleted the MarcCote:resample_cython branch Sep 4, 2014

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