Skip to content

Add hippocampal fMRISurface processing - #412

Open
orenpoliva wants to merge 7 commits into
Washington-University:masterfrom
orenpoliva:oren-hippocampal-fmri-master
Open

orenpoliva wants to merge 7 commits into
Washington-University:masterfrom
orenpoliva:oren-hippocampal-fmri-master

Conversation

@orenpoliva

@orenpoliva orenpoliva commented Aug 26, 2026

Copy link
Copy Markdown

fMRI pipeline changes:

GenericHippocampusfMRISurfaceProcessingPipeline.sh:

Re-enabled pipedirguessed=1.

Removed the output-folder and fmri-qc options.

Added the --resample-mesh option. A validation loop was also added to allow only valid mesh values (512, 2k, 8k, 18k). The selected meshes are then passed to the child script HippocampalVolumeToSurfaceMapping.sh.
Moved surface smoothing into a separate child script, HippocampalSurfaceSmoothing.sh.
Moved CIFTI generation into a separate child script, HippocampalCreateCIFTIs.sh.

Shifted to a more compact formatting style, including reducing unnecessary use of line-continuation backslashes.
Removed the variable previously used for wb_command and now call wb_command directly.

The native mesh is added internally to the requested resampling meshes so that native-space processing is always performed before resampling to the user-requested meshes.

HippocampalVolumeToSurfaceMapping.sh:

The script now receives the mesh list as an input parameter.

Similar to its cortical counterpart, RibbonVolumeToSurfaceMapping.sh, intermediate volumetric files are generated inside the HippocampalVolumeToSurfaceMapping working subdirectory. Final surface-related files are generated in the parent fMRI results directory (e.g., rfMRI_REST1_LR).

Previously, generation of the fMRI surface time series and VN surface files was intermixed. These are now handled in separate processing loops.

VN is mapped once from the volume to the native hippocampal surface and then resampled from native to the requested meshes.

Shifted to a more compact formatting style, including reducing unnecessary use of line-continuation backslashes.
Removed the variable previously used for wb_command and now call wb_command directly.

Questions:

  1. The HippocampalVolumeToSurfaceMapping working-directory name is currently hardcoded in both the parent and child scripts. Should the working-directory path instead be passed from the parent to the child as an input parameter?

  2. If the long-term plan is to combine the cortical and hippocampal fMRISurface pipelines, would it make more sense to begin integrating them now rather than maintaining two parallel pipelines?

  3. There is some inconsistency in how wb_command is called across HCP scripts. For example, some scripts, such as RSNRegression.sh, use wb_command, while others, such as SurfaceSmoothing.sh, use ${CARET7DIR}/wb_command. Which format should I use?

Comment on lines +116 to +120
"${WB}" -metric-math \
'1' \
"${OnesMetric}" \
-var x \
"${ThicknessMetric}"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For math commands, I typically use a more compact style due to the expected compactness of the variable names and the expression. Also, wb_command is on $PATH, we don't need a variable in the way.

Suggested change
"${WB}" -metric-math \
'1' \
"${OnesMetric}" \
-var x \
"${ThicknessMetric}"
wb_command -metric-math '1' "${OnesMetric}" \
-var x "${ThicknessMetric}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a variable for wb_command would allow someone to specify a particular version of wb_command (e.g., in a CHPC-like environment). I usually have had a variable for that reason.

@coalsont coalsont Sep 1, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing $PATH to point to it would do that just fine. There is also a place to do this in SetUpHCPPipeline.sh, which results in it being in $PATH. As currently written, it is hardcoded to use $CARET7DIR, which is the same as what SetUp... puts on $PATH anyway.


opts_AddOptional '--goodvoxel' 'doGoodVoxels' 'YES OR NO' "Controls whether to do goodVoxel procedure (default = YES)" "YES"

opts_AddOptional '--factor' 'factor' 'number' "Scaling factor for eliminating high COV voxels"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably have a default value.

