Open sandrinebedard opened 1 year ago
Comparing b686796816549d4191dcb1a11a426b5145cfcfbd (_new
) and https://github.com/spinalcordtoolbox/PAM50/commit/f8cfc4b331abfda1caaf0d9e72be94df2a9858ac (master):
Few comments:
Is that normal that in the new version, values seem almost binary, ie: the same value is spread out along the S-I axis, without "smoothness" to it. Eg: zoomed version of C2_new (same value across ~20 voxels). Maybe we should inquired the Phillips group about that?
The cord seg and the spinal levels seem to be perfectly matching (good), although I have not verified all levels. @sandrinebedard have you verified that? See example of good matching below:
The world coordinate between the source (Phillips lab) and PAM50, for the same voxel location, is drastically different. I guess this this is the reason for the qform copy (see example below).
For posterity, should we put these scripts somewhere or is it fine to keep it zipped in this PR? @joshuacwnewton
I think it would be nice to include the scripts in this repo, alongside the files themselves. (In past NeuroPoly datasets, sometimes I've seen additional README.md
files that contain a text description + a code block
with the processing steps. I like that method a lot, because it auto-displays on the GitHub page for the subfolder, and provides more room to write plain-English descriptions.)
The only issue here is that there are both shell commands (.sh
) and some Python file renaming logic (.py
), making it more of a chore to just put all of the code in a README.md
file. :thinking:
Ah! I have an idea. We could pretty easily replace the Python script with some bash logic, since we already have access to info_label.txt
, which stores the mappings between spinal level names and filenames. Then we could just have a single set of bash steps that we put into the README. I'll add it to this PR, should take 5m. :)
Is that normal that C1 is completely blank?
Related SCT issue:
The cord seg and the spinal levels seem to be perfectly matching (good), although I have not verified all levels.
Possibly related past SCT discussion:
One other small idea: It might be best to commit the unprocessed spinal level files to this repo first (in a separate PR https://github.com/spinalcordtoolbox/PAM50/pull/4), Then, we merge this PR, which will update the files.
This allows us to refer to a commit within this repo in the README.md, so that the source files are backed up safely, and we are not at risk of the PhillipsLab
repo being deleted one day).
another alternative would be to upload the source files (from Phillips) and the script as release assets when doing the release of the updated spinal levels-- arguments:
- The cord seg and the spinal levels seem to be perfectly matching (good), although I have not verified all levels. @sandrinebedard have you verified that? See example of good matching below:
I'll verify them all!
- Is that normal that C1 is completely blank?
They did not include any C1 files
- Is that normal that in the new version, values seem almost binary, ie: the same value is spread out along the S-I axis, without "smoothness" to it. Eg: zoomed version of C2_new (same value across ~20 voxels). Maybe we should inquired the Phillips group about that?
I'll also check the https://github.com/PhillipsLab/pam50/tree/main/DREZ%20NIfTI to see if the Spinal Cord levels was teh right folder to take here.
Is that normal that the resampling is commented out?
- The world coordinate between the source (Phillips lab) and PAM50, for the same voxel location, is drastically different. I guess this this is the reason for the qform copy (see example below).
Regarding the differences in world coordinates and resampling, I wasn't sure how to proceed.
The dimensions and resolution of:
141x141x991
and 1x1x1
141x141x991
and 0.5x0.5x0.5
If we do resampling to 0.5 mm isotropic, the matrix size doubles, wich doesn't match anymore the PAM50 space.
I thought of doing registration with identity, however, since the world coordinates differ, it dosn't bring the spinal levels in the PAM50 space. How I overcame this issue is by copying the image header (for qform) and this also solves the resolution problem. I am not sure this is the best way to overcome this however...
- the repos is already pretty big (it took me quite some time to git clone)-- we wouldn't want to 'blow it out of proportion-- especially with increasingly more constraining regulation about repos size limit etc.
I think that this is the unavoidable nature of trying to version-track binary files like we're doing here. For example, the total size of the recently-added histology files is 93MB. By comparison, the original Phillips files are ~700kB each, for a total of 16.7MB.
With or without the additional Phillips files, slow repo clones will be an issue. So, if it is unavoidable, I lean towards keeping the files in-repo for posterity.
(Additionally, the GitHub repo size limit seems to be 5GB (strongly recommended), so 16.7MB hardly moves the needle in that regard.)
the script is very specific to the Phillips files, and i'm afraid if we put it in the repos, some ppl might interpret what it is for, etc.
My thought process was a README.md file containing the following contents:
### PAM50 Levels
These level files are a slightly modified copy of the [level files](https://github.com/PhillipsLab/pam50/tree/main/Spinal%20Cord%20Levels%20NIfTI) produced by the [Phillips Lab](https://github.com/PhillipsLab):
Modifications include:
- Reorient from LPI to RPI
- Change data type from float64 to float32
- Copy header from current PAM50/spinal_levels
- Rename files
To reproduce the modified files, please run `git checkout <commit>`, then run the following script in your terminal:
```
<insert commands here>
```
That way it is unambiguous what the commands were used for, and no one can just accidentally run the commands, similar to what was done for sct_testing_data/template/README.md
.
They did not include any C1 files
Hum, I am a bit reluctant to include a file labeled "C1 spinal levels" but without any label inside-- this is quite confusing for users. Maybe we should just get rid of it?
I'll also check the https://github.com/PhillipsLab/pam50/tree/main/DREZ%20NIfTI to see if the Spinal Cord levels was teh right folder to take here.
I think we should include the Phillips group right away in the conversation to avoid any confusion. I'll take care of that.
How I overcame this issue is by copying the image header (for qform) and this also solves the resolution problem.
Only changing the image header should not affect the physical dimension of the voxel, unless (i) you also did a resampling afterwards or (ii) the "1mm iso" on the native image was wrong, and the true voxel size it was in fact 0.5mm iso.
I found some samll mistmatch with the spinal cord (red) of the PAM50 and spinal levels:
(I did not pass yet the entire cord)
It looks like the R-L are reveresed, this suggest that maybe the reorientation to RPI wasn't necessary and the image header is wrong, which maybe also suggest that the true voxel size is maybe also 0.5mm iso.
(ii) the "1mm iso" on the native image was wrong, and the true voxel size it was in fact 0.5mm iso.
I am testing the processing without reorientation to see if we still have mismatch.
No mismatch when I remove the reorientation to RPI: (slice 942)
No mismatch when I remove the reorientation to RPI: (slice 942)
Ah! Good finding 😊
I updated the files removing the reorientation to RPI: https://github.com/spinalcordtoolbox/PAM50/pull/3/commits/ce64bdf30f7b544ae9eb5bf76dd883ecade01b0f process_spinal_levelsv2.zip
I updated the files removing the reorientation to RPI: https://github.com/spinalcordtoolbox/PAM50/commit/ce64bdf30f7b544ae9eb5bf76dd883ecade01b0f (process_spinal_levelsv2.zip)
I have taken the above scripts and put them into a README.md
file. But, I slightly modified the scripts to remove the dependency on Python. I also modified the script so that it can be run directly on commit e854bbad9ab550fd93acabeaf43c97cf66b3a4e5.
(The differences between "process_spinal_levelsv2.zip" and the README.md
file are shown here.)
We decided not to pursue the inclusion of the Mendez et al. paper metrics to update the spinal level in the PAM50 template . Here is a summary of what was done with the related issues:
Description
Partially fixes https://github.com/spinalcordtoolbox/spinalcordtoolbox/issues/3952.
This PR takes the unprocessed Philips Lab spinal level files (stored in the
spinal_levels_PhillipsLab/
folder), modifies and renames them, adds them to thespinal_levels/
folder, then removes the originalspinal_levels_PhillipsLab/
folder.FOR NOW, NOT CONTINUED, see https://github.com/spinalcordtoolbox/PAM50/pull/3#issuecomment-1644622915
Processing
I noticed some differences between the data from the Phillips Lab and the current
PAM50/spinal_levels
:Orientation--> (see comment)LPI
vsRPI
1x1x1
vs0.5x0.5x0.5
float32
vsfloat62
.However, the matrix size is the same (141, 141, 991), resampling to 0.5mm would double the matrix size. Copying the header from the current
PAM50/spinal_levels
fixed the problem instead of resampling. I want to confirm this is ok.Processing steps includes:
Reorient from(see comment)LPI
toRPI
float64
tofloat32
PAM50/spinal_levels
Exact processing commands are included in the README.md of this PR.
Old version of processing steps
I included the processing scripts in the attached zip file: [process_spinal_levels.zip](https://github.com/spinalcordtoolbox/PAM50/files/10726837/process_spinal_levels.zip) (put the python script in the same directory as the bash script ) Here is what I ran for processing: ``` chmod +x ./process_spinal_levels.sh ./process_spinal_levels.sh "./pam50/Spinal Cord Levels NIfTI"Bibliography