--fix-only option for hcp_fix_multi_run #297

demsarjure · 2024-08-20T08:13:40Z

This feature implements a new parameter (--fix-only) for hcp_fix_multi_run. This parameter can be used when to speed up ICAFix optimizations by skipping melodic and proceed with FIX on a previously computed ICA solution.

…_run.

ICAFIX/hcp_fix_multi_run

coalsont · 2024-08-20T13:54:52Z

ICAFIX/hcp_fix_multi_run

-	ls -lRa ${concatfmrihp}.ica
-fi
+	if [ -d "${concatfmrihp}.ica/fix" ]; then
+		mv "${concatfmrihp}.ica/fix" "${concatfmrihp}.ica/fix-bkp-$(date +"%Y-%m-%d.%H%M%S")"


Do we want the pipeline to internally make a backup? Granted, this option is a special case, but we generally don't like making backups the user doesn't know about.

Backing up previous solution is important in the process of iteratively building and evaluating FIX models, which is the use case that this option is made for. We could add another optional parameter to give the user more control, but I suspect that whoever would use --fix-only would also want to have the previous fix solutions backed up. To make the backup explicit, I have now added a message in the log (it will be in the next PR):

if [ -d "${concatfmrihp}.ica/fix" ]; then fix_backup_suffix="-bkp-$(date +"%Y-%m-%d.%H%M%S")" mv "${concatfmrihp}.ica/fix" "${concatfmrihp}.ica/fix${fix_backup_suffix}" log_Msg "previous fix solution found and renamed to fix${fix_backup_suffix}" fi

Perhaps that behavior should be a separate option?

@glasserm, we can make it a separate option, but I suspect it will be always used in the use cases for --skip-ica.

Also, the option only makes sense when using --skip-ica as with a new ica the old fix does not refer to the same components any more.

@demsarjure @sivcek Can you add a --backup-fix-if-ica-skipped boolean option that defaults to true? Or, we could make a more general --backup-existing-fix option that defaults to false, and also works when not using --skip-ica.

Edit: oh right, with a new ICA, a backup of fix may not be very useful, so it should probably stay specific to this new option.

What happened to @coalsont's previous suggestion for an alternative name to --skip-ica? I don't recall exactly what it was, but I do recall that he made a good argument for his alternative name.

Or, to make it more scriptable, it could be something like --fix-backup-suffix=<string> or --fix-backup-folder=<fix-bkp-something>, doesn't make a backup if the value is "", otherwise names the backup folder accordingly, rather than generating it from date. The second form could even take an absolute path, or be used to put all the backups into a backups/ subfolder instead of littering the ${concatfmrihp}.ica folder directly.

Ok, found it. It was --reuse-existing-ica:
#297 (comment)

@mharms I had resolved it as somewhat minor and we seemed close to merging, but I have unresolved it again since you seem to have an opinion on it.

coalsont · 2024-08-20T14:00:08Z

ICAFIX/hcp_fix_multi_run

@@ -124,6 +128,8 @@ opts_AddConfigOptional '--icadim-mode' 'icadimmode' 'icadimmode' '"default" or "

 opts_AddOptional '--processing-mode' 'ProcessingMode' '"HCPStyleData" (default) or "LegacyStyleData"' "controls whether --icadim-mode=fewtimepoints is allowed" 'HCPStyleData'

+opts_AddOptional '--fix-only' 'RunFIXOnly' 'TRUE or FALSE' 'If TRUE, it skips running ICA using melodic and proceeds directly with running FIX. A previous ICA solution has to exist!' 'FALSE'


Consider whether --skip-ica or --reuse-existing-ica or something might be a better name. "Only" implies that it can never be combined with other possible process-altering flags (and that if we add another step after fix, --fix-only might be expected to skip it, while --skip-ica would be expected to run it).

I agree that --skip-ica is clearer. I changed the code for the new version of the PR.

Personally, I like the descriptiveness of --reuse-existing-ica, so the user doesn't think they can just skip the ica on the first run. "reuse" has some parallels to the tICA modes, too.

To be clear, please push the commits that make these changes to the same branch on github, and they will show up here. Please don't open an additional PR for changes requested on this PR.

glasserm · 2024-08-20T22:20:34Z

For some reason this diff is really hard to interpret. Any idea why it is a sea of red and green even though I assume the changes were pretty minor?

glasserm · 2024-08-20T22:21:10Z

Also, it looks like there are conflicts that need to be resolved.

coalsont · 2024-08-20T22:42:31Z

Indentation. In the gear button on the "files changed" page, check "hide whitespace".

Github says no conflicts for squash, but there are some changes from -volume-math/-volume-merge to fslmaths/fslmerge, which I am unsure of the purpose of. If they weren't intentional, something may have been done something with the branch history that could cause a regression (for instance, copying an old version of a file, making edits, and then committing it to the current branch state after a reset).

ICAFIX/hcp_fix_multi_run

glasserm

This pull request seems to be making a lot more changes to the code than are actually needed for the requested functionality. It could be that it is reverting changes that were made subsequent to whenever these commits were first made (it appears they are from some months back). We need a pull request based on the latest master.