Single stage with...apparently half a standard deviation threshold may have been chosen for cortex due to substantial outlier effects (multi-stage and/or dilation to isolate partial voxel effects would probably be a better approach). The single stage approach may work particularly poorly with a much smaller ROI like hippocampus.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Oren did an analysis to figure this out, so we should use that as the default.

"${CARET7DIR}/wb_command" -cifti-create-dense-timeseries \
"${ResultsFolder}/${NameOffMRI}_AtlasHipp_${ProcString}.${Mesh}.dtseries.nii" \
-metric HIPPOCAMPUS_LEFT "${WorkingDirectory}/${Subject}.L.hipp_fMRI_s${SmoothingFWHM}.${Mesh}.func.gii" \
-roi "${WorkingDirectory}/${Subject}.L.hipp_ones.${Mesh}.func.gii" \

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If these are all-ones files, you don't need to use -roi at all. The reason the -roi option exists is because the cortical surfaces are topologically spheres, even though some of the sphere vertices don't actually represent cortex.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree.

MidSurface="${HippUnfoldFolder}/native/${Prefix}_midthickness.native.surf.gii"
OuterSurface="${HippUnfoldFolder}/native/${Prefix}_outer.native.surf.gii"

OnesMetric="${WorkingDirectory}/${Prefix}_ones.native.func.gii"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to keep any all-ones metrics around, I would use tempfiles.shlib to handle it instead (in the /tmp folder or wherever the standardized environment variables point), like this:

tempfiles_create volToCifti_toSurf_XXXXXX.func.gii tempgii

A file created this way gets deleted when the script exits, without any further code.


opts_AddMandatory '--smoothingFWHM' 'SmoothingFWHM' 'number' 'smoothing FWHM (mm)'

opts_AddOptional '--fmri-qc' 'QCMode' 'YES OR NO OR ONLY' "Controls whether to generate a QC scene and snapshots (default=YES). ONLY executes just the QC script." "YES"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this implemented? If not, it should be taken out.


opts_AddOptional '--fmri-qc' 'QCMode' 'YES OR NO OR ONLY' "Controls whether to generate a QC scene and snapshots (default=YES). ONLY executes just the QC script." "YES"

opts_AddOptional '--output-directory' 'OutputDirectory' 'path' 'Directory where pipeline outputs will be written' ""

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
opts_AddOptional '--output-directory' 'OutputDirectory' 'path' 'Directory where pipeline outputs will be written' ""
opts_AddOptional '--output-directory' 'OutputDirectory' 'path' 'Directory where pipeline outputs will be written for testing purposes' ""

Comment on lines +79 to +97
QCMode="$(echo "${QCMode}" | tr '[:upper:]' '[:lower:]')"

doProcessing=1
doQC=1

case "${QCMode}" in
yes)
;;
no)
doQC=0
;;
only)
doProcessing=0
log_Warn "Only generating fMRI QC scene and snapshots from existing data"
;;
*)
log_Err_Abort "Unrecognized value '${QCMode}' for --fmri-qc; use YES, NO, or ONLY"
;;
esac

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove if we are not doing this.

Comment thread fMRISurface/GenericHippocampusfMRISurfaceProcessingPipeline.sh Outdated
Comment on lines +111 to +115
if [ -n "${OutputDirectory}" ]; then
ResultsFolder="${OutputDirectory}/${Subject}/${NameOffMRI}"
else
ResultsFolder="${AtlasSpaceFolder}/${ResultsFolderName}/${NameOffMRI}"
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems specific to your testing.

Comment on lines +158 to +182
#Surface Smoothing
# ------------------------------------------------------------------------------
# Hippocampal Surface Smoothing
# ------------------------------------------------------------------------------

log_Msg "Hippocampal Surface Smoothing"

Sigma=`echo "$SmoothingFWHM / (2 * sqrt(2 * l(2)))" | bc -l`

Meshes=(native 512 2k 8k 18k)
Structures=(hipp dentate)

