remi-gau / bids-app_rshrf Goto Github PK

Lack of formal tests

I think my main concern would be that the code base does not seem to have any
automated tests that could help define what is the expected behavior of part of
the code (unit tests) or the whole (integration tests).

I am aware that that asking for this is setting the bar fairly high and trying
to ask from scientists the same as would be required from a team of developpers
who work in industry (And I will readily admit that a lot of my code so far has
been untested or is under-tested, but I am trying to change this and ask the
same from my colleagues.)

That being said, it must be recognized that shipping code without test is bit
akin to running an experiment without calibrating our instruments beforehand.

Moreover the bids-apps paper does mention that:

each BIDS App is required to include at least one smoke or integration test
running on an example BIDS dataset).

So I would at minima suggest that some automated test be added in the circle CI
configuration of the BIDS app (though a quick look around also showed that
several other BIDS-app do not fullfill this requirement.).

For the Matlab version, automated testing might be a bit more complicated to set
up but Mathworks has recently released some github action that should allow some
form of automated testing:
https://www.mathworks.com/help/matlab/matlab_prog/continuous-integration-with-matlab-on-ci-platforms.html

Expected is actually not a BIDS dataset

Documented in this issue: #2

The following does not work does work because the package expects files ending
with a preproc.nii and not *preproc_bold.nii

input_dir=../../derivatives/fmriprep
output_dir=../../derivatives/rsHRF

input_dir=/home/remi/gin/olfaction_blind/derivatives/fmriprep
output_dir=/home/remi/gin/olfaction_blind/derivatives/rsHRF

mkdir $output_dir

docker run -ti --rm \
-v $input_dir:/input_dir:ro \
-v $output_dir:/results \
bids/rshrf  --bids_dir input_dir \
            --output_dir results \
            --analysis_level participant \
            --brainmask \
            --estimation canon2dd \
            --participant_label blnd01

This gives the following error

rsHRF: error: There are no files of type *preproc.nii / *preproc.nii.gz Please make sure to have at least one file of the above type in the BIDS specification

I suspect the error is due to the fact that the BIDS app actually expect data
that does in fact NOT conform to the BIDS specifications.

BIDS files have the general format:

sub-<label>_entity1-<label>_entity2-<label>[...]_suffix.ext

The general form for BIDS derivatives data is described here:
https://bids-specification.readthedocs.io/en/stable/05-derivatives/02-common-data-types.html#preprocessed-or-cleaned-data

<pipeline_name>/
    sub-<participant_label>/
        <datatype>/
            <source_entities>[_space-<space>][_desc-<label>]_<suffix>.<ext>

Which means that preprocessed bold data should have the suffix bold and not
the preproc one that rsHRF expects. Something like this:

sub-01/func/sub-01_task-rest_desc-preproc_bold.nii.gz
sub-01/func/sub-01_task-rest_desc-preproc_bold.json

Another issue is that rsHRF expects the task label to be rest. This is
indeed the "suggested" label by the BIDs specification and makes for a good
default but resting state dataset can technically have any task label.

Those should be fixed to make the app work on most valid derivatives BIDS
datasets. I suspect it would also be preferable to have the following as
defaults:

• desc: preproc
• suffix: bold
• task: rest but to give the possibility to let users modify them in their
  docker run command.

Bot issues could be tested on the ds002790 dataset from open neuro that should
be accessible from openeuro with datalad.

datalad clone ///
cd datasets.datalad.org/
datalad install openneuro datalad
install openneuro/ds002790
cd openneuro/ds002790
# get rest data first subject
datalad get /derivatives/fmriprep/sub-0001/func/sub-0001*task-restingstate_acq-seq*\*

Additonally when processing a BIDS data set, it would be preferable to generate
a dataset_description.json that helps track the provenance of the of the
input.

See here for more details:
https://bids-specification.readthedocs.io/en/stable/03-modality-agnostic-files.html#derived-dataset-and-pipeline-description

Finally, one little extra: it could be nice when dealing with BIDS dataset to
make sure that the input data is a derivatives dataset (this information should
be included in the DatasetType of the dataset_description.json of a
dataset): the reason for this is to prevent users from running rsHRF on data
that has not been minimally preprocessed (i.e realigned).

Documentation python package: confusing extensions

Documented in this issue: #3

Several examples tend to a be a bit too specific and seem to imply that the
files to be processed cannot have a .nii.gz extension.

For example:

docker run -ti --rm \
-v /path/to/input.nii:/input.nii:ro \
-v /path/to/mask.nii:/mask.nii \
-v /path/to/output/directory/results:/results \
bids/rshrf --input_file input.nii --atlas mask.nii --estimation canon2dd --output_dir results

Might probably be less confusing:

docker run -ti --rm \
-v /path/to/input:/input:ro \
-v /path/to/mask:/mask:ro \
-v /path/to/output/directory/results:/results \
bids/rshrf --input_file /input/input_file --atlas /mask/mask_file --estimation canon2dd --output_dir results

Or maybe it is just me, but it took me more than 30 minutes to track down the issue when trying to implement this.

Single file output name

Documented in this issue: #4

After running the following:

subject=blnd01

input_dir=/home/remi/gin/olfaction_blind/derivatives/fmriprep/sub-$subject/func

input_file=`ls $input_dir/sub-blnd01_task-rest*2009*preproc_bold*nii.gz`
mask=`ls $input_dir/sub-blnd01_task-rest*2009*mask*nii.gz`

echo $input_file
echo $mask

output_dir=/home/remi/gin/olfaction_blind/derivatives/rshrf

rsHRF --input_file $input_file \
	--atlas $mask \
	--estimation canon2dd \
	--output_dir $output_dir

I get these files

sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_bold_deconv.nii.gz
sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_bold_event_number.nii.nii.gz
sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_bold_FWHM.nii.gz
sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_bold_Height.nii.gz
sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_bold_hrf.mat
sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_bold_plot_1.png
sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_bold_plot_2.png
sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_bold_Time2peak.nii.gz

Wrong extension

The extension of this file seems wrong

sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_bold_event_number.nii.nii.gz

Non BIDS name

Although not required, when the input is from a BIDS dataset, it would be
preferable if the output also conformed to the generic BIDS filename format (see
below) when inputs file ARE BIDS valid filenames: the reason for this is that it
simplify the creation of pipelines when filenames follow a given pattern.

# generic BIDS filename format
sub-<label>([_entity-<label>])*_suffix.ext

Maybe something like this could do.

sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_deconv.nii.gz
sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_eventnumber.nii.gz
sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_FWHM.nii.gz
sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_height.nii.gz
sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_hrf.mat
sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_label-1_plot.png
sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_label-2_plot.png
sub-blnd01_task-rest_run-01_space-MNI152NLin2009cAsym_desc-preproc_T2P.nii.gz

Failed demos

Documented in this issue: #1

Using Matlab 2017a on Ubuntu 18.04

The following demos do not run:

• rsHRF_demo_impulseest.m

No public property RegularizationKernel exists for class idoptions.impulseest.

Error in rsHRF_demo_impulseest (line 15)
options.RegularizationKernel = 'none'; %Regularizing kernel, used for regularized estimates
of impulse response for all input-output channels. Regularization reduces variance of
estimated model coefficients and produces a smoother response by trading variance for bias

• rsHRF_demo_voxel_calcium.m

Error using figure
There is no WindowState property on the Figure class.

Error in rsHRF_demo_voxel_calcium (line 111)
figure('WindowState','maximized');

• rsHRF_demo_LFP_IOS.m with flag_all=1;

Undefined function or variable 'wgr_get_parameters'.

Error in rsHRF_demo_LFP_IOS>@(x)wgr_get_parameters(hrf_Blind(:,x),dt)

Error in rsHRF_demo_LFP_IOS (line 97)
        PARA = arrayfun(@(x) wgr_get_parameters(hrf_Blind(:,x),dt),1:size(hrf_Blind,2),
        'UniformOutput', false);

• rsHRF_demo_LFP_IOS.m with flag_sliding_windows=1;

Undefined function or variable 'wgr_get_parameters'.

Error in rsHRF_demo_LFP_IOS>@(x)wgr_get_parameters(hrf_Blind(:,x),dt)

Error in rsHRF_demo_LFP_IOS (line 97)
        PARA = arrayfun(@(x) wgr_get_parameters(hrf_Blind(:,x),dt),1:size(hrf_Blind,2), 'UniformOutput',
        false);