sivcek · 2024-08-21T10:10:15Z

I have committed the version with the changes requested:

adding the functionality to the latest master to avoid reverting existing code
changing --fix-only to --skip-ica
using opts_StringToBool to process the input string
adding a log message reporting that an old fix solution was found and backed up

glasserm

Thanks this is much cleaner.

ICAFIX/hcp_fix_multi_run

coalsont · 2024-09-04T03:11:04Z

The unresolved items as I see it are:

@glasserm suggested the backup saving should be a separate option, and I suggested having the name of the backup be passed into the script, which is more predictable than being generated from the date/time at which it reached that line in the script.
@mharms seconded (I think?) my --reuse-existing-ica suggestion for the flag name

sivcek · 2024-11-30T18:00:25Z

This update:

merges the current state of master into the PR
renames --skip-ica option to --reuse-existing-ica
adds --fix-backup option that specifies the folder to which to backup a previous fix solution if one exists

The update is currently being tested.

coalsont · 2024-12-02T21:57:14Z

ICAFIX/hcp_fix_multi_run

-AlreadyHP="-1" #Don't run highpass on concatenated data (since hp was done on the individual runs)
-
-case ${MatlabMode} in
-	0)
-		#Use compiled Matlab
-		compiledScript="$HCPPIPEDIR"/ICAFIX/scripts/Compiled_functionhighpassandvariancenormalize/run_functionhighpassandvariancenormalize.sh
-		matlab_cmd=("$compiledScript" "${MATLAB_COMPILER_RUNTIME}" "$tr" "$AlreadyHP" "$concatfmrihp" "${Caret7_Command}" "" "$volwisharts" "$ciftiwisharts" "$icadimmode")
-		log_Msg "Run compiled MATLAB with command..."
-		log_Msg "${matlab_cmd[*]}"
-		"${matlab_cmd[@]}"
-		;;
-
-	1 | 2)
-		# Use interpreted MATLAB or Octave
-		# ${hp} needs to be passed in as a string, to handle the hp=pd* case
-		matlab_code="${ML_PATHS} functionhighpassandvariancenormalize(${tr}, ${AlreadyHP}, '${concatfmrihp}', '${Caret7_Command}', '', ${volwisharts}, ${ciftiwisharts}, '${icadimmode}');"
-		log_Msg "Run interpreted MATLAB/Octave (${matlab_interpreter[*]}) with code..."
-		log_Msg "${matlab_code}"
-		"${matlab_interpreter[@]}" <<<"$matlab_code"
-		echo
-		;;
-esac
-
-Dim=$(cat ${concatfmrihp}_dims.txt)
-
-log_Msg "Dim: ${Dim}"
-
-if [[ $ICAmethod == MELODIC ]]; then
-	## ---------------------------------------------------------------------------
-	## Run melodic on concatenated file
-	## ---------------------------------------------------------------------------
-
-	log_Debug_Msg "About to run melodic: Contents of ${concatfmrihp}.ica follow"
-	if [ ! -z "${log_debugOn}" ] ; then
-		ls -lRa ${concatfmrihp}.ica
-	fi
-
-	#grab melodic from $PATH by default, don't hardcode it with respect to $FSLDIR
-	#we need to do "if which ..." because the script currently uses ERR trap
-	if which melodic &> /dev/null
-	then
-		MELODIC=$(which melodic 2> /dev/null)
-	else
-		#if it isn't even in $PATH, fall back on FSLDIR
-		MELODIC="${FSLDIR}/bin/melodic"
-	fi
-
-	log_Msg "Running MELODIC located at: $MELODIC"
-	log_Debug_Msg "Beginning of melodic version log, help, and checksum"
-	if [ ! -z "${log_debugOn}" ] ; then
-		log_Debug_Msg "$MELODIC --version"
-		$MELODIC --version
-		log_Debug_Msg "$MELODIC --help"
-		$MELODIC --help
-		log_Debug_Msg "md5sum $MELODIC"
-		md5sum $MELODIC
-	fi
-	log_Debug_Msg "End of melodic version log, help, and checksum"
-
-	# Use the Wishart-based dim estimate instead of the melodic-based estimate (--dim=${Dim})
-	# Also turn off melodic VN and feed in the input that has already been VN'ed instead (--vn flag, which turns OFF melodic's vn)
-	# Added "-m ${concatfmri}_brain_mask" to avoid memory issue - Takuya Hayahsi July 2018
-	melodic_cmd=("${MELODIC}" -i "${concatfmrihp}_vnts" -o "${concatfmrihp}.ica/filtered_func_data.ica" --nobet --report --Oall --tr="${tr}" --vn --dim="${Dim}" -m "${concatfmri}_brain_mask")
-
-	if [ ! -z "${log_debugOn}" ] ; then
-		melodic_cmd+=(--verbose --debug)
-	fi
-
-	#keep the existing handling code for melodic errors?
-	debug_disable_trap
-
-	log_Msg "melodic_cmd: ${melodic_cmd[*]}"
-	"${melodic_cmd[@]}"
-	return_code=$?
-	log_Msg "melodic has been run: return_code = ${return_code}"
-	log_Debug_Msg "melodic has been run: Contents of ${concatfmrihp}.ica follow"
-	if [ ! -z "${log_debugOn}" ] ; then
-		ls -lRa ${concatfmrihp}.ica
-	fi
-
-	if [ "${return_code}" -ne "0" ] ; then
-		log_Err_Abort "melodic has returned a non-zero code"
+else
+    log_Msg "Skipping ICA and proceeding directly to FIX. All the files generated by ICA have to be present!"
+
+	# Set the variables
+	ConcatFolder=`dirname ${ConcatName}`
+	ConcatNameNoExt=$($FSLDIR/bin/remove_ext $ConcatName)  # No extension, but still includes the directory path
+	concatfmri=`basename ${ConcatNameNoExt}`  # Directory path is now removed
+	concatfmrihp=${concatfmri}_hp${hp}
+
+	# Switch to concat folder
+	cd ${ConcatFolder}
+
+    if [ -d "${concatfmrihp}.ica/fix" ] && [ -n "$FIXBackup" ]; then
+        pushd "${concatfmrihp}.ica"
+        mv fix $FIXBackup
+        popd
+		log_Msg "Previous fix solution found and moved to ${$FIXBackup}."
 	fi