for Mesh in "${Meshes[@]}"; do
for Structure in "${Structures[@]}"; do
for Hemisphere in L R; do
"${CARET7DIR}/wb_command" -metric-smoothing \
"${HippUnfoldFolder}/${Mesh}/${Subject}.${Hemisphere}.${Structure}_midthickness.${Mesh}.surf.gii" \
"${WorkingDirectory}/${Subject}.${Hemisphere}.${Structure}_fMRI.${Mesh}.func.gii" \
"${Sigma}" \
"${WorkingDirectory}/${Subject}.${Hemisphere}.${Structure}_fMRI_s${SmoothingFWHM}.${Mesh}.func.gii" \
-roi "${WorkingDirectory}/${Subject}.${Hemisphere}.${Structure}_ones.${Mesh}.func.gii"
done
done
done
# ------------------------------------------------------------------------------

@glasserm glasserm Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will need to be moved into a module in the future for parallelism with fMRISurface (and eventual inclusion in fMRISurface). If you insist on keeping it in the main pipeline, please put it in a function with the appropriate I/O so that it can easily be copied into the appropriate fMRISurface module when the time comes.

Comment on lines +183 to +199
# Combining ROIs into a single CIFTI file
# ------------------------------------------------------------------------------


for Mesh in ${Meshes[@]}; do
"${CARET7DIR}/wb_command" -cifti-create-dense-timeseries \
"${ResultsFolder}/${NameOffMRI}_AtlasHipp_${ProcString}.${Mesh}.dtseries.nii" \
-metric HIPPOCAMPUS_LEFT "${WorkingDirectory}/${Subject}.L.hipp_fMRI_s${SmoothingFWHM}.${Mesh}.func.gii" \
-roi "${WorkingDirectory}/${Subject}.L.hipp_ones.${Mesh}.func.gii" \
-metric HIPPOCAMPUS_RIGHT "${WorkingDirectory}/${Subject}.R.hipp_fMRI_s${SmoothingFWHM}.${Mesh}.func.gii" \
-roi "${WorkingDirectory}/${Subject}.R.hipp_ones.${Mesh}.func.gii" \
-metric HIPPOCAMPUS_DENTATE_LEFT "${WorkingDirectory}/${Subject}.L.dentate_fMRI_s${SmoothingFWHM}.${Mesh}.func.gii" \
-roi "${WorkingDirectory}/${Subject}.L.dentate_ones.${Mesh}.func.gii" \
-metric HIPPOCAMPUS_DENTATE_RIGHT "${WorkingDirectory}/${Subject}.R.dentate_fMRI_s${SmoothingFWHM}.${Mesh}.func.gii" \
-roi "${WorkingDirectory}/${Subject}.R.dentate_ones.${Mesh}.func.gii"

log_Msg "Generated fMRI time series Cifti file: "${ResultsFolder}/${Subject}.${NameOffMRI}"_AtlasHipp_${ProcString}.${Mesh}.dtseries.nii"

@glasserm glasserm Aug 31, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will also need to be moved into a module for parallelism with fMRISurface and eventual folding into fMRI surface. If you insist on keeping it in the main pipeline, please put it in a function with the appropriate I/O so that it can easily be copied into the appropriate fMRISurface module when the time comes.

Comment on lines +228 to +235
# if ((doQC)); then
# log_Msg "Generating fMRI QC scene and snapshots"
# "${PipelineScripts}/GenerateFMRIScenes.sh" \
# --study-folder="${Path}" \
# --subject="${Subject}" \
# --fmriname="${NameOffMRI}${ProcString}" \
# --output-folder="${ResultsFolder}/fMRIQC"
# fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delete if not doing.

Comment on lines +225 to +226
#Uncomment if want to remove the subfoler with intermediate files
#rm -rf "${WorkingDirectory}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems dangerous.

NeighborhoodSmoothing="5" # Distinguishes large vs small dropout
dilation_dist="10"

Meshes=(512 2k 8k 18k)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to run this on all meshes. Just the one we choose. Mesh density should be an input parameter to the overall pipeline.

Comment on lines +101 to +105
ThicknessMetric="${HippUnfoldFolder}/native/${Prefix}_thickness.native.shape.gii"

