dcm2niix - Do consider exit 1 to be an error

[yarikoptic]

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

[codecov]

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]

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.

[mgxd]

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.

[yarikoptic]

well, in a defensive coding style, it feels more like desiring then to make it strict by default but to allow some error codes to be treated as "ok", e.g. may be someone would also want kEXIT_SOME_OK_SOME_BAD to be "ok"?

Where in nipype we should add such an option for "ok_exit_codes" since it does not translate into a command line option for dcm2niix? i.e. I guess it should not be defined in Dcm2niixInputSpec , right?

[effigies]

It could be in the input spec. You would do that if either:

1. You want an upstream node to be able to pass this value at workflow run time.
2. You want the cache to be invalidated and the node rerun if the value changes.

If neither of these behaviors is desired, I would just set it as an attribute, settable in __init__().