-
Notifications
You must be signed in to change notification settings - Fork 437
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
Affine registration PR1: Transforms. #574
Conversation
return 0 | ||
|
||
|
||
cdef void _dot_prod(double[:,:] A, double[:,:] B, double[:,:] C): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not tested or used?
@omarocegueda I like your structured approach to measuring these results. Can we include some form of the performance (in terms of accuracy) tests in the code base? |
Thank you @stefanv!, |
@@ -0,0 +1,233 @@ | |||
from dipy.align.transforms import * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please import specific names (best) or import the module (import dipy.align.transforms as dit
) and use with dit.
namespace. The *
makes it difficult to spot bad imports, among other things.
Sorry - this might be silly - but how about having a set of strategy objects like:
Instantiate all the ones you need for 2D and 3D, different transformations e.g:
etc, and then do |
… of docstring in test functions. Document return value of Jacobian functions. Remove 'with nogil' clause when calling a nogil function. Associated to --> Associated with. Removed unused '_dot_prod' function.
14225b6
to
937748f
Compare
…d argument handling.
J : array, shape (dim, n) | ||
Jacobian matrix of the transform with parameters theta at point x | ||
""" | ||
n = len(theta) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python call? Maybe use theta.shape[0]
Hi @omarocegueda, I see only 3 files in this PR if this is going to be only about the transforms, you should change the title of the PR. Travis is failing also. Can you correct that? |
…tead of iteritems (deprecated in Python 3)
Hi @Garyfallidis!, yes, this PR only contains the Transforms part. I just changed the title. Travis was failing because "iteritems" was deprecated in Python3, now using "items". |
Do you still need the Work In Progress (WIP) flag on the title? When we see WIP it means that we need to wait until this PR is ready for a review. You can change WIP to MRG if you think this is ready to be merged on your side or just remove the WIP. |
theta : array, shape(2,) | ||
buffer to write the parameters of the 2D translation transform | ||
""" | ||
theta[:2] = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really applied properly in Cython? I mean in a cdef with nogil.
So, this will do theta[0] = 0 and theta[1] = 1? No for loop needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so : http://docs.cython.org/src/userguide/memoryviews.html
# NumPy-style syntax for assigning a single value to all elements.
narr_view[:, :, :] = 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool! I forgot about that. Thank you @matthew-brett.
Omar - do you have a feel for how good the test coverage is for the code? |
( cc * sb + sc * sa * cb ) * pz | ||
J[2, 2] = 0 | ||
|
||
J[0,3:6] = 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
space missing J[0, 3:6]
Hey @omarocegueda please address mine and Matthew's comments. Apart from that. This PR looks very nice and I believe is very close to be merged. Thank you for all the good work and for separating your larger PR in smaller and easier to follow PRs. |
3902149
to
7780dff
Compare
Hi @matthew-brett!, I tried to use your find-cpdef-tests, but I am having some weird errors, I need to see what went wrong. However, we have close to full coverage: in the tests I iterate over all transforms calling all of their methods, so all methods are called at least once. I also check the ValueError exceptions. I think the only missing case is what happens when the user attempts to use the base class Transform instead of one of its derived classes (which is undefined), what do you think would be a good way to handle that case?(an abstract class would be ideal, so it couldn't be instantiated, afaik there is no such a thing as an "abstract struct"). Right now it will raise ValueError when attempting to get its Jacobian or its identity parameters or its matrix representation, and it returns -1 when queried about its dimension and number of parameters. |
Yes, my function was only designed for Cython functions, so won't be much use in this case, with lots of classes. It sounds as if the error modes for the Transform are OK - maybe a few tests for those just to remind ourselves that there are errors and the reason for them. |
8e7e8e4
to
b217bac
Compare
Alright, we should have full coverage now. |
Good to go from my point of view - thanks Omar. |
b217bac
to
5f87d9e
Compare
Thx Omar. Let's move to the second PR. |
Affine registration PR1: Transforms.
Based on some performance comparisons between ANTS2, Flirt and Nipy for affine registration with mutual information, we have observed that ANTS2 consistently performs better in terms of registration accuracy. The following graph depicts the Jaccard index of 31 manually annotated anatomical regions averaged over 306 registrations (1=perfect overlap, 0=no overlap):
http://www.cimat.mx/~omar/imgshare/mi32_flirt_ants2_nipy.png
This motivated us to implement the same registration strategy in Cython, so we can easily use it with our non-linear registration module and at some point modify the algorithms/metrics more easily for DWMRI. This is the current performance of this Cython implementation:
http://www.cimat.mx/~omar/imgshare/ANTS_vs_Dipy.png
I am investigating the cause of the small differences but at some point we should reach the same performance.
In an effort to make the review process (a bit) easier, this part of the code only contains the computation of the affine transforms and their Jacobians.