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

Streamline-based Linear Registration #446

Merged
merged 111 commits into from Oct 28, 2014

Conversation

Projects
None yet
2 participants
@Garyfallidis
Member

Garyfallidis commented Oct 12, 2014

Hi all,

This is the first PR for the new crème de la crème method that I presented in ISMRM.
http://scil.dinf.usherbrooke.ca/wp-content/papers/garyfallidis_etal_ismrm14.pdf

In accordance with @omarocegueda's work you first define a StreamlineDistanceMetric then give this to StreamlineLinearRegistration which has a method called optimize which takes the two bundles (sets of streamlines) as input and returns a StreamlineRegistrationMap which can transform the moving bundle to the static.

I have only one simple tutorial (see bundle_registration.py) for now but I am working with @jchoude to put some streamline datasets online and create a couple more detailed tutorials both for bundle-based, full brain streamline-based registration and bundle-specific atlas creation. But that will come in another PR (probably multiple PRs) most likely after the release.

In all files the coverage should be high. But in optimize.py the coverage cannot be that high because
some of the tests require different versions of scipy therefore those will not run in your machine
if you don't have those scipy versions.

Also, when you build the modules a new config.pxi file will be created in dipy/build. We use this file to check
if your compiler supports OpenMP or not. See dipy/align/bundlemin.pyx to understand how this enables
us to enable/disable functions and imports at compilation time.

@arokem can you prioritize looking at this PR for this release? I will start working on the LiFE PR.

Enjoy your new toy ;)

Garyfallidis added some commits Jan 27, 2014

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 16, 2014

Hi @arokem thank you very much for your review. I finished with my corrections, I think this PR is much better now after your suggestions. Higher up in the comments I have also addressed all your questions. Let me know what you think.

@arokem

This comment has been minimized.

Member

arokem commented Oct 16, 2014

Thanks for the quick turnaround! I will take a look again in a few days. If
someone else has an interest in this, they are welcome to take a look as
well. If no one else brings up any issues before that, I will review again,
and merge when appropriate.

On Thu, Oct 16, 2014 at 11:36 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Hi @arokem https://github.com/arokem thank you very much for your
review. I finished with my corrections, I think this PR is much better now
after your suggestions. Higher up in the comments I have also addressed all
your questions. Let me know what you think.


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

block_size)
def rotation_vec2mat(r):

This comment has been minimized.

@arokem

arokem Oct 18, 2014

Member

I still find it confusing that this is part of the public API of this module. The simplest would be to just change the one single time this function is called to implement the code that's in here.

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 27, 2014

Member

okay will change

rads = np.deg2rad(t[3:6])
T[0:3, 3] = _threshold(t[0:3], MAX_DIST)
R = rotation_vec2mat(rads)

This comment has been minimized.

@arokem

arokem Oct 18, 2014

Member

Here, replace with:

 theta = np.linalg.norm(rads)
 R = rodrigues_axis_rotation(r, np.rad2deg(theta))

This comment has been minimized.

@Garyfallidis
return T
def from_matrix44_rigid(mat):

This comment has been minimized.

@arokem

arokem Oct 18, 2014

Member

This also belongs in dipy.core.geomtery

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 28, 2014

Member

Okay I have good news. I replaced all matrix44 related functions with compose_matrix so all these is going away now.

return rodrigues_axis_rotation(r, np.rad2deg(theta))
def rotation_mat2vec(R):

This comment has been minimized.

@arokem

arokem Oct 18, 2014

Member

Again - this shouldn't be a part of the public API of this module. Either it belongs in dipy.core.geometry, or better, refactor to use these lines of code above. Imagine the beginner who discovers that there are several different functions (in different places) with the same name, that do slightly different things. Confusing...

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 28, 2014

Member

Same as above :)

R = mat[:3, :3]
if det(R) < 0:
R = -R
vec[3:6] = rotation_mat2vec(R)

This comment has been minimized.

@arokem

arokem Oct 18, 2014

Member

Here, simply replace with:

  ax, angle = quat2angle_axis(mat2quat(R))
  vec[3:6] = ax * angle

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 28, 2014

Member

Will be removed in total :)

----------
.. [Garyfallidis14] Garyfallidis et al., "Direct native-space fiber
bundle alignment for group comparisons", ISMRM,
2014.

This comment has been minimized.

@arokem

arokem Oct 18, 2014

Member

Maybe add the URL? Will make it easier for users to find the abstract.

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 28, 2014

Member

The document should be googleable already. Did you try to google it? Also I submitted a paper last week. When it gets accepted I will update the docs here. Sounds good?

This comment has been minimized.

@arokem

arokem Oct 28, 2014

Member

yeah - no problem

Returns
-------
map : StreamlineRegistrationMap

This comment has been minimized.

@arokem

arokem Oct 18, 2014

