update SingleSubjectConcat with frame selection; update MSMAll pipeline

[cyang31]

Background

Previously the script SingleSubjectConcat.sh doesn't enable the frame range selection for each fMRI runs. Hence, the MSMAll is limited to be only applied to the full timeseries.

Main changes

• replace the old opt with Tim's newopt library in SingleSubjectConcat.sh;
• remove the bias correction and computation of VN files in SingleSubjectConcat.sh;
• add two optional arguments StartFrame and EndFrame in SingleSubjectConcat.sh and MSMAllPipeline.sh;
• validate the input frame range and throw an error if invalid range is provided in SingleSubjectConcat.sh;

Evaluation

One subject with resting state fMRI is evaluated. No error.

[coalsont]

I will point out that this PR is currently marked as a draft, which suggests it isn't ready for full scrutiny yet.

[cyang31]

Thanks for your comments. I have addressed all of them.

[glasserm]

Please produce some test outputs after you address Tim's concern above.

[cyang31]

Are you pointing to the duration calculation part?

[cyang31]

For the whole pipeline test, here are the arguments.
Tue Feb 28 19:38:09 CST 2023:MSMAllPipeline.sh: arguments: --study-folder=/media/myelin/brainmappers/Connectome_Project/YA_HCP_ReTest_Final --subject=103818 --fmri-names-list=rfMRI_REST1_LR@rfMRI_REST1_RL@rfMRI_REST2_LR@rfMRI_REST2_RL --multirun-fix-names= --multirun-fix-concat-name= --multirun-fix-names-to-use= --output-fmri-name=rfMRI_REST_38mins26secs_400To1200 --high-pass=2000 --fmri-proc-string=_Atlas_hp2000_clean --msm-all-templates=/media/myelin/alex/pipelines/HCPpipelines/global/templates/MSMAll --output-registration-name=MSMAll_AlexTest_rest_38mins26secs_400To1200 --high-res-mesh=164 --low-res-mesh=32 --input-registration-name=MSMSulc --start-frame=400 --end-frame=1200 --matlab-run-mode=1

Now RegName and OutfMRIName are specified with frame range and the duration. The purpose of including these information in the file name is to avoid overwriting and meanwhile prepare for the following experiments on the MSMAll optimization with varying amount of fMRII data.

The examples for output files:

• /media/myelin/brainmappers/Connectome_Project/YA_HCP_ReTest_Final/103818/MNINonLinear/fsaverage_LR32k/103818.ArealDistortion_MSMAll_AlexTest_rest_38mins26secs_400To1200_1_d40_WRN.32k_fs_LR.dscalar.nii
• /media/myelin/brainmappers/Connectome_Project/YA_HCP_ReTest_Final/103818/MNINonLinear/103818.ArealDistortion_MSMAll_AlexTest_rest_38mins26secs_400To1200_2_d40_WRN.164k_fs_LR.dscalar.nii
• /media/myelin/brainmappers/Connectome_Project/YA_HCP_ReTest_Final/103818/MNINonLinear/Native/103818.ArealDistortion_MSMAll_AlexTest_rest_38mins26secs_400To1200_1_d40_WRN.native.dscalar.nii
• /media/myelin/brainmappers/Connectome_Project/YA_HCP_ReTest_Final/103818/MNINonLinear/Results/rfMRI_REST_38mins26secs_400To1200/rfMRI_REST_38mins26secs_400To1200_Atlas_hp2000_clean_vn.dtseries.nii

[glasserm]

Is this a temporary file: /media/myelin/brainmappers/Connectome_Project/YA_HCP_ReTest_Final/103818/MNINonLinear/Results/rfMRI_REST_38mins26secs_400To1200/frames_duration.txt?

[cyang31]

Is this a temporary file: /media/myelin/brainmappers/Connectome_Project/YA_HCP_ReTest_Final/103818/MNINonLinear/Results/rfMRI_REST_38mins26secs_400To1200/frames_duration.txt?

No, this is an output file that aims to let user know explicitly how long this concatenation is and which frame range is selected.

[glasserm]

But that is in the file name now, right?

[cyang31]

But that is in the file name now, right?

The file name is set by user. This file is calculated via the script. I think we can provide a correct source on this.

[glasserm]

I see, we have gone back and forth on this. I know Tim had concerns about this being programmatically set. Keeping a file around and having the user set the values also seems not ideal. Perhaps we should not keep the file and programmatically set things?

[cyang31]

I see, we have gone back and forth on this. I know Tim had concerns about this being programmatically set. Keeping a file around and having the user set the values also seems not ideal. Perhaps we should not keep the file and programmatically set things?

This frame_duration.txt file is not used to set the file names, it is just a reminder to help user confirm that they have set the right name. If adding another file to the directory is not ideal, maybe I should just drop the logic to save the file and only log the calculated frame and duration.

[glasserm]

Logging would make a lot of sense, though I see the desirability of programmatically setting the file too.

[cyang31]

The latest commit deletes the matlab run mode from SingleSubjectConcat.sh and comments out the part to save the frame and duration file.

[glasserm]

Let's try to come to consensus with @coalsont on the programmatic vs user defined naming before we merge this.

[coalsont]

As I understand it, he specified --output-fmri-name=rfMRI_REST_38mins26secs_400To1200 in that run. It looks auto-generated because it was copied and pasted from the earlier runs. It is likely that future runs would not have that particular format of duration in the name.

I think it is fine to generate a "frames_duration.txt" file when using non-default start and end frames (or always, why not), I don't see a drawback to having that information inside that file, since it appears to be in a folder specific to that concatenation.

As long as the output filenames are built with --output-fmri-name and constants, with no extra internally computed stuff added, I am okay with it.

[coalsont]

If you aren't already, please test the edit I made from MergeSTRING to MergeArray.

[glasserm]

Sounds like Tim has said what he wants, so if this is fully tested I think we can merge it.

[cyang31]

I have updated a commit to address Tim's comments. Regarding the default behavior of an empty EndFrame, here are two passed tests.

Test1: StartFrame=200; EndFrame="";
/media/myelin/alex/MSMAll_optimization/SingleSubjectConcat.sh.o2192569
Passed

Test2: StartFrame=1; EndFrame="";
/media/myelin/alex/MSMAll_optimization/SingleSubjectConcat.sh.o2181189
Passed

[glasserm]

Looks like this needs some cleanup to be able to merge...

[cyang31]

Looks like this needs some cleanup to be able to merge...

Which part exactly?

[cyang31]

I think the current commit should have addressed all the comments above.

[glasserm]

Still has conflicts to merge.

[coalsont]

The conflicts were caused by merging master into the remote branch. Rebasing would have kept it able to rebase cleanly. Squash and merge says it will work cleanly, and might be a better choice than adding the 15 commits separately.

[cyang31]

I opened a new PR where there's only one commit including the latest changes. I wonder if this is an appropriate solution.

[coalsont]

"Squash and merge" is an option on github, you didn't need to do anything. I'd rather merge this one, because we know we reviewed the code in it, than to check that the new one has exactly the same changes.

[cyang31]

Okay, I see.