This has quite a mix of tabs and spaces.

demsarjure and others added 4 commits March 5, 2024 09:14

Added functionality to skip ICA and execute fix only in hcp_fix_multi…

dff7a82

…_run.

fix: add fix housekeeping to fixonly

3d78340

update: backup older fix folder when running fixonly

601b452

fix: add fix housekeeping to fixonly

8c72fb3

coalsont reviewed Aug 20, 2024

View reviewed changes

ICAFIX/hcp_fix_multi_run Outdated Show resolved Hide resolved

coalsont reviewed Aug 20, 2024

View reviewed changes

ICAFIX/hcp_fix_multi_run Outdated Show resolved Hide resolved

glasserm reviewed Aug 20, 2024

View reviewed changes

ICAFIX/hcp_fix_multi_run Show resolved Hide resolved

glasserm requested changes Aug 20, 2024

View reviewed changes

fix: update to changes from the latest master

69c0c61

glasserm approved these changes Aug 21, 2024

View reviewed changes

coalsont reviewed Aug 21, 2024

View reviewed changes

ICAFIX/hcp_fix_multi_run Outdated Show resolved Hide resolved

fix: improved if statement

4188b4f

sivcek added 2 commits November 30, 2024 17:53

Merge branch 'master' into fixonly

e6627bb

fix: minor documetation corrections

dfca18c

coalsont reviewed Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--fix-only option for hcp_fix_multi_run #297

--fix-only option for hcp_fix_multi_run #297

demsarjure commented Aug 20, 2024

coalsont Aug 20, 2024

sivcek Aug 20, 2024

glasserm Aug 20, 2024

sivcek Aug 20, 2024

sivcek Aug 20, 2024

coalsont Aug 22, 2024 •

edited

Loading

mharms Aug 22, 2024

coalsont Aug 22, 2024 •

edited

Loading

mharms Aug 22, 2024

coalsont Aug 22, 2024

coalsont Aug 20, 2024

sivcek Aug 20, 2024

coalsont Aug 20, 2024

glasserm commented Aug 20, 2024

glasserm commented Aug 20, 2024

coalsont commented Aug 20, 2024

glasserm left a comment

sivcek commented Aug 21, 2024 •

edited

Loading

glasserm left a comment

coalsont commented Sep 4, 2024

sivcek commented Nov 30, 2024

coalsont Dec 2, 2024

		@@ -124,6 +128,8 @@ opts_AddConfigOptional '--icadim-mode' 'icadimmode' 'icadimmode' '"default" or "

		opts_AddOptional '--processing-mode' 'ProcessingMode' '"HCPStyleData" (default) or "LegacyStyleData"' "controls whether --icadim-mode=fewtimepoints is allowed" 'HCPStyleData'

		opts_AddOptional '--fix-only' 'RunFIXOnly' 'TRUE or FALSE' 'If TRUE, it skips running ICA using melodic and proceeds directly with running FIX. A previous ICA solution has to exist!' 'FALSE'

--fix-only option for hcp_fix_multi_run #297

Are you sure you want to change the base?

--fix-only option for hcp_fix_multi_run #297

Conversation

demsarjure commented Aug 20, 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

Choose a reason for hiding this comment

coalsont Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coalsont Aug 22, 2024 • 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

glasserm commented Aug 20, 2024

glasserm commented Aug 20, 2024

coalsont commented Aug 20, 2024

glasserm left a comment

Choose a reason for hiding this comment

sivcek commented Aug 21, 2024 • edited Loading

glasserm left a comment

Choose a reason for hiding this comment

coalsont commented Sep 4, 2024

sivcek commented Nov 30, 2024

Choose a reason for hiding this comment

coalsont Aug 22, 2024 •

edited

Loading

coalsont Aug 22, 2024 •

edited

Loading

sivcek commented Aug 21, 2024 •

edited

Loading