Member

Should be:

map : `StreamlineRegistrationMap` class instance

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 28, 2014

Member

Really? Isn't that obvious that this is a class instance?
Where did you found that I need to write class instance? Maybe I missed that in the numpy docs or somewhere? I don't think that we use this convention often. Let me know if I am wrong.

This comment has been minimized.

@arokem

arokem Oct 28, 2014

Member

this is fine

Methods
-------
transform()

This comment has been minimized.

@arokem

arokem Oct 18, 2014

Member

I'd remove the Methods section here. We don't consistently write these anyway, and the html docs will render the methods and their documentation below anyway

This comment has been minimized.

@Garyfallidis

This comment has been minimized.

@arokem

arokem Oct 28, 2014

Member

I think you might have forgotten to do this?

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 28, 2014

Member

Wait for it...

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 28, 2014

Member

Done! Sorry I removed it only from the Registration class and not the Map class. Is done now!

This comment has been minimized.

@arokem

arokem Oct 28, 2014

Member

Now just those array shapes...

On Tue, Oct 28, 2014 at 2:38 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

In dipy/align/streamlinear.py:

  •    fopt : float,
    
  •        final value of the metric
    
  •    matrix_history : array
    
  •        All transformation matrices created during the optimization
    
  •    funcs : int,
    
  •        Number of function evaluations of the optimizer
    
  •    iterations : int
    
  •        Number of iterations of the optimizer
    
  •    Methods
    

  •    transform()
    

Done! Sorry I removed it only from the Registration class and not the Map
class. Is done now!


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

-----
Faster implementation of the ``bundle_min_distance``. This is to
be used after you have called ``unlist_streamlines`` which returns all the
points of all the streamlines as one ndarray.

This comment has been minimized.

@arokem

arokem Oct 18, 2014

Member

Gotcha. So that's the reason. I wouldn't necessarily put it this way, though.

Rather, I'd say something like:

This is a faster implementation of ``bundle_min_distance``, which requires that all the points of each streamline are allocated into an ndarray (of shape N*M by 3, with N the number of points per streamline and M the number of streamlines). This can be done by calling `dipy.tracking.streamlines.unlist_streamlines`. 

This comment has been minimized.

@Garyfallidis
If size is 12, t is interpreted as translation + rotation +
scaling + shearing.
static : ndarray

This comment has been minimized.

@arokem

arokem Oct 18, 2014

Member

Please specify the expected shape of this array

This comment has been minimized.

@Garyfallidis

This comment has been minimized.

@arokem

arokem Oct 28, 2014

Member

I think you might have forgotten to do this?

All the points of the static streamlines. With order of streamlines
intact.
moving : ndarray

This comment has been minimized.

@arokem

arokem Oct 18, 2014

Member

Here as well: specify the shape of the expected array

This comment has been minimized.

@Garyfallidis

This comment has been minimized.

@arokem

arokem Oct 28, 2014

Member

Here as well?

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 28, 2014

Member

Wait for it...

This comment has been minimized.

@Garyfallidis

Garyfallidis Oct 28, 2014

Member

Done! And apologies for missing this!

@arokem

This comment has been minimized.

Member

arokem commented Oct 18, 2014

Looking much better! I had a few more comments and suggestions, and we still need to resolve the move_streamlines/transform_streamlines discussion, hopefully with input from @MrBago, but it's getting really close to ready.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 28, 2014

Hi @arokem, I think I answered all your comments and removed also all old matrix44 implementations. Now I use compose_matrix from dipy.core.geometry. I also wrote some tests for compose_matrix and decompose_matrix. Let me know what you think. On my side it seems this baby is ready to be merged.
I will add other pull requests with more tutorials later to show how powerful this method is and why. But now releasing well tested code is the priority! GGT!!!

@arokem

This comment has been minimized.

Member

arokem commented Oct 28, 2014

Looking good. This is real close now - there were just a couple of
documentation issues that have not been addressed yet.

On Tue, Oct 28, 2014 at 1:24 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Hi @arokem https://github.com/arokem, I think I answered all your
comments and removed also all old matrix44 implementation. Now all use
compose_matrix from dipy.core.geometry. I also wrote some tests for
compose_matrix and decompose_matrix. Let me know what you think. On my side
it seems this baby is ready to be merged.
I will add other pull requests with more tutorials later to show how
powerful this method is and why. But now releasing well tested code is the
priority! GGT!!!


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

arokem added a commit that referenced this pull request Oct 28, 2014

Merge pull request #446 from Garyfallidis/slr
Streamline-based Linear Registration

@arokem arokem merged commit 8127b2e into nipy:master Oct 28, 2014

1 check passed

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

This comment has been minimized.

Member

arokem commented Oct 28, 2014

Congratulations! Hurray!

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 28, 2014

Thank you Ariel!

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