Skip to content

Fix: ANTs cleanup #1278

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 7 commits into from
Dec 4, 2015
Merged

Fix: ANTs cleanup #1278

merged 7 commits into from
Dec 4, 2015

Conversation

blakedewey
Copy link
Contributor

Cleanup of ANTs interfaces.
-- Various PEP8 and other style corrections. Cleanup of _format_xarray
-- Finished the TODO items in ApplyTransforms.
-- Added option for use_random_seed for Atropos
-- Added options for verbose in ApplyTransforms/Atropos
-- Added option for float computations in ApplyTransforms (float)

@blakedewey
Removed redundant _format_xarray from ANTSCommand and replaced other …
19599fb 
…join functions with _format_xarray
@blakedewey
PEP8 and other style corrections. Including doctstrings using multi-l…
d2fabd2 
…ine string formatting. Changed class names for antsBrainExtraction and antsCorticalThickness.
@blakedewey
Finished TODO items in resampling (ApplyTransforms)
126d0ca
@blakedewey
Finished TODO items in ApplyTransforms and added option to Atropos
b68f488 
Added interpolation_parameters, float and verbose to ApplyTransforms and added use_random_seed and verbose to Atropos
satra
satra reviewed Nov 18, 2015
View reviewed changes
nipype/interfaces/ants/__init__.py
@@ -7,11 +7,13 @@
from .registration import ANTS, Registration

# Resampling Programs
from .resampling import ApplyTransforms, ApplyTransformsToPoints, WarpImageMultiTransform, WarpTimeSeriesImageMultiTransform
from .resampling import ApplyTransforms, ApplyTransformsToPoints, WarpImageMultiTransform, \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of using a \ we could use a tuple representation

(ApplyTransforms, ApplyTransformsToPoints, WarpImageMultiTransform,
 WarpTimeSeriesImageMultiTransform)

@satra
Copy link
Member

satra commented Nov 18, 2015

LGTM

@blakedewey
Copy link
Contributor Author

Pushed changes to imports. Merge when ready.

chrisgorgo
chrisgorgo reviewed Nov 18, 2015
View reviewed changes
nipype/interfaces/ants/segmentation.py
@@ -486,24 +491,26 @@ class antsCorticalThicknessOutputSpec(TraitedSpec):
BrainVolumes = File(exists=True, desc='Brain volumes as text')


class antsCorticalThickness(ANTSCommand):
class CorticalThickness(ANTSCommand):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this breaking backwards compatibility?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually that's a good point. we can add a deprecation warning to the class:

class antsCorticalThickness(CorticalThickness):
    DeprecationWarning('This class has been replaced by CorticalThickness and will be removed in version 0.13')

@blakedewey
Copy link
Contributor Author

So the removal of "ants" from the class names would break backwards compatibility. What would be the process to go through this? It has been done in the AFNI interfaces before. antsCorticalThickness was added 1 year ago and antsBrainExtraction was just added.

@chrisgorgo
Copy link
Member

@satra is right. It's the same scheme:

nipype/nipype/algorithms/misc.py

Line 1473 in 175a6de

class Overlap(nam.Overlap):

@blakedewey
Copy link
Contributor Author

Updated to include deprecated classes.

@satra
Copy link
Member

satra commented Nov 19, 2015

@blakedewey - i just ran into the issue that the verbose flag for antsRegistration is not available in version 2.1.0. i saw that you added it recently. i think we need some form of versioning for this flag.

@blakedewey
Copy link
Contributor Author

@hjmjohnson Do you know when this flag came up?

@blakedewey
Copy link
Contributor Author

Nvm. I found the commit and it was after 2.1.0 came out. I didn't realize they added a completely new option. I will remove the additions.

@hjmjohnson
Copy link
Contributor

commit c37982901a1b8c2c51d1b4353b289a44547d63e2
Author: Hans Johnson hans-johnson@uiowa.edu
Date: Wed Jan 14 11:18:40 2015

ENH: Add version command line option

In order to identify different versions of
the ANTs program, it is benificial to have
command line arguments use introspection to
report their version and git hash codes.

./bin/antsRegistration -v
All_Command_lines_OK
ANTs Version: 15.0.0rc3.dev1-g39ad7
Compiled: Jan 14 2015 10:58:35

./bin/antsRegistration --version
All_Command_lines_OK
ANTs Version: 15.0.0rc3.dev1-g39ad7
Compiled: Jan 14 2015 10:58:35

This patch set adds the version information
for the antsRegistration program.

@satra
Copy link
Member

satra commented Nov 19, 2015

@hjmjohnson - hoping there is an ants release soon :)

@hjmjohnson
Copy link
Contributor

@satra What constistutes a release? Do you just want a new version number presented?

I could make a new release version easily so that we have something to identify in nipype.

Do you want version set to 2.2.0.???

ANTs Version: 2.1.0.post407-g8a995

Hans

@satra
Copy link
Member

satra commented Nov 19, 2015

essentially - the last release was 2.1.0 - if the API has changed it should be 3.0.0. see http://semver.org/

satra added a commit that referenced this pull request Dec 4, 2015
@satra
Merge pull request #1278 from nipy/fix/ants_cleanup
cbdc3dd 
Fix: ANTs cleanup
@satra satra merged commit cbdc3dd into master Dec 4, 2015
@blakedewey blakedewey deleted the fix/ants_cleanup branch December 18, 2015 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@blakedewey @satra @chrisgorgo @hjmjohnson