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

Symmetric diffeomorphic non-linear registration #346

Merged
merged 174 commits into from Jul 31, 2014

Conversation

Projects
None yet
5 participants
@omarocegueda
Contributor

omarocegueda commented Apr 13, 2014

This is a Python/Cython implementation of Avant's mid-point algorithm for symmetric diffeomorphic non-linear registration. We basically reverse-engineered the reference implementation available in ANTS.

This module includes the basic features for volumetric registration:

  • The SyN algorithm using free-form deformation fields (a displacement vector per voxel) in 2D and 3D
  • Two metrics: Sum of Squared Differences (SSD) and Cross Correlation (CC)

To validate our implementation, we reproduced the results reported in Rohlfing's [1] paper to make sure we can obtain equivalent results as ANTS with the same number of iterations. This validation consists on registering 18 manually annotated brains (the IBSR database) with each other (306 registrations in total) and comparing the overlap of 31 anatomical areas after registration using the Jaccard index.

Since this is a long pull request we would also like to have a hang out to explain the main structure, algorithms and tests so that it's easier for you to go through the code.

[1] Rohlfing, T. (2012). Image similarity and tissue overlaps as surrogates for image registration accuracy: widely used but unreliable. IEEE Transactions on Medical Imaging, 31(2), 153–63. doi:10.1109/TMI.2011.2163944

omarocegueda and others added some commits May 1, 2013

Renamed ornlm_module.pyx to ornlm.pyx (there was a naming conflict wi…
…th the c++ source code, but it was already eliminated)
Started changing coding style in files and moving files in their corr…
…ect position according to the dipy guidelines
@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 31, 2014

Hi @omarocegueda the coverage in imwarp is now 97%. Is there anything urgent that you would like to add in this PR? If not I would like to merge it and then you can create smaller PRs for everything else. Sounds good?

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Jul 31, 2014

Sure!, no problem. Travis only shows the location of the statements that
were not hit by the tests and I already covered all of them (100% statement
coverage), but I have no idea how to know which code branches were not
covered, that's why right know I don't know how to quickly cover the extra
3%. However I have a strong confidence that if something brakes it will be
detected by the regression tests, so at least the important functionality
(the ability to reproduce Rohlfing's results) will not "silently break".

Thanks! =)

On Thu, Jul 31, 2014 at 5:40 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Hi @omarocegueda https://github.com/omarocegueda the coverage in imwarp
is now 97%. Is there anything urgent that you would like to add in this PR?
If not I would like to merge it and then you can create smaller PRs for
everything else. Sounds good?


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

"Cada quien es dueño de lo que calla y esclavo de lo que dice"
-Proverbio chino.
"We all are owners of what we keep silent and slaves of what we say"
-Chinese proverb.

http://www.cimat.mx/~omar

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 31, 2014

Okay brother! You did some incredible work here. Go Go TEAM!!!

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 31, 2014

Congratulations!

Garyfallidis added a commit that referenced this pull request Jul 31, 2014

Merge pull request #346 from omarocegueda/syn_registration
Symmetric diffeomorphic non-linear registration

@Garyfallidis Garyfallidis merged commit e63c178 into nipy:master Jul 31, 2014

1 check passed

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

This comment has been minimized.

Member

arokem commented Jul 31, 2014

Congratulations to everyone and especially to Omar - awesome work!

On Thu, Jul 31, 2014 at 9:29 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Merged #346 #346.


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

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Jul 31, 2014

Wow! so exciting! =) I know there's still a lot to be done, but having this
code in is encouraging! thank you very much Elef, Ariel, Matthew for your
reviews (as Elef would say, "it was so educational..." =P ), I will
continue with the fixes.

On Thu, Jul 31, 2014 at 9:31 AM, Ariel Rokem notifications@github.com
wrote:

Congratulations to everyone and especially to Omar - awesome work!

On Thu, Jul 31, 2014 at 9:29 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Merged #346 #346.


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


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

"Cada quien es dueño de lo que calla y esclavo de lo que dice"
-Proverbio chino.
"We all are owners of what we keep silent and slaves of what we say"
-Chinese proverb.

http://www.cimat.mx/~omar

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jul 31, 2014

Congratulations for sure - but for next time let's discuss before merging before we've all agreed. If not then it reduces trust in the review process. I mean - if I the reviewer think you the maintainer are going to ignore my comments I likely won't work as hard to review the code.

Omar - would it be reasonable to estimate that we've so far got around 25% coverage on (I believe) many thousands of lines of Cython code? What are we going to do about that?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 31, 2014