InnerSurface="${HippUnfoldFolder}/native/${Prefix}_inner.native.surf.gii"
MidSurface="${HippUnfoldFolder}/native/${Prefix}_midthickness.native.surf.gii"
OuterSurface="${HippUnfoldFolder}/native/${Prefix}_outer.native.surf.gii"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Native should be capitalized.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Native should be capitalized - are you referring to folder name or substring in filename, or both?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Folder.

fi


NativeFolder="${HippUnfoldFolder}/native"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Native again here.

Comment on lines +469 to +553
# =====================================================================
# Map VN volume to native hippocampal surface
# =====================================================================

VolumefMRIVN="${VolumefMRI}_vn.nii.gz"
NativefMRIVN="${WorkingDirectory}/${Prefix}_fMRI_vn.native.func.gii"

"${WB}" -volume-to-surface-mapping \
"${VolumefMRIVN}" \
"${MidSurface}" \
"${NativefMRIVN}" \
-cubic

"${WB}" -metric-mask \
"${NativefMRIVN}" \
"${NativeROI}" \
"${NativefMRIVN}"

log_Msg "Generated VN file in native space at: ${NativefMRIVN}"

# =====================================================================
# Resample complete fMRI timeseries to all densities
# =====================================================================

for Mesh in ${Meshes[@]}; do

MeshFolder="${HippUnfoldFolder}/${Mesh}"

TargetFlat="${MeshFolder}/${Prefix}_flat.${Mesh}.surf.gii"
TargetMidSurface="${MeshFolder}/${Prefix}_midthickness.${Mesh}.surf.gii"
TargetROI="${WorkingDirectory}/${Prefix}_ones.${Mesh}.func.gii"

TargetfMRI="${WorkingDirectory}/${Prefix}_fMRI.${Mesh}.func.gii"


"${WB}" -metric-resample \
"${NativefMRI}" \
"${NativeFlat}" \
"${TargetFlat}" \
ADAP_BARY_AREA \
"${TargetfMRI}" \
-area-surfs \
"${MidSurface}" \
"${TargetMidSurface}" \
-current-roi "${NativeROI}" \
-bypass-sphere-check


"${WB}" -metric-dilate \
"${TargetfMRI}" \
"${TargetMidSurface}" \
${dilation_dist} \
"${TargetfMRI}" \
-nearest


"${WB}" -metric-mask \
"${TargetfMRI}" \
"${TargetROI}" \
"${TargetfMRI}"


log_Msg "Generated fMRI file in ${Mesh} mesh at: ${TargetfMRI}"

# =====================================================================
# Map VN volume to native hippocampal surface
# =====================================================================

VolumefMRIVN="${VolumefMRI}_vn.nii.gz"
TargetfMRIVN="${WorkingDirectory}/${Prefix}_fMRI_vn.${Mesh}.func.gii"

"${WB}" -volume-to-surface-mapping \
"${VolumefMRIVN}" \
"${TargetMidSurface}" \
"${TargetfMRIVN}" \
-cubic

"${WB}" -metric-mask \
"${TargetfMRIVN}" \
"${TargetROI}" \
"${TargetfMRIVN}"

log_Msg "Generated VN file in ${Mesh} mesh at: ${TargetfMRIVN}"
done
done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is disorganized. The VN stuff should all be in a separate section by itself since it is a hack that will be removed in the future.

@glasserm glasserm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

151 lines became over 500 lines of code. I find this very hard to follow. Can we reduce the extent of the excessive carriage returns?

Also I will need to see a test dataset.

@orenpoliva

Copy link
Copy Markdown
Author

fMRI pipeline changes:

GenericHippocampusfMRISurfaceProcessingPipeline.sh:

Re-enabled pipedirguessed=1.

Removed the output-folder and fmri-qc options.

Added the --resample-mesh option. A validation loop was also added to allow only valid mesh values (512, 2k, 8k, 18k). The selected meshes are then passed to the child script HippocampalVolumeToSurfaceMapping.sh.
Moved surface smoothing into a separate child script, HippocampalSurfaceSmoothing.sh.
Moved CIFTI generation into a separate child script, HippocampalCreateCIFTIs.sh.

