Skip to content
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

dcm2niix - Do consider exit 1 to be an error #3594

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

yarikoptic
Copy link
Member

@yarikoptic yarikoptic commented Jul 25, 2023

Based on information in
rordenlab/dcm2niix#733 (comment)

Exit code 0 is for success, exit code 1 is the undefined error.

Handling of exit 1 as successful was added in 6ecd4a9 AKA 1.0.2~2^2~13

Closes #3592

@yarikoptic yarikoptic requested a review from effigies July 25, 2023 13:04
Based on information in
rordenlab/dcm2niix#733 (comment)

> Exit code 0 is for success, exit code 1 is the undefined error.

Handling of exit 1 as successful was added in 6ecd4a9
AKA 1.0.2~2^2~13
@codecov
Copy link

codecov bot commented Jul 25, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (ca27e51) 63.17% compared to head (be5b908) 63.17%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3594   +/-   ##
=======================================
  Coverage   63.17%   63.17%           
=======================================
  Files         308      308           
  Lines       40813    40813           
  Branches     5654     5654           
=======================================
  Hits        25784    25784           
  Misses      14016    14016           
  Partials     1013     1013           
Files Changed Coverage Δ
nipype/interfaces/dcm2nii.py 49.09% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@effigies
Copy link
Member

Should this be made configurable? If I recall, handling a non-error code 1 for HeuDiConv was the motivation, so it may be breaking some workflows to change this. If it's configurable here, HeuDiConv could add a flag.

@effigies effigies requested a review from mgxd July 31, 2023 23:10
Copy link
Member

@mgxd mgxd left a comment

Choose a reason for hiding this comment

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

Because of the amount of non-compliant DICOMs in the wild, I think this added strictness will do more harm than good.

Echoing Chris - perhaps the best thing to do is to add a flag (--strict?) that overrides the correct_return_codes to just (0,). Then users could set/avoid this, depending on their DICOMs' compliance.

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

Successfully merging this pull request may close these issues.

dcm2niix node does not error out whenever dcm2niix exits with 1
3 participants