Modifications to generic fMRIVolume and fMRISurface pipelines to run on longitudinal sessions. #319

mmilch01 · 2024-12-29T00:12:51Z

This pull request comprises modifications to generic fMRIVolume and fMRISurface to run on longitudinal timepoints. Prerequisites for GenericfMRIVolumeProcessingPipeline are:

All cross-sectional pipelines must have been run up to post-Freesurfer for all timepoints.
All longitudinal pipelines for the corresponding timepoints must have been run up to post-Freesurfer.
GenericfMRIVolumeProcessingPipeline must have been run on all cross-sectional timepoints.
All timepoints must be under the same processing directory.

Prerequisites for fMRISurface are the same plus GenericfMRIVolumeProcessingPipeline must have been run on all longitudinal timepoints.

Note that neither multi-echo fMRI nor fMRIReference is currently supported in longitudinal mode.

demsarjure · 2024-12-30T09:42:23Z

@mmilch01

I assume that in

Prerequisites for fMRISurface are the same plus GenericfMRIVolumeProcessingPipeline must have been run on all longitudinal timepoints.

GenericfMRIVolumeProcessingPipeline should be GenericfMRISurfaceProcessingPipeline?

It would be probably great, if I ran some tests for this on our end. Misha, can you please paste 2 example calls that you used for tests on your end, one for long volume and one for long surface. Just so I have something to quickly compare my command calls with. Thanks!

demsarjure · 2024-12-30T09:50:29Z

It looks like this MR now includes changes from Volume + Surface and also changes from the long FS which were already merged into master? So the review tab shows a lot of code that is already tested and reviewed. Maybe it would be best to have a new branch for the new stuff?

Also, the branch is not in sync with the current master, so it would be best to merge the current master into this and resolve the conflicts. Or the cleanest way, which is to create new branch of the master and apply only long volume and long surface changes to it.

demsarjure

I did a somewhat quick review and things look good. As usual I might report some bugs later on, when I actually start running tests.

demsarjure · 2024-12-30T09:52:15Z

fMRIVolume/GenericfMRIVolumeProcessingPipeline-0.sh

What is the -0 suffix in GenericfMRIVolumeProcessingPipeline-0.sh?

What is the -0 suffix in GenericfMRIVolumeProcessingPipeline-0.sh?
That's a backup file that somehow made it into one of commits. should be fixed now.

fMRIVolume/GenericfMRIVolumeProcessingPipeline.sh

coalsont · 2024-12-30T21:54:58Z

It looks like this MR now includes changes from Volume + Surface and also changes from the long FS which were already merged into master? So the review tab shows a lot of code that is already tested and reviewed. Maybe it would be best to have a new branch for the new stuff?

Also, the branch is not in sync with the current master, so it would be best to merge the current master into this and resolve the conflicts. Or the cleanest way, which is to create new branch of the master and apply only long volume and long surface changes to it.

