Expose --float option in ants registration #1024
Conversation
Since the parameter can obviously not be called *float* it is called *float_computations*.
Expose --float option in ants registration
|
@gerddie - thanks for this. one of the reasons we ask people to submit a PR even for some silly things is simply to get used to it. but more importantly in this case, when traits are changed:
so even with simple addition, it gets us some mileage both educationally and from a code perspective. |
|
Well, regarding 2 and 3, you should probably add a basic "how to contribute" guide to the wiki, that you can point to when you ask people that never have sent you a PR before to do one. For me your request to do a PR resulted in the following: Because I use the stable version of nipype that comes with the Debian package, and in order to not mess it up, I created a local copy of the interface file where the functionality of this on-liner was added and tested right after I field the bug report. Then, when you asked for the PR I just added the according line with the GitHub on-line editor. Given your comment I guess this is not what you really wanted. Currently, I don't really have time to prepare a correct PR, so I will get back to you on that later. |
|
@gerddie - i see @oesteban already sent the pointer to contributing. we have been sloppy many times over (that's why our coverage is at 70%). what we are trying to do is to become more concerned about the code. and changing code has quite a few implications for our users and those who use our api. again i'm just using this as a point of illustration that even something seemingly simple has consequences beyond the person making the change. |
|
@satra I completely understand your concerns, but with this in mind you shouldn't tell people who never have contributed to the core project before that this PR "should be an easy one". If I would have read the contribution guide first, I would have answered in #1024 that right now I don't have the time to prepare a PR that follows these guidelines. Nevertheless, I think NiPyPe is an excellent piece of software and when I find the time I will happily contribute to it following the proper procedures. |
|
@gerddie, I can imagine that @satra meant that the PR "should be an easy one" in the sense that adding a new trait in an interface does not require a strict understanding of nipype. However, it is definitely not an easy PR if your framework does not meet the very specific conditions explained on https://github.com/nipy/nipype/blob/master/CONTRIBUTING.md. Sorry about that. I've sent you a PR so that you are able to update this one without problems (gerddie#2) . I've also opened a new wiki page with a pointer to the contributing file (https://github.com/nipy/nipype/wiki/Contributing). I agree on that it was lacking, and it would have saved you some time. I'm pretty sure that the nipype team will appreciate any thought to ease the contribution process. IMO, you are right on that the process might be a bit too demanding for spontaneous contributions. Every contribution is a valuable piece of nipype, thanks a lot for yours! |
Update test after make specs
| @@ -322,7 +322,7 @@ class RegistrationInputSpec(ANTSCommandInputSpec): | |||
| low=0.0, high=1.0, value=0.0, argstr='%s', usedefault=True, desc="The Lower quantile to clip image ranges") | |||
| collapse_linear_transforms_to_fixed_image_header = traits.Bool( | |||
| argstr='%s', default=False, usedefault=True, desc='') | |||
|
|
|||
| float_computations = traits.Int(argstr='--float %d', value=0, desc="Use single floating point for computations") | |||
There was a problem hiding this comment.
@hjmjohnson quick check here: does --float require an argument? in the past i have simply used --float as a flag. should we turn this into a Bool?
There was a problem hiding this comment.
After a quick look at the ANTs command line parser implementation it seems that the parser adds a default '1' if the argument following the command line option string starts with '-' and can not interpreted as a float (i.e. it is supposedly the start of the next command line option string), or if there are no arguments left.
However, one could also add "--float 0" to the command line ...
| @@ -321,6 +321,8 @@ class RegistrationInputSpec(ANTSCommandInputSpec): | |||
| winsorize_lower_quantile = traits.Range( | |||
| low=0.0, high=1.0, value=0.0, argstr='%s', usedefault=True, desc="The Lower quantile to clip image ranges") | |||
|
|
|||
| float_computations = traits.Int(argstr='--float %d', value=0, desc="Use single floating point for computations") | |||
There was a problem hiding this comment.
@gerddie - i think this can simply be:
use_float_computations = traits.Bool(argstr='--float', desc="Use single floating point for computations")
There was a problem hiding this comment.
for example as used here:
|
Well, do as you prefer it,"officially" the option is exposed as having an int parameter. |
|
@hjmjohnson - since you are the ants/itk master, what do you suggest? should we make use_float_computations a bool parameter? it seems that there is no reason to have an additional argument for this. perhaps this can be changed in antsRegistration as well. |
|
@satra For nipype using a boolean may be better. In antsRegistration a poor choice was made in creating the command line option to take an integer of either 0 or 1 (several of ants tools do this) instead of just being a boolean. We can not fix it upstream because it would require 100's of scripts to need to be re-written. I don't have the bandwidth to deal with all those changes. Ants should have required a PR procedure for review before accepting a new parameter with a sub-optimal type :). |
|
This patch is superseded by: #1159 Backwards compatibility, and testing added along with using a boolean operator in nipype to provide a consistent behavior and matching to ANTs command line behavior. |
As requested the pull request. The --float option is added as parameter float_computations to the ANTs registration inputs interface.
On a more personal note I might add that generally I'm happy to contribute and follow the appropriated workflows, but IMHO the use of the pull request for a one-liner is kind of overusing the GitHub workflow. Asking for the PR, and now reviewing and commiting it will probably take more of your time that would adding this code by yourself.
thanks, anyway