-
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
[Rebase] Affine Registration Workflow with Supporting Quality Metrics #1735
Conversation
…rkflow. 1) Committing to this branch all the code from the affine_registration branch (PR 1581) 2) New PR should be opened for this branch.
…ile for test cases.
… dipy_align_affine.
…of mass' transformation rather than 'translation' when the progressive flag is off.
…ix the PEP8 issue.
…value of distance metric) to validate the tests by travis.
…ilarity metric in the optimize() function of the imaffine.py file. 2) Changed the parameter documentation in the run() method of the alignpy file for improved view on the command line.
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 have a few comments/questions
|
||
Returns | ||
------- | ||
affine_map : instance of AffineMap | ||
the affine resulting affine transformation | ||
xopt : optimal parameters | ||
the optimal parameters (translation, rotation shear etc.) |
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.
How many parameters are there here? In exactly what order are they returned?
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.
The number of parameters depends on the transformation.
eg. 2D translation -> 2 parameters, 2D Rotation -> 1 parameter, etc........
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.
this flat vector of parameter build the affine_map so maybe we could just add a method in the AffineMap class to get this parameter instead of returning them, but it deserves its own PR
The object containing the affine matrix. | ||
|
||
""" | ||
np.savetxt(fname, affine) |
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.
Why do we need a wrapper for np.savetxt
? Couldn't we just use that directly?
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.
Even if I have the same opinion, it is always good to have an explicit function name for the end-user. Currently, they have no "explicit official" way to save an affine matrix. I do not have a strict opinion about that, I can remove it. What do you think?
Codecov Report
@@ Coverage Diff @@
## master #1735 +/- ##
==========================================
- Coverage 84.27% 84.25% -0.02%
==========================================
Files 115 115
Lines 13606 13680 +74
Branches 2144 2156 +12
==========================================
+ Hits 11466 11526 +60
- Misses 1643 1650 +7
- Partials 497 504 +7
|
This looks good to me. Anyone else want to take a look? If I don't hear anything, I'll go ahead and merge this on Friday. |
The goal of this PR is to clean and rebase #1604. Look at #1604 for more information