Fix/dcm2niix #2498

mgxd · 2018-03-16T18:01:49Z

Fixes? #2474 .

Changes proposed in this pull request

Add support for new parameters
Some refactoring of dcm2niix interface
Regression test with datalad
~~Add dcm2niix/datalad to travis testing matrix~~

codecov-io · 2018-03-16T18:43:17Z

Codecov Report

Merging #2498 into master will decrease coverage by 18.49%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2498      +/-   ##
==========================================
- Coverage   66.86%   48.36%   -18.5%     
==========================================
  Files         327      119     -208     
  Lines       42474    24162   -18312     
  Branches     5271        0    -5271     
==========================================
- Hits        28400    11686   -16714     
+ Misses      13373    12476     -897     
+ Partials      701        0     -701

Flag	Coverage Δ
#smoketests	`48.36% <ø> (-2.48%)`	⬇️
#unittests	`?`

Impacted Files	Coverage Δ
nipype/workflows/smri/freesurfer/ba_maps.py	`11.11% <0%> (-79.17%)`	⬇️
nipype/workflows/rsfmri/fsl/resting.py	`14.51% <0%> (-70.97%)`	⬇️
nipype/workflows/smri/freesurfer/autorecon2.py	`2.68% <0%> (-68.59%)`	⬇️
nipype/workflows/smri/freesurfer/autorecon3.py	`2.43% <0%> (-66.67%)`	⬇️
nipype/pipeline/plugins/pbsgraph.py	`29.41% <0%> (-64.71%)`	⬇️
nipype/pipeline/plugins/tools.py	`24.28% <0%> (-62.86%)`	⬇️
nipype/workflows/smri/freesurfer/bem.py	`38.46% <0%> (-61.54%)`	⬇️
nipype/workflows/fmri/spm/preprocess.py	`11.22% <0%> (-61.23%)`	⬇️
nipype/pipeline/plugins/multiproc.py	`20.83% <0%> (-58.93%)`	⬇️
nipype/interfaces/utility/csv.py	`42.85% <0%> (-57.15%)`	⬇️
... and 270 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95e81c9...41c45b9. Read the comment docs.

mgxd · 2018-03-16T23:30:25Z

looks like same issue as ReproNim/neurodocker#148

I'll save including dcm2niix and datalad to our testing suite for a later PR

satra · 2018-03-17T20:45:53Z

nipype/interfaces/dcm2nii.py

+    >>> converter.inputs.compression = 5
+    >>> converter.inputs.output_dir = 'ds005'
+    >>> converter.cmdline
+    'dcm2niix -b y -z y -5 -x n -t n -m n -o ds005 -s n -v n dicomdir'


perhaps add:

>>> converter.run() # doctest: +SKIP

after this

effigies · 2018-03-19T17:31:25Z

Is this targeting 1.0.2?

effigies · 2018-03-22T16:47:21Z

@mgxd Is this ready for review?

mgxd · 2018-03-22T17:50:49Z

@effigies yes

effigies · 2018-03-22T17:58:47Z

nipype/interfaces/dcm2nii.py


    >>> flags = '-'.join([val.strip() + ' ' for val in sorted(' '.join(converter.cmdline.split()[1:-1]).split('-'))])
    >>> flags
-    ' -b y -f %t%p -m n -o . -s y -t n -v n -x n -z i '
+    ' -5 -b y -m n -o ds005 -s n -t n -v n -x n -z y '


What is the point of this flags line, if we're already showing cmdline?

I'm fine to remove this as well, just kept around since it previously existed

Looks like @satra specifically added this in #1529. Guessing the flag ordering was non-deterministic, since he also added #doctest: +SKIP to the cmdline line. If the cmdline check is working consistently, I'd say go ahead and drop.

effigies · 2018-03-22T18:01:02Z

nipype/interfaces/dcm2nii.py

@@ -251,6 +263,8 @@ class Dcm2niixInputSpec(CommandLineInputSpec):
        argstr="%s",
        position=-1,
        copyfile=False,
+        deprecated='1.0.2',
+        new_name='source_dir',


I'm not sure why this is being deprecated. Is it no longer available in dcm2niix, or is it simply disrecommended? Should we use max_ver instead?

usage: dcm2niix [options] <in_folder>