Shifted to a more compact formatting style, including reducing unnecessary use of line-continuation backslashes.
Removed the variable previously used for wb_command and now call wb_command directly.

The native mesh is added internally to the requested resampling meshes so that native-space processing is always performed before resampling to the user-requested meshes.

HippocampalVolumeToSurfaceMapping.sh:

The script now receives the mesh list as an input parameter.

Similar to its cortical counterpart, RibbonVolumeToSurfaceMapping.sh, intermediate volumetric files are generated inside the HippocampalVolumeToSurfaceMapping working subdirectory. Final surface-related files are generated in the parent fMRI results directory (e.g., rfMRI_REST1_LR).

Previously, generation of the fMRI surface time series and VN surface files was intermixed. These are now handled in separate processing loops.

VN is mapped once from the volume to the native hippocampal surface and then resampled from native to the requested meshes.

Shifted to a more compact formatting style, including reducing unnecessary use of line-continuation backslashes.
Removed the variable previously used for wb_command and now call wb_command directly.

Questions:

  1. The HippocampalVolumeToSurfaceMapping working-directory name is currently hardcoded in both the parent and child scripts. Should the working-directory path instead be passed from the parent to the child as an input parameter?

  2. If the long-term plan is to combine the cortical and hippocampal fMRISurface pipelines, would it make more sense to begin integrating them now rather than maintaining two parallel pipelines?

  3. There is some inconsistency in how wb_command is called across HCP scripts. For example, some scripts, such as RSNRegression.sh, use wb_command, while others, such as SurfaceSmoothing.sh, use ${CARET7DIR}/wb_command. Which format should I use?

@orenpoliva

Copy link
Copy Markdown
Author

151 lines became over 500 lines of code. I find this very hard to follow. Can we reduce the extent of the excessive carriage returns?

Also I will need to see a test dataset.

@orenpoliva

Copy link
Copy Markdown
Author

A path to a test dataset is:
This path link to final files dtseries.nii and _vn.dscalar.nii
/media/myelin/brainmappers/Connectome_Project/YA_HCP_Final/103818/MNINonLinear/Results/rfMRI_REST1_LR
See also subfolder for intermediate files:
/media/myelin/brainmappers/Connectome_Project/YA_HCP_Final/103818/MNINonLinear/Results/rfMRI_REST1_LR/HippocampalVolumeToSurfaceMapping

Script was modified to reduce in size

@coalsont

coalsont commented Sep 3, 2026

Copy link
Copy Markdown
Member
  1. The HippocampalVolumeToSurfaceMapping working-directory name is currently hardcoded in both the parent and child scripts. Should the working-directory path instead be passed from the parent to the child as an input parameter?

Defining the folder name only once would be better, yes. Hardcoding it in the top-level script is still good, in case we have a need for the intermediates later.

  1. There is some inconsistency in how wb_command is called across HCP scripts. For example, some scripts, such as RSNRegression.sh, use wb_command, while others, such as SurfaceSmoothing.sh, use ${CARET7DIR}/wb_command. Which format should I use?

