-
Notifications
You must be signed in to change notification settings - Fork 272
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
StructuralQC integration #201
Conversation
Also, a test run is available at myelin/brainmappers/Connectome_Project/HCP_Lifespan/_testing/PostFSQC/ I changed the extension of the scene file to |
The test scenes worked for me with the exception of this error message on the 4th scene that results in a crash: Tab for deleting with tabIndex=33 is not at that index in array of tabs. The code changes look fine. |
I was unable to replicate the above issue. |
@coalsont : This is looking great. Thanks for working on it. Re your earlier 3 points:
Couple things: |
If we wanted to reduce the size of that |
I have not yet tested the links option, and I'll look into using global/templates. |
BTW: I just finished taking a look at your test subject on myelin via an SSHFS mount. Took a while to transfer/load the files into wb_view, but once I was able to work with them, all the scenes looked good and the snapshots looked fine as well. I did notice that a small bit of "zoom" that I had added into the Volume views in Scene 1 and Scene 4 is no longer present, which I assume is related to the fact that the 3 plane volume view in 'row' mode now orders as Sag, Cor, Ax (vs. Cor, Sag, Ax previously). It's a minor bit of wasted space, and not one that I'm going to attempt to create a new template scene file to change. I probably wouldn't even have noticed it if I didn't compare the snapshots generated by your test vs. those already existing for the same subject on IntraDB. |
…regex find with something simple
I didn't want to mix the QC templates into a folder used for other things, so I made a global/templates/StructuralQC folder. The easy way to deal with the MNI T1 file being in another directory was this (note the '..' in the link target):
Links mode appears to work. |
Also, I'm not that worried about data files in the repo history, but a squash merge would probably deal with that. |
As for "zeroing out background", since I don't know that the T1w image has a predictable range of values, my first thought is to dilate a brain mask by something like 20mm (note, this is slow in wb_command until 1.4.0, released november 2019), though that probably wouldn't keep the neck. Otherwise, it would probably need -volume-stats -percentile, some guesswork about noise standard deviation and percentage of FoV occupied by head, dilate/erode, and remove islands. I'm not sure how much we care about saving ~20MB per subject, so maybe leave this as-is. |
I think Mike was suggesting you dilate the brain mask. |
20mm dilation of MNINonLinear/brainmask_fs.nii.gz did reduce the size by ~20MB, but clipped off a little skull in at least one place. I have set it to 30mm default (with an optional argument controlling or disabling it), which saves ~15MB:
In scene 3, toggling layer 3 off with layer 2 on shows some blank space around the eyes and lower neck edges where it was masked - with both layers on and toggling layer 2, it is practically invisible. |
"$tempmask" | ||
wb_command -volume-math 'data * (mask > 0)' "$volumeOut" \ | ||
-var data "$tempresample" \ | ||
-var mask "$tempmask" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we explicitly remove the temp files that we've created, to leave the temp space as we found it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that is good practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tempfiles_create does a tempfiles_add internally, which does rm
in a trap ... EXIT
, so this is already taken care of. This is the purpose of tempfiles.shlib
, temporaries it knows about get deleted without needing to remember to rm
them manually.
@coalsont : If you are happy with this, then I think we are good? I thought that maybe FSL had a predefined "head" mask in MNI152 space that we could use, but I don't see one, so applying a generous dilation to brainmask_fs is a fine solution. If we end up missing a little bit of neck, that's fine. |
@mharms I don't use FSL much, so I wouldn't know that kind of detail. I'm fine with the current state. |
Sounds good to me too. |
Items to consider:
GenerateStructuralScenes.sh
for handling templates is to copy them as files to the output directory - since the pipelines may be on a different mountpoint than the subject data, copying as files is the least likely to have issues when opened on a different computer, though it will take ~82MB (edit: looks like the .surf.gii files aren't used, do they need to be in the QC_templates folder at all? edit2: but the largest file is the T1w resampled to MNI space...with an affine?) instead of "just" ~68MB per subject (for the scene file and files created by the script, mainly the affine-resampled MNILinear T1w).PostFreeSurferPipeline.sh
currently does not specify a non-default value for this option.--path
for the study folder, and had no usage info. I added usage info which lists the arguments, and it accepts both--path
and--study-folder
, but the usage only displays one option switch for each input, and currently it will show only--study-folder
, which is more descriptive and I believe matches newer pipeline scripts.For a diff of the StructuralQC script compared to its prior version, see https://github.com/coalsont/StructuralQC/pull/1/files?diff=unified&w=1 - even with whitespace changes hidden (was a mess of tabs + spaces, 2 vs 4 spaces...), a lot of it was modified to add whitespace safety, remove the internal subjects loop and special empty string handling, etc.
I have not tested the changes to
..._1res.sh
, but it seemed appropriate to keep it mostly synchronized (adding back in the whitespace safety that was broken when the long lines were split, and adding usage info).