HCP HippUnfold and Post HippUnfold Implementations #320

glasserm · 2025-01-08T03:56:41Z

This is an implementation of the current version of HippUnfold 1.5.2-pre.2: https://hippunfold.readthedocs.io/en/latest/. This requires the current development version of Connectome Workbench to use. The file organization is non-final and needs evaluation and comment. There are a variety of unfinished things marked with TODO. Some of these are pending comments and some pending a new planned HippUnfold 2.0 release.

For those who have local access, the data are in here: myelin/brainmappers/Connectome_Project/YA_HCP_Final/100307/T1w/HippUnfold and MNINonLinear/HippUnfold

…eliminary and is CERTAIN TO CHANGE including file organization given additional Connectome Workbench features needed, HippUnfold bugs discovered and further development and optimization needed.

*Add Support for multi-resolution meshes *Add Spec Files *Add Support for Dentate in cifti-matlab

*Support reading CIFTI files with dentate in cifti-matlab *Use consistent hippocampal labels in CIFTI and volume *Code cleanup *Remove matlab hack and just recreate all CIFTI files

*Add some combined hippocampal and cerebral spec files

demsarjure

I reviewed this and added some minor comments/discussion points.

demsarjure · 2025-01-08T07:59:27Z

Examples/Scripts/SetUpHCPPipeline.sh

@@ -26,6 +26,8 @@ export MATLAB_COMPILER_RUNTIME=/export/matlab/MCR/R2017b/v93
 # If a suitable version of wb_command is on your $PATH, CARET7DIR can be blank
 export CARET7DIR=
 export HCPCIFTIRWDIR="$HCPPIPEDIR"/global/matlab/cifti-matlab
+export HIPPUNFOLDPATH="${HOME}/pipeline_tools/HippUnfold/khanlab_hippunfold_latest.sif"


This seems like a highly specific local path. Do you have any conventions about where users should put external tools that are required to run certain HCP pipelines? For example, $HCPPIPEDIR/external.

Users should not add anything inside of the repo folder. That way madness lies.

More to the point, the pipelines setup instructions don't tell users where to install FSL, or freesurfer, or MSM, etc, and I don't think we should start doing so. The standard "set this variable to where you have downloaded hippunfold" formula should apply.

This is the standard convention we have used for all of the HCP Pipelines so it is odd to be complaining about it here. It is intended to be changed but is not supposed to be blank (unlike the CARET7DIR).

demsarjure · 2025-01-08T08:07:37Z

HippUnfoldHCP/HippUnfoldHCP.sh

+log_Msg "T2w HippUnfold completed."
+log_Msg "Running T1wT2w HippUnfold for subject: $Subject"
+apptainer run --bind $StudyFolder -e $HIPPUNFOLDPATH $HippUnfoldT1wT2wFolder $HippUnfoldT1wT2wFolderOut participant --modality T1w --cores all --force-output --generate_myelin_map --output-density 0p5mm 1mm 2mm --force-nnunet-model T1T2w
+log_Msg "T1wT2w HippUnfold completed."


I knew this day would come sooner or later. This will force us to run containers inside containers (HippUnfold inside QuNex), on paper it should work. I guess we will see in practice if there are any issues with this.

What we need to figure out is whether we should put the HippUnfold container inside the QuNex container (this adds a couple GBs to the QuNex size) or if we require users to download it manually. I am leaning towards the first option as it is more user friendly and nowadays size is not that important as we all have good connectivity and enough disk space. A case for having the container outside could be made by saying this is a pipeline that will not be executed by most of our users?

Need to think this through a bit.

Do they provide their code such that it can be run without their container wrapping it? I think of containers as a convenience of not having to manually configure a bunch of software, not as a replacement for being able to install someone's software.

HippUnfold v2.0.0 will be available on conda (ETA ~1 month)
The current HippUnfold v1.5.2 can be installed with git clone; pip install -e ., however it also uses containers internally for dependencies like nighres, wb_command, etc. See here (and note that poetry is not necessary, its just for python linting)

