Dev/use tissue2MR - generating Permittivity and Conductivity for whole-spine dataset #21
Conversation
Script running OK on windows, problem recognizing input nifti files.
|
Thanks for opening this @sriosq ! I'm trying to finish another task this morning, I'll take a look at this PR ASAP afterwards |
| ] | ||
|
|
||
| try: | ||
| subprocess.run(cmd, check=True) |
There was a problem hiding this comment.
This isn't good practice unfortunately - it will make it very difficult to debug / test, and also make it challenging for cross-platform functionality.
A better approach would be to import your tissue2MR python module and call the python functions directly.
| bids_sidecar['input file'] = str(merged_labels_path.resolve()) | ||
| bids_sidecar['command'] = 'python label_to_chi.py -s ' + str(bids_subject_dir) | ||
|
|
||
| bids_sidecar['anatomy'] = {} |
There was a problem hiding this comment.
Why remove the bids sidecar that report the values that were used to generate the map from the label? I think this is valuable - especially since the values can change with new information, as you have discovered re: chi.
An alternative would maybe be to report the tissue2MR version / commit, but this makes it more difficult to actually go find the values / fetch them in the json sidecar files in a script.
| os.makedirs(bids_subject_dir / ".." / 'derivatives' / subject / 'anat' ) | ||
|
|
||
| chi_file = bids_subject_dir / ".." / 'derivatives' / subject / 'anat' / (subject + '_T1w-chi.nii.gz') | ||
| chi_file = bids_subject_dir / ".." / 'derivatives' / 'b0sim' /subject / 'anat' / (subject + '_T1w-chi.nii.gz') |
There was a problem hiding this comment.
Thanks for updating this! Maybe a comment could be left explaining why the change (i.e. previously was made for wholespine version X, but in version Y this was updated.
| @@ -0,0 +1,90 @@ | |||
| import nibabel as nib | |||
There was a problem hiding this comment.
This whole file duplicates most of the code in the label2chi.py. It would be better to reuse that code; there could be one file / function that would be called label_to_property, and add a flag depending on the property you want to use (eg chi, perm_cond, etc). That way you won't need to add new files for every other new map you'll want to create in the future (eg T2, T1, etc), you may just need to change one line or two in that one function.
The other reason this is a better approach is from a review side - since there are no tests and many similar lines, I would need to go line by line to check that you properly copied / didn't accidentally change a line.
Overall though, it looks like it should work the way you intend it to, but you would know best if it works or not.
| echo "[INFO] Processing $subject" | ||
|
|
||
| # Convert BIDS_DIR to Windows path | ||
| WIN_BIDS_DIR=$(echo "$BIDS_DIR" | sed 's|/cygdrive/\([a-z]\)/|\U\1:/|') |
There was a problem hiding this comment.
Is this line needed for every OS?
| fi | ||
|
|
||
| # Run Python to generate perm/cond maps | ||
| "C:/Users/Admin/Miniconda3/python.exe" "$WIN_DIR/label_to_perm_cond.py" -s "$WIN_BIDS_DIR/$subject" |
There was a problem hiding this comment.
This is too hardcoded; a user without a miniconda installation or with it installed in a different username won't be able to run this script when it is hardcoded like this.
There was a problem hiding this comment.
Overall it's a good start!
The purpose of these codes is to change hard-coding the quantitative values and use tissue2MR instead.
This will be a good change I think, but the values should still be transferred from tissue2MR into the processing file so that they can then be hardcorded in the BIDS sidecar JSON file.
Also, maybe add a note of any major changes in values from the previous version to your new tissue2MR call.
My initial testing with whole-spine dataset failed because the nifti files weren't recognized appropriately.
From the additional information you had DM'd me,
Im using windows but for some reason Im stuck here: nibabel.filebasedimages.ImageFileError: File E:/msc_data/whole-spine/derivatives/labels/sub-amuALT/anat/sub-amuALT_T1w_label-all.nii.gz is not a gzip file
[ERROR] tissue_to_MR failed!
This I'm afraid is out of the scope of this PR / repository. It seems there's three possible issues:
1- Your image is corrupted / wasn't fully pulled from git-annex
2- There is a windows-side issue resulting in nibabel (or however you are opening it in tissue2MR) can't load it
3- There is an issue in the tissue2MR script, which is an issue that should be opened in your repo.
A simple test should be simply to open a python session on your windows computer vs in a unix/linux/macos system, and try to open it via nibabel in it. If you can't in both, then the file is the issue; if you can in mac but not windows you would need to open an issue in the nibabel repo; if you can in both OS's, then the issue is in how you are loading it via nibabel in your tissue2MR script and/or how you are calling it via the system command in python (which I left a note, I wouldn't recommend you do).
Once that is resolved and you have addressed the other issues I raised in the comments, I can take another pass as this
I have added 2 new files: label_to_perm_cond.py and step_2_1_generate_perm_cond.sh, which are modifications of label_to_chi.py and step_2_generate_chi.sh files.
The purpose of these codes is to change hard-coding the quantitative values and use tissue2MR instead.
My initial testing with whole-spine dataset failed because the nifti files weren't recognized appropriately.