Hi @matthew-brett , I think if you read the previous discussion on this PR. I kind of said that it was too much to ask from a new member to do everything in the same PR which has already rebased many many times. Nobody, disagreed with that. I think also Ariel agreed to have subsequent PRs with the rest of the updates.

Of course at the moment we don't have a way to see the cython lines tested directly. But why do you say 25%? How did you calculate this?

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jul 31, 2014

I don't think we agreed it was a good idea to merge with expanding the coverage of the Cython files.

I can see why you might want to do that, all I'm saying is that you should check, with something like 'Hey @matthewbrett - are you OK to merge as is and expand test coverage later'. Actually, I would have said that we need a better estimate of Cython test coverage first, but hey.

I'm guessing at the 25% Cython test coverage, from doing searches for function names when I was reviewing the code. But doesn't it worry you that you have no idea what the Cython test coverage is? And that there is so much Cython code?

You're the guy, but if it was me, I would worry that dipy is now carrying such a large burden of untested code for which we can't get automated coverage stats, and that is hard to debug compared to the Python code.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 31, 2014

There are many things that worry me. One of them is coverage of Cython, the other one is to have as many people as possible using it I mean before releasing it. But your point went through. I think also Omar made it clear that he is going to work on more fixes.

Now, about the cython coverage I found this post https://groups.google.com/forum/#!msg/cython-users/TqMJiFHkevU/HcL-niR_SsgJ

It seems we need to compile with CYTHON_TRACE enabled.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jul 31, 2014

If dipy starts accumulating untested code, it will slow down development and eventually become unmaintainable like many other projects we know.

Here's are three bets - that when we do discover how to do Cython test coverage, the coverage will be well below 50%. And that that will still be true at the next release of dipy. And that Omar will be the only person touching this code.

If you wanted this in soon, I think we would have needed an all-hands-to-the-pumps after say a google hangout, to get a coverage estimate, and to increase the coverage.

I humbly recommend discussing this on the list, seeing whether we can gather some help, then revert the merge, planning to merge back in say two weeks after a mini-sprint. This also depends on Omar's availability to help out.

@arokem

This comment has been minimized.

Member

arokem commented Jul 31, 2014

Yes - I believe that @stefanv mentioned CYTHON_TRACE a few weeks ago.

As for @matthew-brett's suggestion to revert this merge, I am a -0.5 on that, under the assumption that @omarocegueda does intend to go in there and work on coverage immediately. If only to keep our forward momentum.

What is your exact plan on this?

I agree with @matthew-brett that it would be a big hit to all of us, if we leave this untested for any significant amount of time. Ultimately, we might have to pull all of this stuff out. So, I see this issue (figuring out how much coverage this has, and bringing this as close to 100% as humanly possible) as blocking the upcoming release of dipy.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 31, 2014

Hi, because the important part of the vector_fields.pyx filed is already being called from imwarp and it is well tested there is no reason to revert the merge. I think we are in a safe "enough" land. It could be that just some of the functions in vector_fields.pyx are not currently being used. This after the coverage test could be easily resolved with a simple cleanup.

The truth is that we need to cleanup some other modules too. For example, there is old code from @cnguyen on bootstrapping that it was never used and there is from me the eit.py file that it was never used.

But don't get me wrong. This merge was only to help us move forward with things as I trust that the rest of us and Omar will do the necessary work to deliver this at 100%. This is not for rushing a dipy release.

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Jul 31, 2014

Hello everyone!,
I'm really sorry that this PR is causing so much trouble, I don't know how
much effort it is required to "un-merge" the PR, but IMHO the most
important thing right now is not that this is included in the next release
or not but that everyone agrees on whatever decision is made (the most
important part of the project is people, not the product). I wrongly
assumed that everyone was OK with merging with 97% coverage, after all,
many other dipy modules have way less coverage than that. Anyway that was
my wrong assumption and I didn't meant to "ignore" anyone's opinion. I will
finish my internship next week and then I will be working on registration
full time (starting Monday August 11), I think the priority will be to
measure the cython coverage (I need to see how to do what Stefan suggested)
and improve the tests, leaving the linear registration part as second
priority. I am planning to spend say the first week on this to see how far
I can get and then I should have a better idea of how much effort I will
need to reach an acceptable coverage. Regarding hitting 100%, does anyone
know how to find the branches that are not covered by the tests? (the tests
cover 100% of the statements, but that's not considered 100% coverage by
Travis)
Thanks!

On Thu, Jul 31, 2014 at 2:15 PM, Ariel Rokem notifications@github.com
wrote:

Yes - I believe that @stefanv https://github.com/stefanv mentioned
CYTHON_TRACE a few weeks ago.

As for @matthew-brett https://github.com/matthew-brett's suggestion to
revert this merge, I am a -0.5 on that, under the assumption that
@omarocegueda https://github.com/omarocegueda does intend to go in
there and work on coverage immediately. If only to keep our forward
momentum.

What is your exact plan on this?

I agree with @matthew-brett https://github.com/matthew-brett that it
would be a big hit to all of us, if we leave this untested for any
significant amount of time. Ultimately, we might have to pull all of this
stuff out. So, I see this issue (figuring out how much coverage this has,
and bringing this as close to 100% as humanly possible) as blocking the
upcoming release of dipy.


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

"Cada quien es dueño de lo que calla y esclavo de lo que dice"
-Proverbio chino.
"We all are owners of what we keep silent and slaves of what we say"
-Chinese proverb.

http://www.cimat.mx/~omar

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Jul 31, 2014

I don't think the fact that the code is called from other code that is tested, it a good one.

Omar - thanks for the reply. I don't think the controversy is any way down to you, you didn't suggest merging before checking coverage or expanding the tests. The Python code coverage is more or less a distraction at this point, it was already much better tested than the Cython code, and tests are even more important for the Cython code.

Technically pulling the merge is completely trivial at this point. In due course it will become harder when other people build on top.

Why don't we compromise, and pull the merge until one of us has had a chance to work out Cython coverage. We can set a cut-off for a week to get that done otherwise we re-merge. If the coverage is greater than 70 percent then I withdraw my objection and we can finish up testing in trunk. Otherwise we can revisit. How about that?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Jul 31, 2014

I am playing right now with the cython coverage. Let me see if I can give some feedback by tomorrow.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Aug 1, 2014

Hi all,

I put some serious effort yesterday to make coverage work with the cython modules but with no success yet. If you are interested you may want to have a look at the revisited post by Pankaj Pandey here http://blog.pankajp.com/2010/10/cython-functions-coverage-revsited.html

I have also asked in the Cython list for some more pointers. I hope that they will get back to me.

In the meantime I made a thorough check on what is actually being tested from vector_fields.pyx. So, here are the results. Here I am only reporting the defs (not cdefs):

defs called from tests:

In test_vector_fields.py

    vfu.create_random_displacement_2d
    vfu.warp_image
    vfu.warp_image_nn
    vfu.warp_volume
    vfu.warp_volume_nn
    vfu.consolidate_2d
    vfu.create_random_displacement_3d
    vfu.consolidate_3d
    vfu.compose_vector_fields_3d
    vfu.create_harmonic_fields_2d
    vfu.invert_vector_field_fixed_point_2d
    vfu.create_harmonic_fields_3d
    vfu.invert_vector_field_fixed_point_3d

In test_imwarp.py

    vfu.create_random_displacement_2d
    vfu.create_harmonic_fields_2d

defs not called directly from tests but called:
from imwarp at SymmetricDiffeomorphicRegistration creation

    prepend_affine_to_displacement_field_2d 
    prepend_affine_to_displacement_field_3d 

    append_affine_to_displacement_field_2d 
    append_affine_to_displacement_field_3d

from imwarp at SymmetricDiffeomorphicRegistration _optimize

    expand_displacement_field_3d
    expand_displacement_field_2d

defs not currently tested at all (dangerous!)

reorient_vector_field_2d
reorient_vector_field_3d

upsample_displacement_field
upsample_displacement_field_3d

accumulate_upsample_displacement_field3D
downsample_scalar_field3D   
downsample_displacement_field3D

downsample_scalar_field2D
downsample_displacement_field2D

get_displacement_range
warp_volume_affine
warp_volume_affine_nn
warp_image_affine
warp_image_affine_nn
create_linear_displacement_field_2d

@omarocegueda this last part seems like it is not tested at all. Are these functions useful or not anymore and they can be removed? Or some of them are to support Linear registration which will come in upcoming PRs? So, which of these are currently useful for the code as it is?

@omarocegueda

This comment has been minimized.

Contributor

omarocegueda commented Aug 1, 2014

Thanks Elef!, this is very useful! seems like some of them are functions
that I was using before the big refactor to iterate in the physical space
instead of image coordinates (all the upsample/downsample ones), and some
of them (the warp_affine ones) can easily be tested by adapting the tests
for the other warp functions. Let me check carefully and I'll get back to
you.
Thanks a lot man! thats awesome!

On Fri, Aug 1, 2014 at 7:19 AM, Eleftherios Garyfallidis <
notifications@github.com> wrote:

Hi all,

I put some serious effort yesterday to make coverage work with the cython
modules but with no success yet. If you are interested you may want to have
a look at the revisited post by Pankaj Pandey here
http://blog.pankajp.com/2010/10/cython-functions-coverage-revsited.html

