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

Bundle alignment #333

Closed
wants to merge 72 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@Garyfallidis
Member

Garyfallidis commented Mar 4, 2014

With this PR I am initiating a series of PRs of many new and old algorithms for the registration of DWI data.

This PR concentrates only on bundle registration and analysis for which I will give an oral presentation at the upcoming ISMRM.

I think it is good idea to share this code now that it is still relatively small so that it is easier for you to review.

GGT!

The best way to understand this code is by starting from the short tutorial called bundle_registration.py and then go to the internal functions.

Together with @omarocegueda who has implemented an upcoming PR about non linear registration for DWI we have designed a common API both for streamline and volume registration. The API is as follows:

First you define a similarity or dissimilarity metric (e.g. cross correlation, sum of squared distances etc.):

metric = Metric()

Then you create a Registration class (e.g. Streamline or Diffeomorphic Registration) which takes as input the metric and has a method called optimize.

reg = Registration(metric)
map = reg.optimize(static, moving)

Method optimize takes as input the static(reference image or bundle) and the moving (target image or bundle) and it returns a Map() object (e.g. containing the affine or diffeomorphic transformation).

The Map object has a method called transform which applies the mapping to the given input.

moved = map.transform(moving)

Enjoy :)

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Mar 16, 2014

Anyone looking at this? @mdesco, @demianw, @arokem ?

@demianw

This comment has been minimized.

Contributor

demianw commented Mar 18, 2014

We can talk about this further but, registration is a huge problem that might require a lot of work and be a huge portion of DiPy. Particularly most deformable registration problems are defined in the realm of differential geometry and that might require special care of this in the metrics and transforms. Also, methods are usually very sensitive to the optimiser strategy so an optimiser class is imperative.

I would suggest the following:

  1. You can use this project of mine ;) https://github.com/demianw/pyMedImReg-public which already has many of those problems solved. It has a similar structure, a metric object class, a transform object class, an optimiser and a registration object.
  2. This can be added as a submodule to DiPy and then you can develop the particular metrics for this. A lot of transforms are already defined with some testing. Some transforms work better than others.

@Garyfallidis Garyfallidis changed the title from Bundle alignment to WIP: Bundle alignment Mar 29, 2014

@Garyfallidis Garyfallidis changed the title from WIP: Bundle alignment to Bundle alignment Mar 30, 2014

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Apr 11, 2014

Apologies for the delay. A bit of feedback on this.

Demian visited our lab and we had some great discussion and feedback from him on the registration topic. We mainly discussed what me and Omar have already implemented and what he has available in his registration module. The decision was that Demian's registration has a different objective than ours. He wants to create a much more general package for registration and it seems that much more work and research is needed for that. For now we are only interested in registration for dMRI data.

At the same time Demian got a permanent position at INRIA and he doesn't know how much time he can find to deliver such an ambitious project i.e. a generic registration package. As we need registration in Dipy for group comparisons as soon as possible, we decided that his module should be an independent project for general registration problems and give him the time to improve upon.

Therefore, we will continue developing our registration methods in dipy with focusing on simple registration needed for diffusion MRI data. As Demian's registration package will get better and more tested then we are very happy to refactor our methods to his new package and use that instead.

Please start reviewing this PR asap. GGT!

class Optimizer(object):
def __init__(self, fun, x0, args=(), method='BFGS', jac=None, hess=None,

This comment has been minimized.

@arokem

arokem Apr 11, 2014

Member

I think this is a bug: everywhere else you use the string L-BFGS-B to refer to that (including below within this class...). So it seems that the default behavior wouldn't work here. Might be worth making a test of that

This comment has been minimized.

@Garyfallidis
if scipy_less_0_11:
if method == 'L-BFGS-B':

This comment has been minimized.

@arokem

arokem Apr 11, 2014

Member

Here. What would this do if you did not provide the kwarg method at all?

This comment has been minimized.

@Garyfallidis

Garyfallidis Apr 12, 2014

Member

Cool, will fix.

@arokem

This comment has been minimized.

Member

arokem commented Apr 13, 2014

Please rebase on top of master, so we can see how well this is covered by tests. That's what that PR was really all about. I was too lazy to pull this onto my machine and run the tests with coverage.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Apr 13, 2014

Sure will do asap. Thank you for pointing out.

@arokem

This comment has been minimized.

Member

arokem commented Apr 13, 2014

And apologies for the ambush :-)