I would prefer to remove this entirely, as it's misleading to dcm2niix's actual usage (I've checked as far back as version 28Aug2016)

Okay. Looks like you added this interface in the first place, so nobody else really to consult with. I'm fine with this.

effigies · 2018-03-22T18:02:48Z

nipype/interfaces/dcm2nii.py

    output_dir = Directory(
-        exists=True, argstr='-o %s', genfile=True, desc="Output directory")
+        os.getcwd(),


This will be evaluated at import time, and thus output to the working directory from which the python interpreter was started (usually). If you want the base directory of the node (or CWD at interface runtime), I think you'll want ".".

effigies · 2018-03-22T18:04:50Z

nipype/interfaces/dcm2nii.py

    has_private = traits.Bool(
        False,
        argstr='-t',
        usedefault=True,
        desc="Flag if text notes includes private patient details")
+    compression = traits.Enum(
+        1, 2, 3, 4, 5, 6, 7, 8, 9,
+        argstr='-%s',


effigies · 2018-03-22T18:06:55Z

nipype/interfaces/dcm2nii.py

+        desc="Ignore derived, localizer and 2D images")
+    series_numbers = InputMultiPath(
+        traits.Str(),
+        argstr='-n %s',


Just to check: If this is series_numbers = ["a", "b", "c"], is the desired output -n a b c or -n a -n b -n c? Right now it's coded for the former.

after checking, it should be the latter, making a fix now

You can just do argstr='-n %s...', rather than add a special case to _format_arg.

effigies · 2018-03-22T20:25:17Z

nipype/interfaces/dcm2nii.py

-                    else:
-                        output_dir = self._gen_filename('output_dir')
-                    out_file = os.path.abspath(os.path.join(output_dir, fname))
+                    out_file = os.path.abspath(fname)


Do you not want to use output_dir at all? It no longer seems to have any effect in this interface.

if omitted, dcm2niix defaults to save within input folder, so I'd prefer to have it save within the node directory/cwd. The output_dir is parsed from dcm2niix's stdout as fname

Ah, sorry, I misunderstood what was going on here.

effigies · 2018-03-23T16:29:06Z

nipype/interfaces/dcm2nii.py

-    >>> flags = '-'.join([val.strip() + ' ' for val in sorted(' '.join(converter.cmdline.split()[1:-1]).split('-'))])
-    >>> flags
-    ' -5 -b y -m n -o ds005 -s n -t n -v n -x n -z y '
-    >>> converter.run() # doctest: +SKIP


I think we did still want the converter.run() line, though.

effigies

LGTM. I assume the tests pass locally?

effigies · 2018-03-23T19:27:56Z

Passing, but Circle's not updating right now.

mgxd added 3 commits March 16, 2018 13:50

ref: dcm2niix interface

6ecd4a9

tst: add datalad/dcm2niix to testing

26d82fe

fix: dcm2niix doctest

b60b4d8

mgxd added 2 commits March 16, 2018 15:39

tst: move dcm2niix test to circle

6d7bb8d

fix: forgotten changes

43c43a7

mgxd added 4 commits March 16, 2018 19:42

tst: remove dcm2niix and datalad testing until neurodocker fixes

4c2e885

tst: add check for dcm2niix executable

ec72e99

enh: add version checking

140b715

fix: version parsing

e904bbb

satra reviewed Mar 17, 2018

View reviewed changes

mgxd added 3 commits March 19, 2018 14:44

doc: add test [skip-ci]

7ba24f1

Merge branch 'fix/dcm2niix' of github.com:mgxd/nipype into fix/dcm2niix

333c5a3

Merge branch 'master' of git://github.com/nipy/nipype into fix/dcm2niix

715de9c

effigies mentioned this pull request Mar 22, 2018

The trait 'converted_files' of a Dcm2niixOutputSpec .. is an existing file name, but the path ....nii.gz' does not exist #2474

Closed

effigies reviewed Mar 22, 2018

View reviewed changes

mgxd added 2 commits March 23, 2018 12:21

fix: address review comments

dd0503f

Merge branch 'master' of git://github.com/nipy/nipype into fix/dcm2niix

c92289b

effigies reviewed Mar 23, 2018

View reviewed changes

fix: argstr and doctest

41c45b9

effigies approved these changes Mar 23, 2018

View reviewed changes

effigies merged commit 803982c into nipy:master Mar 23, 2018

effigies added this to the 1.0.2 milestone Mar 23, 2018

effigies mentioned this pull request Mar 29, 2018

Minor cleanups #2519

Merged

mgxd deleted the fix/dcm2niix branch May 17, 2018 20:44

mgxd mentioned this pull request Sep 13, 2019

RF: dcm2niix interface #2936

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/dcm2niix #2498

Fix/dcm2niix #2498

mgxd commented Mar 16, 2018 •

edited

Loading

codecov-io commented Mar 16, 2018 •

edited

Loading

mgxd commented Mar 16, 2018

satra Mar 17, 2018

effigies commented Mar 19, 2018

effigies commented Mar 22, 2018

mgxd commented Mar 22, 2018

effigies Mar 22, 2018

mgxd Mar 23, 2018

effigies Mar 23, 2018

effigies Mar 22, 2018

mgxd Mar 23, 2018

effigies Mar 23, 2018

effigies Mar 22, 2018

effigies Mar 22, 2018

mgxd Mar 23, 2018

effigies Mar 22, 2018

mgxd Mar 23, 2018

effigies Mar 23, 2018

mgxd Mar 23, 2018

effigies Mar 22, 2018

mgxd Mar 23, 2018

effigies Mar 23, 2018

effigies Mar 23, 2018

mgxd Mar 23, 2018

effigies left a comment

effigies commented Mar 23, 2018

Fix/dcm2niix #2498

Fix/dcm2niix #2498

Conversation

mgxd commented Mar 16, 2018 • edited Loading

codecov-io commented Mar 16, 2018 • edited Loading

Codecov Report

mgxd commented Mar 16, 2018

Choose a reason for hiding this comment

effigies commented Mar 19, 2018

effigies commented Mar 22, 2018

mgxd commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies left a comment

Choose a reason for hiding this comment

effigies commented Mar 23, 2018

mgxd commented Mar 16, 2018 •

edited

Loading

codecov-io commented Mar 16, 2018 •

edited

Loading