I have also asked in the Cython list for some more pointers. I hope that
they will get back to me.

In the meantime I made a thorough check on what is actually being tested
from vector_fields.pyx. So, here are the results. Here I am only reporting
the defs (not cdefs):

defs called from tests:

In test_vector_fields.py

vfu.create_random_displacement_2d
vfu.warp_image
vfu.warp_image_nn
vfu.warp_volume
vfu.warp_volume_nn
vfu.consolidate_2d
vfu.create_random_displacement_3d
vfu.consolidate_3d
vfu.compose_vector_fields_3d
vfu.create_harmonic_fields_2d
vfu.invert_vector_field_fixed_point_2d
vfu.create_harmonic_fields_3d
vfu.invert_vector_field_fixed_point_3d

In test_imwarp.py

vfu.create_random_displacement_2d
vfu.create_harmonic_fields_2d

defs not called directly from tests but called:
from imwarp at SymmetricDiffeomorphicRegistration creation

prepend_affine_to_displacement_field_2d
prepend_affine_to_displacement_field_3d

append_affine_to_displacement_field_2d
append_affine_to_displacement_field_3d

from imwarp at SymmetricDiffeomorphicRegistration _optimize

expand_displacement_field_3d
expand_displacement_field_2d

defs not currently tested at all (dangerous!)

reorient_vector_field_2d
reorient_vector_field_3d

upsample_displacement_field
upsample_displacement_field_3d

accumulate_upsample_displacement_field3D
downsample_scalar_field3D
downsample_displacement_field3D

downsample_scalar_field2D
downsample_displacement_field2D

get_displacement_range
warp_volume_affine
warp_volume_affine_nn
warp_image_affine
warp_image_affine_nn
create_linear_displacement_field_2d

@omarocegueda https://github.com/omarocegueda this last part seems like
it is not tested at all. Are these functions useful or not anymore and they
can be removed? Or some of them are to support Linear registration which
will come in upcoming PRs? So, which of these are currently useful for the
code as it is?


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

"Cada quien es dueño de lo que calla y esclavo de lo que dice"
-Proverbio chino.
"We all are owners of what we keep silent and slaves of what we say"
-Chinese proverb.

http://www.cimat.mx/~omar

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Aug 1, 2014

Okay, if @matthew-brett agrees please @omarocegueda make a new PR removing all that is not necessary for the current version of your code and test the rest. For example do you need the warp_volume_affine functions anymore?

@matthew-brett I am going to wait a bit for a response from the Cython list and try a few ideas. If nothing works I will ask for a hangout with you to work together on the cython coverage next week. Sounds good?

If you still want to revert the PR go ahead. In that case @omarocegueda will have to update his old PR.

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Aug 4, 2014

I do want to revert the PR before we build on top of it, and until we know what the Cython coverage is. I'll join in on the Cython mailing list, and let's organize a hangout soon. Maybe we could also discuss general policy on merges then, to reduce future surprises?

If there are no objections, I'll revert the PR later today.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Aug 4, 2014

Go ahead with the revert. About the policies: I have already started a google hangout document which we can extend and put online. And I can find some time to work on Cython coverage together on Thursday afternoon (after 4pm ETS). From what the Cython creators suggested coverage.py does not work with Cython at the moment. Which might be bad news for us. So, maybe we may not be able to find a way to have cython coverage. Could be too complicated for our level of expertise on the topic. In that way we shouldn't ask from @omarocegueda to wait until we have the coverage working because it may take much longer time than expected. But if he cleans up and further tests his cython files then we should merge. Sounds good?

@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Aug 4, 2014

Yes, sure - completely agree we shouldn't wait more than a short time to get Cython coverage working. Maybe give it this week to get something automated going, then give up and try a reasonable guess based on manual code review?

matthew-brett added a commit to matthew-brett/dipy that referenced this pull request Aug 4, 2014

Revert "Merge pull request nipy#346 from omarocegueda/syn_registration"
This reverts commit e63c178, reversing
changes made to c273fe5.

We decided to back off these commits until we had checked the code
coverage of the Cython modules, or timed out doing that.

Discussion here:

nipy#34
@matthew-brett

This comment has been minimized.

Member

matthew-brett commented Aug 4, 2014

Revert PR - #406

Garyfallidis added a commit that referenced this pull request Aug 6, 2014

Merge pull request #406 from matthew-brett/sorry-to-omar-for-now
Revert "Merge pull request #346 from omarocegueda/syn_registration"
@stefanv

This comment has been minimized.

Contributor

stefanv commented Aug 6, 2014

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