Conversation
…v5 (and Mac25Cyno.
glasserm
left a comment
There was a problem hiding this comment.
Please see my above comments.
Co-authored-by: Tim Coalson <coalsont@users.noreply.github.com>
| # Download the PROFUMO Singularity image from the following link and place it in the PFM folder, | ||
| # or change the path below to point to your own copy of the image | ||
| # https://balsa.wustl.edu/myelin/download?dirName=public&filepath=profumo_v2.sif | ||
| ProfumoSingularity="$HCPPIPEDIR/PFM/profumo_v2.sif" |
There was a problem hiding this comment.
Usually stuff like this is handled in the HCP Pipelines setup script.
There was a problem hiding this comment.
As I commented earlier, unless we are expecting every user to be required to run PFMs, then downloading and specifying the location of the profumo container seems specific to only this pipeline, unlike FSL or wb_command, which are used by nearly all pipelines. Even for freesurfer, bbregister is used in multiple pipelines, which is why it makes sense to have in the global setup.
There was a problem hiding this comment.
I m thinking if we can make it overridable rather than required in the global setup:
ProfumoSingularity="${ProfumoSingularity:-$HCPPIPEDIR/PFM/profumo_v2.sif}"That way users who don't run PFMs never have to set anything, but anyone running this regularly can export it from their setup script if they prefer.
There was a problem hiding this comment.
I don't expect all end users to feel comfortable editing a line that has ${something:- in it, but I do expect some of them to download it to somewhere other than the pipelines folder. Someone who is comfortable with bash syntax can easily set things up some other way anyway.
This particular container appears to be single-purpose for a single pipeline. As long as that is the case, I don't think it belongs in a centralized configuration location (where people who aren't running PFMs would have to read it and understand that the setting isn't relevant to them).
There was a problem hiding this comment.
With regard to putting it in the setup script, if we make it optional in there, isn't that fine? I think it might be best to keep all external dependencies referenced in there instead of scattered across the pipelines. I don't think we currently have any external dependencies that are not in the setup script.
There was a problem hiding this comment.
If we expect nearly all users to run PFMs, it might make sense. For any user that isn't going to run PFMs, having it in the global setup would just be clutter that they have to read, understand, and then skip over. We can move it to a global location later if it becomes something that an overwhelming majority of users will need to do. Keeping the global setup short by only containing things that nearly all users need to do is a benefit to the user experience and gets them started faster before asking them to make judgement calls.
"External" isn't the reason I put things in a global location, it is whether they are used in more than one pipeline (or aren't actually associated with any specific pipeline, like VolumeToCIFTI.sh). MSM, while also being a single file, is used in both PostFS and MSMAll, hence it would not be a good fit for being in a single pipeline's batch, as then users would need to configure it twice. That does not currently apply to profumo, and I suspect it may never apply. The hippunfold container likewise will probably only be used in one pipeline, and it isn't clear what percentage of users will want to use that, either, so it would also be a candidate for batch script configuration, regardless of being external.
There was a problem hiding this comment.
In the long run, I think HippUnfold will be run by most/all users (as the future CIFTI grayordinates space will use hippocampal surfaces rather than hippocampal volumes). Also, in the long run, I think PFMs will be the primary functional network analysis tool. I am leaning strongly towards putting them in the setup script for that reason.
…d from hcp_multli_run_fix), add variance normalization option that uses the clean_vn dscalar from the tICA pipeline. Add option for weighting data by vertex areas, using the mean area for volume grayordinate weights.
Co-authored-by: glasserm <glasserm@wusm.wustl.edu>
Co-authored-by: Tim Coalson <coalsont@users.noreply.github.com>
…ndling, simplify va_norm.grayordinates creation.
… subject-specific
| if [[ "$VAweightBool" == 1 ]]; then | ||
| # create VA_norm cifti with volume grayordinates filled with ones areas for weighting | ||
| VAnorm=${StudyFolder}/${Subject}/T1w/fsaverage_LR${LowResMesh}k/${Subject}.midthickness${RegString}_va_norm.${LowResMesh}k_fs_LR.dscalar.nii | ||
| VAgray=${StudyFolder}/${Subject}/T1w/fsaverage_LR${LowResMesh}k/${Subject}.midthickness${RegString}_va_norm.grayordinates.${LowResMesh}k_fs_LR.dscalar.nii |
There was a problem hiding this comment.
This file has normalized vertex areas from /T1w space but the subcortical voxels (which are filled with 1's) are in /MNINonlinear space. Should the file be saved in /T1w or /MNINonLinear? Or should it be a temporary file like the one created in RSNRegression? One issue with it being a temporary file is that it would need to be created twice (in RunPROFUMO step and again in the postPROFUMO step).
There was a problem hiding this comment.
For fMRI don't we use the MNINonLinear vertex areas elsewhere? I think this is correct given that the fMRI data are mapped onto the surface in MNI space. That would solve this delemma...
There was a problem hiding this comment.
The surface used for mapping is irrelevant to the choice of vertex areas, because both the surfaces and volume are warped with the same warp (and thus the results are effectively identical to doing it in native space, as long as the jacobians aren't extreme - the choice of using MNI for mapping cortical data was purely for convenience and storage efficiency). The question is what they are being used for, and whether that should use the subject's original brain shape or not.
There was a problem hiding this comment.
RSNRegression uses normalized the /T1w areas and creates a temporary file with the volume components filled with ones. So I've now done the same in both the runPROFUMO and postPROFUMO steps.
Pipeline for PROFUMO decomposition. I started from Andrea's Yang's PFMs code, and finished generalizing it in the HCP Pipelines style. I tested in on Mac25Rhesus and Mac25Cyno, and it is working there.
Known issues:
(1) This PR does not include the ./PFM/profumo_v2.sif singularity container, because it is 6.6G.
(2) The full run example script uses Mac25Rhesus rather than something more standard like HCP1071 and has local paths in it.
(3) The matlab needs to be compiled