Skip to content

Eddy-Wrapper does not work properly #769

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

Closed
JensNRAD opened this issue Jan 18, 2014 · 4 comments
Closed

Eddy-Wrapper does not work properly #769

JensNRAD opened this issue Jan 18, 2014 · 4 comments

Comments

@JensNRAD
Copy link
Contributor

Hi,

thanks for the great Framework.

I think there are two bugs in the FSL Eddy wrapper.

[319] in_topup = File(exists=True, desc='Base name for output files from topup', argstr='--topup=%s' )

does not work correctly, because eddy just expects a base name which it uses for two files (basename_fieldcoef.nii.gz and basename_movpar.txt). A quick fix is to set exists=False.

The second bug is in the output check.
[368] outputs['out_parameter'] = '%s..eddy_parameters' % self.inputs.out_base

it should read:
[368] outputs['out_parameter'] = '%s.eddy_parameters' % self.inputs.out_base
(Just one dot between %s and eddy_parameters.

@satra
Copy link
Member

satra commented Jan 23, 2014

thanks for this. could you please send a PR with the fixes?

also a question regarding the first bug. doesn't in_topup refer to two files that are output from topup? if so, i think the interface will need to take those files separately from a workflow perspective and use the basename only.

@oesteban - fyi

@oesteban
Copy link
Contributor

Thanks, I am currently finishing things in a project, so I will look at it very soon :).

@JensNRAD
Copy link
Contributor Author

Yes, "in_topup" refers to the two files that are output from topup.

I am not sure how to modify the interface, so it takes just the basename as an input and then checks for the presence of both files. Can you point me to an example interface, where this has been implemented. Then I can change it and send a PR.

@chrisgorgo
Copy link
Member

I think the most flexible way to do this is to remove in_topup and replace
it with "fieldcoef" and "movpar" inputs. Then overload _run_interface to
copy/rename the input fieldcoef and movpar files to whatever pattern eddy
expects. This way we enable straighforward connection between topup and
eddy as well as enable users to easily intercept and modify fieldcoef and
movpar files. Similar scheme has been used for bedpostx and probtractx.

On Thu, Jan 23, 2014 at 11:40 PM, JensNRAD [email protected] wrote:

Yes, "in_topup" refers to the two files that are output from topup.

I am not sure how to modify the interface, so it takes just the basename
as an input and then checks for the presence of both files. Can you point
me to an example interface, where this has been implemented. Then I can
change it and send a PR.


Reply to this email directly or view it on GitHubhttps://github.com//issues/769#issuecomment-33177550
.

JensNRAD added a commit to JensNRAD/nipype that referenced this issue Feb 4, 2014
JensNRAD added a commit to JensNRAD/nipype that referenced this issue Feb 4, 2014
JensNRAD added a commit to JensNRAD/nipype that referenced this issue Feb 4, 2014
satra added a commit that referenced this issue Feb 5, 2014
Fix for Eddy-Wrapper does not work properly #769
bpinsard pushed a commit to bpinsard/nipype that referenced this issue Mar 5, 2014
* nipype/master:
  Don't call subprocess.Popen twice when output == 'file'.
  Don't call subprocess.Popen twice when output == 'file'.
  fix: updated with copyfile to ensure the inputs get linked to a local directory when running in a workflow
  Fix version 2 for Eddy-Wrapper does not work properly nipy#769
  Fix version 2 for Eddy-Wrapper does not work properly nipy#769
  Fix for Eddy-Wrapper does not work properly nipy#769
  doc: no support for directories
  updated changelog
  0.9.2 release
  fix: use two files
  update changelog
  Tests and doc changes
  Added ApplyTransformsToPoints interface
  fix: make dcm2nii compatible with node for efficient execution, improve docs
  cleanup
  Added VtoMat vista interface and updated CHANGES and autogenerated tests accordingly
  added test for Vnifti2Image interface
  updated CHANGES log
  Created interface for vimage2nifti command from lipvia vista package
  fixed mcflirt mean image output
bpinsard pushed a commit to bpinsard/nipype that referenced this issue Mar 5, 2014
* master:
  fix: updated with copyfile to ensure the inputs get linked to a local directory when running in a workflow
  Fix version 2 for Eddy-Wrapper does not work properly nipy#769
  Fix version 2 for Eddy-Wrapper does not work properly nipy#769
  Fix for Eddy-Wrapper does not work properly nipy#769
  doc: no support for directories
  updated changelog
  0.9.2 release
  fix: use two files
  update changelog
  Tests and doc changes
  Added ApplyTransformsToPoints interface
  fix: make dcm2nii compatible with node for efficient execution, improve docs
  cleanup
  Added VtoMat vista interface and updated CHANGES and autogenerated tests accordingly
  added test for Vnifti2Image interface
  updated CHANGES log
  Created interface for vimage2nifti command from lipvia vista package
  fixed mcflirt mean image output
oesteban added a commit to oesteban/nipype that referenced this issue Sep 1, 2014
* TOPUP: regarding nipy#794, nipy#796 I've cleaned up the docstring and implemented
  the three arguments that were still missing. The problem is that TOPUP
  admits one or more values for them, so the implementation of all the
  traits should be updated.

* Eddy: the implementation of @JensNRAD seems to work out, I reviewed
  the docstring. Close nipy#769.

* Added deprecation notices and minor fixes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants