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

Response to issues documented in #5.

Matlab

Unit tests and integration tests for the MATLAB version of the rsHRF toolbox were added to the current release (https://github.com/compneuro-da/rsHRF/commit/36f2d3a499867d10017051c16d89fdb82ede9461).

For a full overview of the MATLAB unit and integration tests see: https://github.com/compneuro-da/rsHRF/tree/master/unittests.

In order to run the unit tests use the following command (after having removed any old version of the rsHRF toolbox and the SPM plugin reinstalled):

run ./spm12/rsHRF/unittests/rsHRF_run_all_unittests.m

BIDS-App

For the BIDS-app we added unit tests and an integration test (single subject analysis on a BIDS formatted dataset).

These tests were added into the circleCI configuration and the workflow completed without issues (https://github.com/BIDS-Apps/rsHRF/runs/2863148153).

This CirleCI workflow was run on the latest BIDS-App release (including updates to fix issues mentioned in #2, #4 , #3).

For a full overview of the unit tests see : https://github.com/BIDS-Apps/rsHRF/tree/master/rsHRF/unit_tests.

The configuration of the integration test and unit tests in the CircleCI workflow can be found at: : https://github.com/BIDS-Apps/rsHRF/blob/master/.circleci/config.yml.

And in short is integrated as :

Unit tests

 run:
 name: run unit tests         
 command: |             
    python3 -m venv venv            
    . venv/bin/activate
    pip3 install -r requirements.txt            
    pytest rsHRF/unit_tests/ -p no:warnings

Integration test

-run:
name: run integration test
command: |
python3 -m venv venv
. venv/bin/activate
pip3 install rshrf
mkdir ${HOME}/scratch
mkdir ${HOME}/scratch/downloads
wget -c -P ${HOME}/scratch/downloads/ "https://osf.io/xv72t//download"
tar xf ${HOME}/scratch/downloads/download -C ${HOME}/scratch
rsHRF --bids_dir ${HOME}/scratch/ds002790/derivatives/fmriprep/ --output_dir ${HOME}/scratch/ds002790 -- participant_label
0001 --brainmask --estimation canon2dd --analysis_level participant
no_output_timeout: 6h

Remi-Gau / bids-app_rsHRF