I thought that this would be good to have, considering how much emphasis we
put on tests in our developer conference call a few months back. I agree
with you that we have been doing very well in our more recent past. At some
point, we should circle back and cover as much as possible those modules
that are not 100%. For example, I will take a look ASAP at dipy.reconst.dti
to see what are the 10% of code not covered by tests, and cover that.

On Sat, Apr 12, 2014 at 6:56 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Sure will do asap. Thank you for pointing out.


Reply to this email directly or view it on GitHubhttps://github.com//pull/333#issuecomment-40297407
.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Apr 13, 2014

Yeah, we should all do that. I think with only a bit of work we may easily get around 90% in total. GGT!!

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Apr 17, 2014

@matthew-brett I am having a weird problem with cPickles and Python3 in Travis. I thought that the six.moves package was enabling us to save and load correctly pickles in Python 2 and 3. But maybe not always?

Here is the error:

ERROR: dipy.align.tests.test_streamlinear.test_vectorize_streamlines

Traceback (most recent call last):
File "/usr/lib/python3/dist-packages/nose/case.py", line 198, in runTest
self.test(*self.arg)
File "/usr/local/lib/python3.2/dist-packages/dipy/align/tests/test_streamlinear.py", line 442, in test_vectorize_streamlines
cingulum_bundles = load_pickle(fname)
File "/usr/local/lib/python3.2/dist-packages/dipy/io/pickles.py", line 57, in load_pickle
dix=cPickle.load(inp)
Exception: UnicodeDecodeError: ascii
-------------------- >> begin captured stdout << ---------------------
/usr/local/lib/python3.2/dist-packages/dipy/data/cb_2.pkl
--------------------- >> end captured stdout << ----------------------

What I did here is that I created two lists of streamlines as cb_2.pkl (pickle file) and loading them in a test. I did the saving part in Python 2. The commands I used are:
dipy.io.pickles.save_pickle
dipy.io.pickles.load_pickle

Any ideas?

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Apr 18, 2014

Yo,

Try:

fobj = open('cb_2.pkl', 'rb')
res = pickle.load(fobj, encoding='latin-1')

Does that work?

http://stackoverflow.com/questions/11305790/pickle-incompatability-of-numpy-arrays-between-python-2-and-3

On Thu, Apr 17, 2014 at 4:41 PM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

@matthew-brett https://github.com/matthew-brett I am having a weird
problem with cPickles and Python3 in Travis. I thought that the six.moves
package was enabling us to save and load correctly pickles in Python 2 and
3. But maybe not always?
Here is the error: ERROR:
dipy.align.tests.test_streamlinear.test_vectorize_streamlines

Traceback (most recent call last):
File "/usr/lib/python3/dist-packages/nose/case.py", line 198, in runTest
self.test(*self.arg)
File
"/usr/local/lib/python3.2/dist-packages/dipy/align/tests/test_streamlinear.py",
line 442, in test_vectorize_streamlines
cingulum_bundles = load_pickle(fname)
File "/usr/local/lib/python3.2/dist-packages/dipy/io/pickles.py", line 57,
in load_pickle
dix=cPickle.load(inp)
Exception: UnicodeDecodeError: ascii
-------------------- >> begin captured stdout << ---------------------
/usr/local/lib/python3.2/dist-packages/dipy/data/cb_2.pkl
--------------------- >> end captured stdout << ----------------------

What I did here is that I created two lists of streamlines as cb_2.pkl
(pickle file) and loading them in a test. I did the saving part in Python
2. The commands I used are:
dipy.io.pickles.save_pickle
dipy.io.pickles.load_pickle

Any ideas?


Reply to this email directly or view it on GitHubhttps://github.com//pull/333#issuecomment-40774635
.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Apr 18, 2014

Okay, thank you @matthew-brett for the tip but unfortunately it didn't work although I played with it for quite a bit. At the end I found a workaround using compressed numpy files. So, all good now.

@arokem I have increased the coverage of streamlinear to 97% in Travis and 100% in my machine. I think I am going to stop here. To cover everything at 100% I need to have full support of the functionality for L-BFGS-B and Powell's method for before Scipy 0.11 which is I think too much pain for no gain.

So, all good on my side. Let it roll!

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Oct 7, 2014

I am closing this PR for now. Will make another one on the same topic so that it fits with the other registration PRs and it is rebased etc.

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