Optional file behavior

[neurolabusc]

This is a very simple patch that allows developers to modify the behavior of this library. All are disabled by default, so no change for tools unless they invoke these features.

This provides three compile time options to influence how file handling behavior

1. Related to issue 129. Emulate FSL file handing behavior. Refuse to open T1.nii if file T1.nii.gz exists. Likewise, refuse to open T1.nii.gz if file T1.nii exists. This feature enabled with the directive:

#define FSLSTYLE 

A conflict generates an error:

Multiple possible filenames detected for basename (*.nii, *.nii.gz): T1.nii.gz

2. Use pigz parallel compression when writing .nii.gz files if the environment includes the variable AFNI_COMPRESSOR=PIGZ and the pigz executable is found. This behavior is enabled by the compiler directive:

#define PIGZ

3. Refuse to open files of complex datatype. Many tools are not designed for handling complex data. This option is enabled by the compiler directive:

#define REJECT_COMPLEX

[gdevenyi]

Only request I would have is that the first commit be split into relevant separate changes rather than a single commit.

[gdevenyi]

Oh and it looks like there's no modification to the configure or cmake to expose these options.

[neurolabusc]

@gdevenyi I don't think this needs to be three commits - the code uses ifdef blocks to isolate each. I think the vast majority of developers will want to switch them on/off together. Only advanced cases would separate them (e.g. a tool that mimics fslcomplex would want to allow complex data types; while most scalar processing tools will want to reject these images). The PIGZ option is dormant unless the user explicitly sets an environment variable.

I have added options to allow the user to switch on these features:

To compile with cmake, use the FSLSTYLE option:

cmake -DFSLSTYLE=ON ..

To compile with make in the nifti2 folder:

run "FSLSTYLE=1 make"

To demonstrate, assume the folder contains two files a.nii and a.nii.gz. Both fslmaths and nifti_tool will reject this:

nifti2 > fslmaths ~/a -add 13 anew
Image Exception : #61 :: Multiple possible filenames detected for basename: a
libc++abi: terminating with uncaught exception of type std::runtime_error: Multiple possible filenames detected for basename: a
zsh: abort      fslmaths a -add 13 anew

nifti2 > ./nifti_tool -mod_hdr -prefix anew -infiles a -mod_field descrip 'beer, brats and cheese, mmmmm...'

Image Exception : Multiple possible filenames detected for basename (*.nii, *.nii.gz): a.nii.gz

I added these features to my open source fslmaths clone niimath. Selfishly, having this patch included with nifti_clib will allow my to ease keeping my project up to date. However, given the popularity of FSL tools, I do think these features will insulate users from confusion.

[afni-rickr]

Hi @neurolabusc
For the direct AFNI and FSL references, are these to match the existing PIGZ reference as an AFNI evar, and an error code of 134 used by FSL?
I would like to alter this a bit and add a function like:
static int multi_extensions_exist(const char * fname);

Then the FSLSTYE part might be changed to:

if ( multi_extensions_exist(fname) ) {
#ifdef FSLSYLE
   fprintf("death by chocolate!\n");
   exit(134);
#else
   fprintf("warning: likely death by chocolate!\n");
#endif
}

[neurolabusc]

I tried to mimic the FSL behavior, where having a naming conflict (e.g. 'filename.nii' and 'filename.hdr') generates a hard error. I like your idea of generating a warning if this is not the case. Perhaps you want to add this in a way that will be familiar to AFNI users.

[afni-rickr]

How would you prefer to proceed? Would you like me to add a function to detect name conflicts? Would you like to? Thanks.

[afni-rickr]

@neurolabusc It might be preferable not to have exit() statements within a general library. It seems better to return failure (NULL) in such cases, and leave it for the calling function to terminate the existential thread.
Along with multi_extensions_exist(), it might be reasonable to add something like niti_datatype_in_list(int datum, const int * dlist);, which could be applied at a higher level. It does seem nice to avoid allocation in such as case, but if exit() seems okay, then alloc/free do not seem like a big penalty. And really, even datatype_in_list() might be better off outside the library, that is a small question.

[neurolabusc]

Feel free to adapt as you wish. My goal was to emulate fslmaths behavior, and the immediate exit() allows us to mimic the same error messages. If a developer wants to reject complex data, I do think that exiting here is acceptable, as this is a terminal error for these use cases.

[afni-rickr]

These Pigz functions seem useful. It might be good to define them as "static int", unless you want to make them public (and in such a case, might be called nifti_...).
It might be nice to name them like write_via_Pigz2(), rather than doPigz2().
Rather than accessing an env var in the library we could consider adding compress_type (or shorter) to the g_opts struct.
It might not need to be optionally compiled. If someone requests use of pigz and they do not have it on their system, it will fail, which seems fine. I am not sure of the benefit of #ifdef for this (unless there is a system-dependent build issue). It does not require pigz, it merely allows for it.

[neurolabusc]

My goal here was to allow the developer to emulate AFNI behavior, using the same environment variable a user could specify for AFNI. Feel free to rename or modify to follow the conventions of this library.

[afni-rickr]

Sorry, I did not realize some of my comments got hidden in a partial review...

[afni-rickr]

@neurolabusc Sure, this seems good. To proceed, so that your commits are recorded and I can add to them, would it be okay for me to make a new branch and pull your changes into it? Then I can make changes in the branch before seeing whether we are happy to merge into master. Thanks!

[neurolabusc]

Sounds good.

[hjmjohnson]

@afni-rickr Can the requests from this PR be addressed soon?

[seanm]

@neurolabusc this was merged over in the ITK fork of nifti_clib: InsightSoftwareConsortium#9

I did have a few questions in that PR that I wonder if you might be able to answer...?

[neurolabusc]

@seanm feel free to ask questions or email for a zoom meet.

@afni-rickr I wonder if we should just use the ITK fork as a PR to this main repo

[seanm]

@seanm feel free to ask questions or email for a zoom meet.

@neurolabusc my questions are here: InsightSoftwareConsortium#9 (review) Would be great if you could reply there. I can make a followup PR to clean things up a bit based on your responses.