Thanks for the info. I felt like adding your whole container into our container would be suboptimal space wise as there should be some overlap between the tools we depend on. We indeed both use wb_command, so that one would be redundant for sure, there could be others. I will first try to do this optimally and install all the dependencies manually. If this does not work and is too much of a hassle, we can go the container route.

@glasserm What is your preferred timeline here? Would you like to wait for HippUnfold v2.0.0 and then start processing with this one. Sounds like a major upgrade, that is only a month or so away. Or is this urgent and waiting an extra month is not an option?

RE: #Couldn't get non-BIDS to work (wouldn't do anything) #Seriously: don't put a $ here...

The Subject wildcard should simply be subject. This can be used to avoid having to make a BIDS directory as input. Note that hippunfold expects to iterate over all possible completions of the wildcard subject (and can also iterate over other wildcards like session or acq, as per pyBIDS). Best parallelization can be achieved by running batches of subjects, but I udnerstand you may not want to set up your pipelines that way.

Thanks for the info. I felt like adding your whole container into our container would be suboptimal space wise as there should be some overlap between the tools we depend on. We indeed both use wb_command, so that one would be redundant for sure, there could be others. I will first try to do this optimally and install all the dependencies manually. If this does not work and is too much of a hassle, we can go the container route.

@glasserm What is your preferred timeline here? Would you like to wait for HippUnfold v2.0.0 and then start processing with this one. Sounds like a major upgrade, that is only a month or so away. Or is this urgent and waiting an extra month is not an option?

This isn't intended to be merged until HippUnfold v2.0.0 is available and the PR reflects and changes needed for that. I agree that ideally you would install HippUnfold + dependencies in QuNex rather than the container.

RE: #Couldn't get non-BIDS to work (wouldn't do anything) #Seriously: don't put a $ here...

The Subject wildcard should simply be subject. This can be used to avoid having to make a BIDS directory as input. Note that hippunfold expects to iterate over all possible completions of the wildcard subject (and can also iterate over other wildcards like session or acq, as per pyBIDS). Best parallelization can be achieved by running batches of subjects, but I understand you may not want to set up your pipelines that way.

I currently wrap in some BIDS because I couldn't get the non-BIDS options to work reliably. If you guys provide an ability to simply specify paths to relevant inputs, I will remove the wrapped BIDS.

demsarjure · 2025-01-08T08:10:30Z

HippUnfoldHCP/PostHippUnfoldHCP.sh

+opts_SetScriptDescription "Make some BIDS structures and run HippUnfold"
+
+opts_AddMandatory '--study-folder' 'StudyFolder' 'path' "folder containing all subjects"
+opts_AddMandatory '--subject' 'Subject' 'subject ID' ""


This was changed to Session during the refactoring?

I think some of those changes might have been overzealous. In any case, this is a cross-sectional pipeline at the moment and I have not thought at all about making it longitudinal. That might be something that HippUnfold would need to internally do. We rely on FreeSurfer to setup the longitudinal processing for cerebral cortex.

demsarjure · 2025-01-08T08:10:36Z

HippUnfoldHCP/HippUnfoldHCP.sh

+opts_SetScriptDescription "Make some BIDS structures and run HippUnfold"
+
+opts_AddMandatory '--study-folder' 'StudyFolder' 'path' "folder containing all subjects"
+opts_AddMandatory '--subject' 'Subject' 'subject ID' ""


This was changed to Session during the refactoring?

Same as the other comment.

demsarjure · 2025-01-08T08:11:09Z

HippUnfoldHCP/PostHippUnfoldHCP.sh

+${CARET7DIR}/wb_command -add-to-spec-file ${AtlasHippUnfoldFolder}/${Subject}.${Mesh}.wb_spec INVALID ${AtlasFolder}/T1w_restore.nii.gz
+${CARET7DIR}/wb_command -add-to-spec-file ${AtlasHippUnfoldFolder}/${Subject}.${Mesh}.wb_spec INVALID ${AtlasFolder}/T2w_restore.nii.gz
+
+#TODO: Merge Native Meshes, anything with 0pt5mm meshes?


TODO or not TODO?

Dependent on HippUnfold 2.0

demsarjure · 2025-01-08T08:11:31Z

HippUnfoldHCP/PostHippUnfoldHCP.sh

+#$PhysicalHippUnfoldFolderOut/hippunfold/sub-${Subject}/surf/sub-${Subject}_space-${Modality}_den-${Mesh}_label-hipp_atlas-multihist7_subfields.dlabel.nii
+
+#VolumeLabels
+#$PhysicalHippUnfoldFolderOut/hippunfold/sub-${Subject}/anat/sub-${Subject}_hemi-${Hemisphere}_space-${Modality}_desc-subfields_atlas-multihist7_dseg.nii.gz


Cleanup these notes at the end?

They will likely stay as code documentation to help me remember what HippUnfold does.

coalsont · 2025-01-10T03:31:55Z

HippUnfoldHCP/HippUnfoldHCP.sh

+cp "$T1wImage" "$HippUnfoldT1wFolder/sub-${Subject}_T1w.nii.gz"
+cp "$T2wImage" "$HippUnfoldT1wFolder/sub-${Subject}_T2w.nii.gz"
+cp "$T1wImage" "$HippUnfoldT2wFolder/sub-${Subject}_T1w.nii.gz"
+cp "$T2wImage" "$HippUnfoldT2wFolder/sub-${Subject}_T2w.nii.gz"
+cp "$T1wImage" "$HippUnfoldT1wT2wFolder/sub-${Subject}_T1w.nii.gz"
+cp "$T2wImage" "$HippUnfoldT1wT2wFolder/sub-${Subject}_T2w.nii.gz"


Does it really need to create T1w/T2w.nii.gz and T2w/T1w.nii.gz? Do the 3 calls even need 3 separate input folders (with currently the same initial contents)? Why are there 3 hippunfold calls anyway?

This is all necessary for the BIDS wrapping, if we can get working direct input paths, I would not do any of this. There are 3 HippUnfold calls because there are 3 ways to call HippUnfold currently. I am hopeful we could standardize on using both the T1w and T2w in HippUnfold 2.0, but we need to hear from the HippUnfold folks that it is better or at least not worse than using individual inputs.

coalsont · 2025-01-10T03:40:13Z

HippUnfoldHCP/PostHippUnfoldHCP.sh

+if [ ${Flag} = "On" ] ; then
+  Modality="T1wT2w"
+fi


Would be simpler and less error-prone to declare the variables local at the start of the function (if you feel the need to reuse the variable names)...

I think I tried that first and it didn't work. You are welcome to make a commit that you think will work and we can test it.

coalsont · 2025-01-10T03:40:47Z

HippUnfoldHCP/PostHippUnfoldHCP.sh

+function PostHippUnfold {
+HippUnfoldFolderOut=${1}


I would indent the entire function body to make it obvious where it ends.

Also, there is only one line in the code (in a loop) that calls this function. Functions are mainly for when you want to call it from more than one line in the code (such as when you have to change the arguments in a case-specific way).

That is in fact why it is a function, there are a bunch of variables that change depending upon which HippUnfold output is being used. If we standardized on only one kind of HippUnfold inputs (e.g., both T1w and T2w) then I wouldn't need the function.

As it stands it is most of the script and I didn't want to waste the space. The function may not be needed in the end if we standardize on only one kind of HippUnfold inputs.

coalsont · 2025-01-10T03:54:03Z

HippUnfoldHCP/PostHippUnfoldHCP.sh

+function PALETTE {
+File=${1}


Probably better to define this function outside the body of the other function. Nesting like this doesn't remove the function once the outer function ends (so it doesn't have the usual benefits of nested functions), but it does redundantly redefine it each call. It also increases the distance between the start and end of the outer function.

Also, the body isn't indented relative to the context. I don't want matlab-style indentation to leak outside of matlab (I don't even like it in matlab...).

I don't think I was clear on what was inside or outside a function's scope, so I put things inside just to be sure. Let's see what happens with the HippUnfold inputs...

HippUnfoldHCP/PostHippUnfoldHCP.sh

coalsont · 2025-01-10T04:12:06Z

HippUnfoldHCP/PostHippUnfoldHCP.sh

+if [ ${Modality} = "T1wT2w" ] ; then
+  Modality="T1w"
+  Flag="On"
+else
+  Flag="Off"
+fi


You could also consider this pattern:

Suggested change

if [ ${Modality} = "T1wT2w" ] ; then

Modality="T1w"

Flag="On"

else

Flag="Off"

fi

EffectiveModality="$Modality"

if [ ${Modality} = "T1wT2w" ] ; then

EffectiveModality="T1w"

fi

I think this issue wouldn't have come up if the local variables worked, but they didn't as I recall.

Co-authored-by: Tim Coalson <[email protected]>

glasserm added 7 commits December 7, 2024 19:11

*Initial Commit of HippUnfoldHCP and PostHippUnfoldHCP, tested but pr…

a50375f

…eliminary and is CERTAIN TO CHANGE including file organization given additional Connectome Workbench features needed, HippUnfold bugs discovered and further development and optimization needed.

*Add Apptainer/Singularity Cache control

8d8c3ca

*Add Support for multi-resolution meshes *Add Spec Files *Add Support for Dentate in cifti-matlab

*Make vertex areas specific to the volume space they are located in

b279be6

*Combine hipp and dentate in CIFTI files

30356e7

*Support reading CIFTI files with dentate in cifti-matlab *Use consistent hippocampal labels in CIFTI and volume *Code cleanup *Remove matlab hack and just recreate all CIFTI files

*Remove matlab hack code

6a3f00b

*Add inner and outer surfaces

10238bf

*Add some combined hippocampal and cerebral spec files

*Remove absolute paths

3d320b7

glasserm requested review from sivcek, coalsont, adeyemob, mharms and demsarjure January 8, 2025 03:58

demsarjure reviewed Jan 8, 2025

View reviewed changes

coalsont reviewed Jan 10, 2025

View reviewed changes

HippUnfoldHCP/PostHippUnfoldHCP.sh Outdated Show resolved Hide resolved

coalsont reviewed Jan 10, 2025

View reviewed changes

HippUnfoldHCP/PostHippUnfoldHCP.sh Outdated Show resolved Hide resolved

coalsont reviewed Jan 10, 2025

View reviewed changes

glasserm and others added 2 commits January 11, 2025 10:49

*math doesn't require the Var to be used

03a4bc6

Co-authored-by: Tim Coalson <[email protected]>

*math allows Var to be used without modification.

8b1c465

Co-authored-by: Tim Coalson <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HCP HippUnfold and Post HippUnfold Implementations #320

HCP HippUnfold and Post HippUnfold Implementations #320

glasserm commented Jan 8, 2025 •

edited

Loading

demsarjure left a comment

demsarjure Jan 8, 2025

coalsont Jan 8, 2025 •

edited

Loading

glasserm Jan 11, 2025

demsarjure Jan 8, 2025

coalsont Jan 10, 2025

jordandekraker Jan 10, 2025

demsarjure Jan 10, 2025

jordandekraker Jan 10, 2025

glasserm Jan 11, 2025

glasserm Jan 11, 2025 •

edited

Loading

demsarjure Jan 8, 2025

glasserm Jan 11, 2025

demsarjure Jan 8, 2025

glasserm Jan 11, 2025

demsarjure Jan 8, 2025

glasserm Jan 11, 2025

demsarjure Jan 8, 2025

glasserm Jan 11, 2025

coalsont Jan 10, 2025

glasserm Jan 11, 2025

coalsont Jan 10, 2025

glasserm Jan 11, 2025

coalsont Jan 10, 2025 •

edited

Loading

coalsont Jan 10, 2025

glasserm Jan 11, 2025

coalsont Jan 10, 2025

glasserm Jan 11, 2025

coalsont Jan 10, 2025

glasserm Jan 11, 2025

HCP HippUnfold and Post HippUnfold Implementations #320

Are you sure you want to change the base?

HCP HippUnfold and Post HippUnfold Implementations #320

Conversation

glasserm commented Jan 8, 2025 • edited Loading

demsarjure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coalsont Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasserm Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coalsont Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasserm commented Jan 8, 2025 •

edited

Loading

coalsont Jan 8, 2025 •

edited

Loading

glasserm Jan 11, 2025 •

edited

Loading

coalsont Jan 10, 2025 •

edited

Loading