RF: dcm2niix interface #2936

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

Jump to bottom

Merged

effigies merged 1 commit into nipy:master from mgxd:enh/dcm2niix

Sep 19, 2019

Merged

RF: dcm2niix interface #2936

effigies merged 1 commit into nipy:master from mgxd:enh/dcm2niix

Sep 19, 2019

Conversation

@mgxd

Copy link

Member

@mgxd mgxd commented May 22, 2019

Summary

This PR fixes the convoluted approach used for gathering dcm2niix outputs.

List of changes proposed in this PR (pull-request)

Checks the output prefix for possible suffixes, instead of assuming outputs based off stdout switches.

Acknowledgment

(Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

mgxd

mgxd commented

May 22, 2019

View reviewed changes

nipype/interfaces/dcm2nii.py Outdated

def _parse_files(self, filenames):

outfiles, bvals, bvecs, bids = [], [], [], []

outtypes = (".nii", ".nii.gz", ".bval", ".bvec", ".json")

Copy link

Member Author

@mgxd mgxd May 22, 2019

Choose a reason for hiding this comment

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

@neurolabusc not that we were catching it before, but am I missing any output formats?

Copy link

@neurolabusc neurolabusc May 22, 2019

Choose a reason for hiding this comment

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

The recent release (v1.0.20190410) includes ".nrrd" (-e y -z n) and ". nhdr"/".raw.gz" (-e y -z y). This was added for the Slicer team and so you may or may not want to support NRRD format export. There is also a deprecated text output ".txt" (-t y) which predates BIDS.

@mgxd mgxd changed the title ~~(削除) Enh/dcm2niix (削除ここまで)~~ (追記) RF: dcm2niix interface (追記ここまで)

May 22, 2019

@codecov-io

Copy link

codecov-io commented May 22, 2019 •

edited by codecov bot

Loading

Codecov Report

Merging #2936 into master will decrease coverage by 3.31%.
The diff coverage is 18.18%.

Impacted file tree graph

@@ Coverage Diff @@
## master #2936 +/- ##
=========================================
- Coverage 67.51% 64.2% -3.32% 
=========================================
 Files 344 342 -2 
 Lines 44094 44030 -64 
 Branches 5556 5552 -4 
=========================================
- Hits 29772 28270 -1502 
- Misses 13576 14642 +1066 
- Partials 746 1118 +372

Flag	Coverage Δ
#smoketests	`?`
#unittests	`64.2% <18.18%> (-0.78%)`	⬇️

Impacted Files	Coverage Δ
nipype/info.py	`88.73% <ø> (-5.64%)`	⬇️
nipype/interfaces/dcm2nii.py	`50.23% <18.18%> (+2.1%)`	⬆️
nipype/interfaces/nilearn.py	`40% <0%> (-56.67%)`	⬇️
nipype/utils/spm_docs.py	`25.92% <0%> (-44.45%)`	⬇️
nipype/interfaces/freesurfer/base.py	`50% <0%> (-30.51%)`	⬇️
nipype/utils/logger.py	`59.7% <0%> (-29.86%)`	⬇️
nipype/algorithms/rapidart.py	`35% <0%> (-29.42%)`	⬇️
nipype/interfaces/spm/base.py	`57.94% <0%> (-29.14%)`	⬇️
nipype/utils/provenance.py	`55.73% <0%> (-28.35%)`	⬇️
nipype/interfaces/fsl/model.py	`55.26% <0%> (-25.35%)`	⬇️
... and 41 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 23379eb...1f0b85c. Read the comment docs.

@mgxd mgxd added this to the 1.2.2 milestone

Aug 21, 2019

@effigies

Copy link

Member

effigies commented Sep 5, 2019

@mgxd Any chance of a test? This is pretty tough to evaluate without seeing the code paths exercised.

@oesteban oesteban modified the milestones: 1.2.2, 1.3.0, 1.2.3

Sep 5, 2019

@mgxd

Copy link

Member Author

mgxd commented Sep 6, 2019

we already have a regression test

nipype/nipype/interfaces/tests/test_extra_dcm2nii.py

Line 27 in 704b97d

def test_dcm2niix_dwi(tmpdir):

though I'm not sure it's actually being hit in the current testing. I'll fix this up for the next release

@oesteban

Copy link

Contributor

oesteban commented Sep 13, 2019

CircleCI (both py36 and py27):

[gw1] [ 87%] SKIPPED ../src/nipype/interfaces/tests/test_extra_dcm2nii.py::test_dcm2niix_dwi

Travis (py37):

interfaces/tests/test_extra_dcm2nii.py::test_dcm2niix_dwi 
[gw1] [ 87%] SKIPPED interfaces/tests/test_extra_dcm2nii.py::test_dcm2niix_dwi

@mgxd

Copy link

Member Author

mgxd commented Sep 13, 2019

looks like we never got around to adding it to testing in #2498 - looks like the original problem (#2498 (comment)) was fixed so I'll add that change here.