Rebase is a cleaner way to resolve normal conflicts (this situation isn't normal). Merge commits are weird and awkward, and hard to look at diffs of. "In sync" isn't really applicable here, the whole purpose of a PR is to propose changes, conflicts mean that someone else has also made changes to the same files (or, it can be a symptom that your branch's starting point is wrong or contains extra stuff).

I can try to salvage the intended commits.

+Work on longitudinal version of fMRIVolume instead.

…es to dependent scripts. +added options (not finished) to OneStepResampling to output in native space.

…on-University/HCPpipelines into feature/longitudinal_fs

+fMRIVolume, longitudinal version, finishes successfully. +Output not visually verified. +Example batch script for longitudinal fMRIVolume

+Rename 'Subject' to 'Session' in fMRISurfaceProcessingPipeline and dependencies

…ssions.

wq

mmilch01 · 2024-12-31T15:09:44Z

I assume that in
Prerequisites for fMRISurface are the same plus GenericfMRIVolumeProcessingPipeline must have been run on all longitudinal timepoints.
GenericfMRIVolumeProcessingPipeline should be GenericfMRISurfaceProcessingPipeline?

Longitudinal fMRISurface needs longitudinal fMRIVolume to have been run on the same timepoint. Cross-sectional fMRISurface is not required to have been run.

It would be probably great, if I ran some tests for this on our end. Misha, can you please paste 2 example calls that you used for tests on your end, one for long volume and one for long surface. Just so I have something to quickly compare my command calls with. Thanks!

You can use corresponding Examples/scripts/*-long.sh batch scripts for call examples. The only difference from my calls are obfuscated paths for StudyFolder and EnvironmentScript.

mmilch01 · 2024-12-31T15:10:45Z

It looks like this MR now includes changes from Volume + Surface and also changes from the long FS which were already merged into master? So the review tab shows a lot of code that is already tested and reviewed. Maybe it would be best to have a new branch for the new stuff?

Also, the branch is not in sync with the current master, so it would be best to merge the current master into this and resolve the conflicts. Or the cleanest way, which is to create new branch of the master and apply only long volume and long surface changes to it.

This should have been fixed now.

fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBRAndFreeSurferBBRbased.sh

glasserm · 2025-01-05T03:46:51Z

fMRIVolume/GenericfMRIVolumeProcessingPipeline.sh


-#Split echos
+#Split echos. 


Only run in cross-sectional.

Sorry, could you expand on what you mean?

You should not run this block in longitudinal mode, so please if/then it accordingly.

The block between lines (new) 683 and 809 is already only run in cross-sectional mode. Did you mean the next 'split echos' block? For that, I kept statements assigning [tcs/sct]Echoes[..] but put all wb_command commands under conditional.

At the moment, it will still print "splitting echoes" if multi echo is detected, even though it will then only set some shell variables. If we don't need any of the variables in longitudinal mode, I would put the longitudinal conditional around the entire block instead of buried in the loop, for readability (and avoiding wrong messages).

We do need tcsEchoes* to be set for multi-echo for both longitudinal and non-longitudinal modes. The if clause inside the "for iEcho.." loop prevents re-running wb_command in longitudinal mode.

fMRIVolume/GenericfMRIVolumeProcessingPipeline.sh

glasserm · 2025-01-05T03:48:25Z

fMRIVolume/GenericfMRIVolumeProcessingPipeline.sh

@@ -940,7 +1010,7 @@ then
            ${FSLDIR}/bin/fslmaths ${ResultsFolder}/${NameOffMRI}_pseudo_transmit_raw.nii.gz -mas ${fMRIFolder}/${FreeSurferBrainMask}.${FinalfMRIResolution}.nii.gz ${ResultsFolder}/${NameOffMRI}_pseudo_transmit_raw.nii.gz
            ${FSLDIR}/bin/applywarp --interp=trilinear -i ${DCFolder}/ComputeSpinEchoBiasField/${NameOffMRI}_pseudo_transmit_field.nii.gz -r ${fMRIFolder}/${NameOffMRI}_SBRef_nonlin -w ${AtlasSpaceFolder}/xfms/${AtlasTransform} -o ${ResultsFolder}/${NameOffMRI}_pseudo_transmit_field.nii.gz
            ${FSLDIR}/bin/fslmaths ${ResultsFolder}/${NameOffMRI}_pseudo_transmit_field.nii.gz -mas ${fMRIFolder}/${FreeSurferBrainMask}.${FinalfMRIResolution}.nii.gz ${ResultsFolder}/${NameOffMRI}_pseudo_transmit_field.nii.gz
-        else
+        else #never triggered in Longitudinal mode


@demsarjure @sivcek This needs your attention.

Like mentioned above, I think this is fine as reference is not needed any more in longitudinal processing.

glasserm

I have made various comments. Please address them and also @demsarjure @sivcek please assist with the fMRIReference stuff. Thanks!

+comment out 'output to native' code changes

fMRISurface/GenericfMRISurfaceProcessingPipeline.sh

fMRISurface/scripts/RibbonVolumeToSurfaceMapping.sh

fMRIVolume/GenericfMRIVolumeProcessingPipeline.sh

coalsont · 2025-01-06T20:53:23Z

fMRIVolume/GenericfMRIVolumeProcessingPipeline.sh

+
+if (( IsLongitudinal )); then
+    if [ ! -d "$Path/$SessionLong" ]; then
+        log_Err_Abort "the --longitudinal-session must be specified and folder must exist in longitudinal mode"


This suggests --longitudinal-session must be specified and non-empty whenever --is-longitudinal=TRUE. So, do we really even need the --is-longitudinal option, or should it simply test if --longitudinal-session gave an empty string?

Correct. But, I am inclined to leave is-longitudinal option as standard way to point to longitudinal session across scripts, instead of forcing the user to guess it is not required here.

coalsont · 2025-01-06T21:01:54Z

fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBRAndFreeSurferBBRbased.sh

+
+if (( $IsLongitudinal )); then
+    if [ ! -f "$T1wCross2LongXfm" ]; then
+        log_Err_Abort "--t1w-cross2long-xfm must point to a valid file when --is-longitudinal is used"


It could be argued that --is-longitudinal is redundant here also (you wouldn't supply --t1w-cross2long-xfm in cross-sectional mode).

Same as above, trying to be consistent across longitudinal /cross-sectional parameters

fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBRAndFreeSurferBBRbased.sh

glasserm · 2025-01-07T01:41:02Z

Since my attempt at e-mail-based comments didn't work, I deleted them and moved them to the appropriate threads.

Fix for missing long description Co-authored-by: Tim Coalson <[email protected]>

…on-University/HCPpipelines into feature/longitudinal_fs

fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBRAndFreeSurferBBRbased.sh

…AndFreeSurferBBRbased.sh Co-authored-by: Tim Coalson <[email protected]>

fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBRAndFreeSurferBBRbased.sh

fMRIVolume/scripts/OneStepResampling.sh

coalsont · 2025-01-22T15:36:42Z

fMRIVolume/scripts/OneStepResampling.sh

-FreeSurferBrainMaskFile=`basename "$FreeSurferBrainMask"`
+BiasFieldFile=$(basename "$BiasField")
+T1wImageFile=$(basename $T1wImage)
+#T1wImageFileNative=$(basename $T1wImageNative)


This line also looks like a native volume space change, commented out.

coalsont · 2025-01-22T15:40:56Z

fMRIVolume/GenericfMRIVolumeProcessingPipeline.sh

+		if (( ! IsLongitudinal )); then 
+			wb_command -volume-merge "${fMRIFolder}/${tcsEchoesOrig[iEcho]}.nii.gz" -volume "${fMRIFolder}/${OrigTCSName}.nii.gz" \
+				-subvolume $((1 + FramesPerEcho * iEcho)) -up-to $((FramesPerEcho * (iEcho + 1)))
+			wb_command -volume-merge "${fMRIFolder}/${sctEchoesOrig[iEcho]}.nii.gz" -volume "${fMRIFolder}/${OrigScoutName}.nii.gz" \


This section is using tabs when the surrounding is using spaces.

mmilch01 requested review from glasserm, coalsont, mharms and demsarjure December 29, 2024 00:13

demsarjure reviewed Dec 30, 2024

View reviewed changes

mmilch01 and others added 17 commits December 30, 2024 16:23

Longitudinal development - fMRIVOlumeProcessing pipeline in process

674d066

+Roll back changes in fMRIVolume.

f18eefa

+Work on longitudinal version of fMRIVolume instead.

+first pass of fMRIVolume longitudinal pipeline, with necessary chang…

d5a2fe4

…es to dependent scripts. +added options (not finished) to OneStepResampling to output in native space.

+function to create filewise symlinks at directory level

31e8418

+longitudinal modificatin - in progress

2699a6e

+sync with debug version

05044b5

+ in process

0298415

Merge branch 'feature/longitudinal_fs' of https://github.com/Washingt…

20fd71b

…on-University/HCPpipelines into feature/longitudinal_fs

Merge branch 'feature/longitudinal_fs' of https://github.com/Washingt…

67534ad

…on-University/HCPpipelines into feature/longitudinal_fs

+fix BiasField not being added in MNI space for longitudinal timepoints

c0ec85a

+longitudinal modification/debug in process

96e33e8

Merge branch 'feature/longitudinal_fs' of https://github.com/Washingt…

2f63e81

…on-University/HCPpipelines into feature/longitudinal_fs

+fMRIVolume longitudinal - stable version v. 0.1

cf25caa

+fMRIVolume, longitudinal version, finishes successfully. +Output not visually verified. +Example batch script for longitudinal fMRIVolume

+Remove filewise symlinking as unusable

53fdf69

+Rename 'Subject' to 'Session' in fMRISurfaceProcessingPipeline and dependencies

+batch script to run fMRISurfaceProcessingPipeline on longitudinal se…

0619cdc

…ssions.

+fix cp -rf syntax

ffe0873

wq

+remove obsolete GenericfMRIVolumeProcessingPipeline-long.sh

c8a2c88

coalsont force-pushed the feature/longitudinal_fs branch from 30fcc3c to c8a2c88 Compare December 30, 2024 22:44

+fix fMRI Distortion correction code that seems to have been duplicated

9316d4a

+remove test files

a0865f1

glasserm reviewed Jan 5, 2025

View reviewed changes

fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBRAndFreeSurferBBRbased.sh Outdated Show resolved Hide resolved

glasserm reviewed Jan 5, 2025

View reviewed changes

fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBRAndFreeSurferBBRbased.sh Outdated Show resolved Hide resolved

glasserm reviewed Jan 5, 2025

View reviewed changes

fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBRAndFreeSurferBBRbased.sh Outdated Show resolved Hide resolved

glasserm reviewed Jan 5, 2025

View reviewed changes

fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBRAndFreeSurferBBRbased.sh Outdated Show resolved Hide resolved

glasserm reviewed Jan 5, 2025

View reviewed changes

fMRIVolume/GenericfMRIVolumeProcessingPipeline.sh Outdated Show resolved Hide resolved

glasserm reviewed Jan 5, 2025

View reviewed changes

glasserm requested changes Jan 5, 2025

View reviewed changes

+address reviewer comments

cb1faef

+comment out 'output to native' code changes

coalsont reviewed Jan 6, 2025

View reviewed changes

fMRISurface/GenericfMRISurfaceProcessingPipeline.sh Outdated Show resolved Hide resolved

coalsont reviewed Jan 6, 2025

View reviewed changes

fMRISurface/GenericfMRISurfaceProcessingPipeline.sh Outdated Show resolved Hide resolved

coalsont reviewed Jan 6, 2025

View reviewed changes

fMRISurface/scripts/RibbonVolumeToSurfaceMapping.sh Show resolved Hide resolved

coalsont reviewed Jan 6, 2025

View reviewed changes

fMRIVolume/GenericfMRIVolumeProcessingPipeline.sh Outdated Show resolved Hide resolved

coalsont reviewed Jan 6, 2025

View reviewed changes

fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBRAndFreeSurferBBRbased.sh Outdated Show resolved Hide resolved

coalsont reviewed Jan 6, 2025

View reviewed changes

fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBRAndFreeSurferBBRbased.sh Outdated Show resolved Hide resolved

coalsont reviewed Jan 6, 2025

View reviewed changes

fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBRAndFreeSurferBBRbased.sh Show resolved Hide resolved

mmilch01 and others added 3 commits January 15, 2025 12:52

Update fMRISurface/GenericfMRISurfaceProcessingPipeline.sh

592e411

Fix for missing long description Co-authored-by: Tim Coalson <[email protected]>

+Addressing reviewer comments

841f4b6

Merge branch 'feature/longitudinal_fs' of https://github.com/Washingt…

1eb393f

…on-University/HCPpipelines into feature/longitudinal_fs

coalsont reviewed Jan 15, 2025

View reviewed changes

fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBRAndFreeSurferBBRbased.sh Outdated Show resolved Hide resolved

mmilch01 and others added 2 commits January 16, 2025 09:06

Update fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBR…

503255a

…AndFreeSurferBBRbased.sh Co-authored-by: Tim Coalson <[email protected]>

Update fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBR…

723df87

…AndFreeSurferBBRbased.sh Co-authored-by: Tim Coalson <[email protected]>

coalsont reviewed Jan 22, 2025

View reviewed changes

fMRIVolume/scripts/DistortionCorrectionAndEPIToT1wReg_FLIRTBBRAndFreeSurferBBRbased.sh Outdated Show resolved Hide resolved

coalsont reviewed Jan 22, 2025

View reviewed changes

fMRIVolume/scripts/OneStepResampling.sh Outdated Show resolved Hide resolved

+per review, removed irrelevant comments

53c153c

coalsont reviewed Jan 22, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modifications to generic fMRIVolume and fMRISurface pipelines to run on longitudinal sessions. #319

Modifications to generic fMRIVolume and fMRISurface pipelines to run on longitudinal sessions. #319

mmilch01 commented Dec 29, 2024 •

edited

Loading

demsarjure commented Dec 30, 2024

demsarjure commented Dec 30, 2024

demsarjure left a comment

demsarjure Dec 30, 2024

mmilch01 Dec 31, 2024

coalsont commented Dec 30, 2024

mmilch01 commented Dec 31, 2024 •

edited

Loading

mmilch01 commented Dec 31, 2024

glasserm Jan 5, 2025

mmilch01 Jan 6, 2025

glasserm Jan 7, 2025

mmilch01 Jan 15, 2025

coalsont Jan 22, 2025 •

edited

Loading

mmilch01 Jan 22, 2025

glasserm Jan 5, 2025

demsarjure Jan 5, 2025

glasserm left a comment

coalsont Jan 6, 2025

mmilch01 Jan 15, 2025 •

edited

Loading

coalsont Jan 6, 2025

mmilch01 Jan 15, 2025

glasserm commented Jan 7, 2025

coalsont Jan 22, 2025

coalsont Jan 22, 2025


		#Split echos
		#Split echos.

Modifications to generic fMRIVolume and fMRISurface pipelines to run on longitudinal sessions. #319

Are you sure you want to change the base?

Modifications to generic fMRIVolume and fMRISurface pipelines to run on longitudinal sessions. #319

Conversation

mmilch01 commented Dec 29, 2024 • edited Loading

demsarjure commented Dec 30, 2024

demsarjure commented Dec 30, 2024

demsarjure left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coalsont commented Dec 30, 2024

mmilch01 commented Dec 31, 2024 • edited Loading

mmilch01 commented Dec 31, 2024

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 22, 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

glasserm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmilch01 Jan 15, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasserm commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmilch01 commented Dec 29, 2024 •

edited

Loading

mmilch01 commented Dec 31, 2024 •

edited

Loading

coalsont Jan 22, 2025 •

edited

Loading

mmilch01 Jan 15, 2025 •

edited

Loading