I use (and recommend) bare wb_command (and fsl commands, too), because it is more readable and has no substantial disadvantages (as I understand it, fsl uses $FSLDIR in the scripts in their installation directory specifically in case the user has called a version of an fsl tool that isn't the first one on $PATH, but we control $PATH in our SetUpHCP... script, and our scripts aren't part of an FSL installation, so that situation doesn't apply to us). @glasserm does other things, partly because he made a wb_command_retry script long ago, but that can be done by adding a special "retry script" folder to the front of $PATH instead of using a different executable name.

Comment on lines +26 to +28
#comment the line back in when done
#source "${HCPPIPEDIR}/global/scripts/debug.shlib" "$@" # Debugging functions; also sources log.shlib
source "${HCPPIPEDIR}/global/scripts/log.shlib" "$@" # Debugging functions; also sources log.shlib

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it runs with set -eu, then we should be sourcing debug.shlib. I'm not sure what commenting it out was intended to accomplish, as it should merely give more information than set -eu, but otherwise similar "exit on error" effects.

Suggested change
#comment the line back in when done
#source "${HCPPIPEDIR}/global/scripts/debug.shlib" "$@" # Debugging functions; also sources log.shlib
source "${HCPPIPEDIR}/global/scripts/log.shlib" "$@" # Debugging functions; also sources log.shlib
source "${HCPPIPEDIR}/global/scripts/debug.shlib" "$@" # Debugging functions; also sources log.shlib

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that. I'm writing my code on vs code using bashdb debugger. This specific script crashes my debug. I was planning to uncomment before committing to the PR.

Comment on lines +7 to +17
########################################## PIPELINE OVERVIEW ##########################################

# TODO

########################################## OUTPUT DIRECTORIES ##########################################

# TODO

################################################ SUPPORT FUNCTIONS ##################################################

set -eu

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Redundant set -eu. Also, if we aren't going to fill these documentation sections in, I wouldn't bother having them (yet).

Suggested change
########################################## PIPELINE OVERVIEW ##########################################
# TODO
########################################## OUTPUT DIRECTORIES ##########################################
# TODO
################################################ SUPPORT FUNCTIONS ##################################################
set -eu

Comment on lines +3 to +5
# Requirements for this script
# installed versions of: FSL, Connectome Workbench (wb_command)
# environment: HCPPIPEDIR, FSLDIR, CARET7DIR

@coalsont coalsont Sep 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure how useful this list is, as all of these are always set up by SetUpHCP....

Comment thread fMRISurface/scripts/HippocampalSmoothing.sh Outdated
Comment thread fMRISurface/scripts/CreateHippocampalCIFTIs.sh Outdated
@@ -0,0 +1,74 @@
#!/bin/bash
set -eu
# --------------------------------------------------------------------------------

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# --------------------------------------------------------------------------------
#based on fMRISurface/scripts/SurfaceSmoothing.sh
# --------------------------------------------------------------------------------

@@ -0,0 +1,82 @@
#!/bin/bash
set -eu
# --------------------------------------------------------------------------------

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# --------------------------------------------------------------------------------
#similar to fMRISurface/scripts/CreateDenseTimeseries.sh
# --------------------------------------------------------------------------------

@glasserm

glasserm commented Sep 5, 2026

Copy link
Copy Markdown
Contributor
  1. The HippocampalVolumeToSurfaceMapping working-directory name is currently hardcoded in both the parent and child scripts. Should the working-directory path instead be passed from the parent to the child as an input parameter?

Defining the folder name only once would be better, yes. Hardcoding it in the top-level script is still good, in case we have a need for the intermediates later.

  1. There is some inconsistency in how wb_command is called across HCP scripts. For example, some scripts, such as RSNRegression.sh, use wb_command, while others, such as SurfaceSmoothing.sh, use ${CARET7DIR}/wb_command. Which format should I use?

I use (and recommend) bare wb_command (and fsl commands, too), because it is more readable and has no substantial disadvantages (as I understand it, fsl uses $FSLDIR in the scripts in their installation directory specifically in case the user has called a version of an fsl tool that isn't the first one on $PATH, but we control $PATH in our SetUpHCP... script, and our scripts aren't part of an FSL installation, so that situation doesn't apply to us). @glasserm does other things, partly because he made a wb_command_retry script long ago, but that can be done by adding a special "retry script" folder to the front of $PATH instead of using a different executable name.

Okay if you think that we don't need to specify the wb_command location even for the user who wants to run a non-default wb_command, then we don't have to keep using it.

@orenpoliva

orenpoliva commented Sep 8, 2026

Copy link
Copy Markdown
Author

fMRI Pipeline
Matt - per your request, all .func.gii files were merged into CIFTI files and afterward deleted. The final dtseries files are now saved directly in the main fMRI results directory (e.g., MNINonLinear/Results/rfMRI_REST1_LR), and intermediates files into the HippocampalVolumeToSurfaceMapping subdirectory. These changes were incorporated into CreateHippocampalCIFTIs.sh.
Question: Is this a correct division of files into the folders? Do you want all dscalar.nii files in the main fMRI results directory?

Updated the scripts to read data from the ‘Native’ directory instead of the previous ‘native' directory.

Removed remnants of previous analyses from the test subjects' fMRI results directories so that they now contain outputs generated by the current version of the pipeline.

Two datasets are available for review:
/media/myelin/brainmappers/Connectome_Project/YA_HCP_Final/103818/MNINonLinear/Results/rfMRI_REST1_LR

/media/myelin/brainmappers/Connectome_Project/YA_HCP_Final/105923/MNINonLinear/Results/rfMRI_REST1_LR

fi

source "${HCPPIPEDIR}/global/scripts/debug.shlib" "$@" # Debugging functions; also sources log.shlib
#source "${HCPPIPEDIR}/global/scripts/log.shlib" "$@" # Debugging functions; also sources log.shlib

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#source "${HCPPIPEDIR}/global/scripts/log.shlib" "$@" # Debugging functions; also sources log.shlib

Or at least remove the incorrect comment on the end.

Alternatively, it shouldn't actually be harmful to source log.shlib again (or before debug.shlib actually, since debug.shlib actually checks whether log.shlib has already been sourced), if we want to support the type of debugging that is improved by commenting out the debug line.

Comment on lines +37 to +49
################################################ SUPPORT FUNCTIONS ##################################################
set -eu
pipedirguessed=0
if [[ "${HCPPIPEDIR:-}" == "" ]]
then
pipedirguessed=1
# Fix this if the script is more than one level below HCPPIPEDIR
export HCPPIPEDIR="$(dirname -- "$0")/.."
fi

source "${HCPPIPEDIR}/global/scripts/debug.shlib" "$@" # Debugging functions; also sources log.shlib
source "${HCPPIPEDIR}/global/scripts/log.shlib" "$@" # Debugging functions; also sources log.shlib

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this entire file indented now? This has made some of my previous comments no longer show up on the diff ("files") page (github thinks they are outdated because the lines were edited).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reformatted the indentation because I understood your earlier feedback to mean that all of my scripts should follow the same style, specifically four-space indentation. This script had previously been using two-space indentation. I did not realize that reformatting the entire file would cause some of your earlier comments to appear outdated in GitHub. I’ll avoid broad formatting changes like this in future revisions so that existing review comments and the substantive diff remain easier to track.

@coalsont coalsont Sep 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify, by "entire file indented", I meant that after this change, there are now 4 spaces before even the first line of the script. Anything that isn't in any conditional/loop/function/etc context should have zero spaces/tabs before the first character. Concretely:

Suggested change
################################################ SUPPORT FUNCTIONS ##################################################
set -eu
pipedirguessed=0
if [[ "${HCPPIPEDIR:-}" == "" ]]
then
pipedirguessed=1
# Fix this if the script is more than one level below HCPPIPEDIR
export HCPPIPEDIR="$(dirname -- "$0")/.."
fi
source "${HCPPIPEDIR}/global/scripts/debug.shlib" "$@" # Debugging functions; also sources log.shlib
source "${HCPPIPEDIR}/global/scripts/log.shlib" "$@" # Debugging functions; also sources log.shlib
################################################ SUPPORT FUNCTIONS ##################################################
set -eu
pipedirguessed=0
if [[ "${HCPPIPEDIR:-}" == "" ]]
then
pipedirguessed=1
# Fix this if the script is more than one level below HCPPIPEDIR
export HCPPIPEDIR="$(dirname -- "$0")/.."
fi
source "${HCPPIPEDIR}/global/scripts/debug.shlib" "$@" # Debugging functions; also sources log.shlib
source "${HCPPIPEDIR}/global/scripts/log.shlib" "$@" # Debugging functions; also sources log.shlib

I prefer four spaces per indentation level, so in any new scripts that would be appreciated. In existing scripts, following whatever convention is already present (as long as it is consistent) means that the per-line commit history remains more useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants