[RTM] Code revision - fieldmaps #640
Conversation
oesteban
changed the title
|
@oesteban Can you merge/rebase master? Will make it easier to review. |
fmriprep/interfaces/fmap.py
Outdated
| @@ -68,12 +69,14 @@ def _run_interface(self, runtime): | |||
| iterations=self.inputs.mask_erode | |||
| ).astype(np.uint8) # pylint: disable=no-member | |||
|
|
|||
| self._results['out_file'] = genfname(self.inputs.in_file, suffix='enh') | |||
| self._results['out_file'] = fname_presuffix( | |||
| self.inputs.in_file, suffix='enh', newpath=os.getcwd()) | |||
There was a problem hiding this comment.
genfname put a _ before suffices, so this should be suffix='_enh' to keep behavior unchanged.
And since we're in _run_interface, we can avoid the syscall with runtime.cwd. (I know that this isn't a necessary optimization since we're not bottlenecked by syscalls; it's just a pattern I've been using in my interfaces to use the information provided in runtime when possible.)
(Not going to tag all of the uses of fname_presuffix, but both comments apply to most.)
fmriprep/workflows/fieldmap/fmap.py
Outdated
| @@ -96,9 +91,11 @@ def init_fmap_wf(reportlets_dir, omp_nthreads, fmap_bspline, name='fmap_wf'): | |||
|
|
|||
| else: | |||
| torads = pe.Node(niu.Function(output_names=['out_file', 'cutoff_hz'], | |||
| function=_torads), name='torads') | |||
| function=_torads), name='torads', | |||
| run_without_submitting=True) | |||
There was a problem hiding this comment.
We're not using run_without_submitting on Function nodes at present. See #554.
fmriprep/workflows/fieldmap/fmap.py
Outdated
|
|
||
| if out_file is None: | ||
| out_file = genfname(in_file, suffix='rad') | ||
| out_file = fname_presuffix( | ||
| in_file, suffix='rad', newpath=os.getcwd()) |
There was a problem hiding this comment.
Marking this one: suffix='_rad', but os.getcwd() is necessary here.
There was a problem hiding this comment.
What do you mean by "os.getcwd() is necessary here"?
There was a problem hiding this comment.
That it can't be replaced with runtime.cwd. (Obviously, but just a flag in case you were going through with a search and replace.)
There was a problem hiding this comment.
oh, so you meant to replace os.getcwd() by runtime.cwd when it is possible? I missed that. Please confirm, and I'll update accordingly.
There was a problem hiding this comment.
Yes, that's what I was suggesting. As I said, it's no big deal. I'm happy to merge without that change.
There was a problem hiding this comment.
Sorry again, I missed that out. Updating it now...
|
|
||
|
|
||
| def init_phdiff_wf(reportlets_dir, name='phdiff_wf'): | ||
| def init_phdiff_wf(reportlets_dir, omp_nthreads=1, name='phdiff_wf'): |
There was a problem hiding this comment.
Why a default value for omp_nthreads?
|
|
||
|
|
||
| def init_phdiff_wf(reportlets_dir, name='phdiff_wf'): | ||
| def init_phdiff_wf(reportlets_dir, omp_nthreads, name='phdiff_wf'): |
There was a problem hiding this comment.
The omp_nthreads also needs to be added in the docstring.
#225 - depends on #629