Testing is good #19
Testing is good #19
Conversation
|
@bilgelm tagging you on this as I'm adding a bunch of stuff to the Makefile to make black etc easier to apply locally |
|
Additionally, it looks like we're going to be able to use the deface mask to check whether or not the output has been defaced or not, see https://bids-specification.readthedocs.io/en/stable/glossary.html#defacemask-suffixes The next concrete steps are going to be to add checks for those defaced masks post pipeline, from there we can formally begin validating the pipeline outputs for various layouts (inherited, anat in first session folder, ...) with We are a bit limited at the moment by the size of Freesurfer (~18GB) wrt to running automated tests here on gh actions, hence another motivation for making the I'm envisioning a large enough gh actions runner that is able to execute the following: - name: set up 3rd party dependencies
run: |
make installfreesurfer
make installbidsvalidator
make installdocker
- name: run tests
run: make testsetc etc |
|
Part of testing is going to require conversion of the defacemask into This is going to a hard requirement for testing going forward, however, it all appears to be bids valid, see the following example (ignore the datalad stuff): sub-PS50
├── ses-baseline
│ ├── anat
│ │ ├── sub-PS50_ses-baseline_T1w.json -> ../../../.git/annex/objects/p8/xM/MD5E-s1321--4c2c1a45095196ebc00f34118bb99167.json/MD5E-s1321--4c2c1a45095196ebc00f34118bb99167.json
│ │ ├── sub-PS50_ses-baseline_T1w.nii.gz -> ../../../.git/annex/objects/x3/QK/MD5E-s15632037--1a3307921378137adb2fbf2827fab266.nii.gz/MD5E-s15632037--1a3307921378137adb2fbf2827fab266.nii.gz
│ │ └── sub-PS50_ses-baseline_mod-T1w_defacemask.nii.gz
│ └── pet
│ ├── sub-PS50_ses-baseline_pet.json -> ../../../.git/annex/objects/KQ/x8/MD5E-s3359--7237477e4d2802851eb4eb11c05e7ddd.json/MD5E-s3359--7237477e4d2802851eb4eb11c05e7ddd.json
│ ├── sub-PS50_ses-baseline_pet.nii.gz -> ../../../.git/annex/objects/61/9Q/MD5E-s145802787--af6e71a315e35dd1525c9abda5611280.nii.gz/MD5E-s145802787--af6e71a315e35dd1525c9abda5611280.nii.gz
│ ├── sub-PS50_ses-baseline_recording-manual_blood.json -> ../../../.git/annex/objects/Qk/G1/MD5E-s702--d8b2947c6ecc12911297da3b1b843514.json/MD5E-s702--d8b2947c6ecc12911297da3b1b843514.json
│ └── sub-PS50_ses-baseline_recording-manual_blood.tsv -> ../../../.git/annex/objects/j1/2g/MD5E-s1268--58e9845e8c151c1197dfb0237c697b99.tsv/MD5E-s1268--58e9845e8c151c1197dfb0237c697b99.tsv
└── ses-blocked
└── pet
├── sub-PS50_ses-blocked_pet.json -> ../../../.git/annex/objects/mx/j3/MD5E-s3324--25ecdc9902770d79e592651cd7a9ab5b.json/MD5E-s3324--25ecdc9902770d79e592651cd7a9ab5b.json
├── sub-PS50_ses-blocked_pet.nii.gz -> ../../../.git/annex/objects/4W/JX/MD5E-s142495013--0c27a85eb5fcfef6c2e6883e64b7d61d.nii.gz/MD5E-s142495013--0c27a85eb5fcfef6c2e6883e64b7d61d.nii.gz
├── sub-PS50_ses-blocked_recording-manual_blood.json -> ../../../.git/annex/objects/Qk/G1/MD5E-s702--d8b2947c6ecc12911297da3b1b843514.json/MD5E-s702--d8b2947c6ecc12911297da3b1b843514.json
└── sub-PS50_ses-blocked_recording-manual_blood.tsv -> ../../../.git/annex/objects/ww/ZK/MD5E-s1295--ca58145d917c4f2aeda5e3c89fa804d4.tsv/MD5E-s1295--ca58145d917c4f2aeda5e3c89fa804d4.tsv
5 directories, 11 files |
|
@bilgelm @mnoergaard I think the latest comment above is a good way to proceed with testing and refactoring. Could you take a look and comment whether you agree or disagree? |
| from os import cpu_count | ||
| from bids.layout import BIDSLayout | ||
|
|
||
| import tempfile |
There was a problem hiding this comment.
pytest has temp dir fixtures (https://docs.pytest.org/en/6.2.x/tmpdir.html); not sure if that would be more advantageous to use than the tempfile based implementation
There was a problem hiding this comment.
Yeah, pytest has everything, but despite my owning a book on it I'm not wholly familiar. I think we could move to using exclusively pytest, but for the moment it works and I'm able to easily inspect the temp dir if something fails.
There was a problem hiding this comment.
Okay, this is getting out of hand. I've gone ahead and created a petutils library that includes the sort of directory tree's we're intent on testing on. It has it's own fixtures that use pytest's tmpdir. Going to just merge this and switch over directory creation to the fixtures contained in in openneuropet/petutils for further development.
| n_procs=nthreads, | ||
| ) | ||
| petdeface.run() | ||
|
|
There was a problem hiding this comment.
It would be good to check if the defaced PET file(s) is/are created.
There was a problem hiding this comment.
See my comment about the mask files, we're going to need to slightly modify the pipeline to output nifti masks instead of mgz, but that's irrelevant for the purposes of this branch/PR. I'd like to add all the tests even if it means everything is failing.
| import tempfile | ||
|
|
||
| # collect test bids dataset from data directory | ||
| data_dir = Path(__file__).parent.parent / "data" |
There was a problem hiding this comment.
Should we consider downsampling the test data to speed up the tests?
There was a problem hiding this comment.
From my understanding it's a registration issue with the current test data, either way I think we should optimize later and push through with it now.
| import pytest | ||
| from pathlib import Path | ||
| import shutil | ||
| import bids |
There was a problem hiding this comment.
L1 and L4 can be deleted as these imports are not being used
There was a problem hiding this comment.
I think pytest might be as I use a fixture from it, and bids will be used to validate that the outputs look correct.
Adding testing for eventual CI and in an effort to support: