Nifti image conflicted names - #9
Conversation
emulate FSL when multiple ambiguous filenames exist (.nii/.nii.gz), Configure 'run "FSL=1 make"' and 'cmake -DFSLSTYLE=ON .' Make names consistent nifti_strdup() is too small to include an extension
emulate AFNI pigz support Fixed conversion error when PIGZ is defined. Configure 'run "FSL=1 make"' and 'cmake -DFSLSTYLE=ON .'
Reject complex data like FSL does Configure 'run "FSL=1 make"' and 'cmake -DFSLSTYLE=ON .'
bc14651 to
8047aee
Compare
|
@neurolabusc Could you please review this update to your suggestions posed several years ago to NIFTI-Imaging/nifti_clib? These changes may provide a fix for an issue reported to ITK. Thanks, |
|
@gdevenyi @lassoan Would you mind commenting on this upstream change as a solution to InsightSoftwareConsortium/ITK#2243? We could have ITK build with the nifti libraries with FSLSTYLE_NAME_CONFLICTS:BOOL=ON. |
|
Well, its 3 commits, which is what I suggested in the original PR :) Erroring out on some of the more...interesting choices of how the reader works seems the most sensible thing to do, mirroring how FSL does it already (and re-using the environment variables) is good too. I'm agnostic on pigz, its a band-aid to improve things on a file format lacking good internal compression. |
seanm
left a comment
seanm
left a comment
There was a problem hiding this comment.
IMHO this was merged prematurely...
| #ifdef REJECT_COMPLEX | ||
| if ((nim->datatype == DT_COMPLEX64) || (nim->datatype == DT_COMPLEX128) || (nim->datatype == DT_COMPLEX256)) { | ||
| fprintf(stderr,"Image Exception Unsupported datatype (COMPLEX64): use fslcomplex to manipulate: %s\n", hname); | ||
| exit(13); |
There was a problem hiding this comment.
This was a few years ago, perhaps that was the output of fsl tools at the time. Feel free to change this to 134 as that is what I get with the current version of fslmaths (the FSL error is odd, the tools work fine with float 32, so the error really refers to the enumeration for DT_COMPLEX64 which is 32.
$ fslmaths complex.nii.gz -add 0 sfsf
Image Exception : #8 :: Fslread: DT 32 not supported
libc++abi: terminating due to uncaught exception of type std::runtime_error: Fslread: DT 32 not supported
/Users/chris/fsl/share/fsl/bin/fslmaths: line 2: 19588 Abort trap: 6 /Users/chris/fsl/bin/fslmaths "$@"
$ echo $?
134
There was a problem hiding this comment.
And you actually need that exact error code? As I said in another comment, it's not ideal IMHO to call exit() in a library...
| #ifdef REJECT_COMPLEX | ||
| if ((nim->datatype == DT_COMPLEX64) || (nim->datatype == DT_COMPLEX128) || (nim->datatype == DT_COMPLEX256)) { | ||
| fprintf(stderr,"Image Exception Unsupported datatype (COMPLEX64): use fslcomplex to manipulate: %s\n", hname); | ||
| exit(13); |
|
|
||
| #ifdef PIGZ | ||
| #ifdef HAVE_ZLIB | ||
| int doPigz2(nifti_image *nim, struct nifti_2_header nhdr, const nifti_brick_list * NBL) { |
There was a problem hiding this comment.
struct nifti_2_header is a pretty big struct, might be better to pass by reference, not value, no?
| CFLAGS = -Wall -std=gnu99 -pedantic $(USEZLIB) $(IFLAGS) | ||
|
|
||
|
|
||
| #run "FSLSTYLE=1 make" for JPEGLS build |
There was a problem hiding this comment.
I don't see the JPEGLS connection...
| strcpy(gzname, hdrname); | ||
| strcat(gzname,extzip); |
There was a problem hiding this comment.
strcpy and strcat should not be used in new code
| if (nifti_fileexists(gzname)) { | ||
| fprintf(stderr,"Image Exception : Multiple possible filenames detected for basename (*.nii, *.nii.gz): %s\n", basename); | ||
| free(gzname); | ||
| exit(134); |
There was a problem hiding this comment.
Why 134?
Also, it doesn't seem appropriate for library code to call exit(), nowhere else in this file calls exit(). This should probably return NULL.
There was a problem hiding this comment.
The compile directive FSLSTYLE emulates the beahavior of fsl tools, these terminate with these exit codes for these failures:
$fslmaths a add -0 b
Image Exception : #61 :: Multiple possible filenames detected for basename: a
libc++abi: terminating due to uncaught exception of type std::runtime_error: Multiple possible filenames detected for basename: a
/Users/chris/fsl/share/fsl/bin/fslmaths: line 2: 20227 Abort trap: 6 /Users/chris/fsl/bin/fslmaths "$@"
$ echo $?
134
There was a problem hiding this comment.
So that's a SIGABRT. If we really need to emulate that, better to call abort().
But what would happen if we just return NULL? Could you try this: #11
4 participants
If there are conflicted names, prefer to give a un-ambigous error stating the problem rather than guessing what to do.
Related to: InsightSoftwareConsortium/ITK#2243
Inspired by: NIFTI-Imaging#138 @neurolabusc