Conversation
| if [[ "$RegressMethod" != "PFM_weighted" ]]; then | ||
| log_Err_Abort "Only weighted regression is supported!" | ||
| fi |
There was a problem hiding this comment.
Then why does the option exist?
There was a problem hiding this comment.
I agree, we should support PFM_DR too.
| # download HCP_MMP_ROIs if necessary | ||
| filecount_HCP_MMP_ROIs=$(find "$HCPPIPEDIR"/ArealClassifier/data/HCP_MMP_ROIs -maxdepth 1 -type f | wc -l) | ||
| if [[ "$filecount_HCP_MMP_ROIs" -lt 2024 || "$ForceDownloadWeight" -ne 0 ]]; then | ||
| "$HCPPIPEDIR"/global/scripts/download_areal_classifier_dependencies.sh "MMP_ROIs" | ||
| fi | ||
|
|
||
| # download model weights if necessary | ||
| if [ "$ClassifierVersion" == "MATLAB" ]; then | ||
| filecount_model_weights=$(find "$HCPPIPEDIR"/ArealClassifier/data/mlp_classifier -maxdepth 1 -type f | wc -l) | ||
| if [[ "$filecount_model_weights" -lt 368 || "$ForceDownloadWeight" -ne 0 ]]; then | ||
| "$HCPPIPEDIR"/global/scripts/download_areal_classifier_dependencies.sh "MATLAB" | ||
| fi | ||
| elif [ "$ClassifierVersion" == "ARENA_v1" ]; then |
There was a problem hiding this comment.
Consider making a --force-download=TRUE flag in download_areal_classifier_dependencies.sh, and having it internally check whether they are already downloaded (if not requested to be forced, which would save a small amount of time and IOPS). This would put the details of the check for completeness into the same file as the URL, so if we ever needed to modify the zips, both changes would be in a single script.
You could also have it internally unconditionally look at the ROIs. Then here, you could simply do:
"$HCPPIPEDIR"/global/scripts/download_areal_classifier_dependencies.sh --model="$ClassifierVersion" --force-download="$ForceDownloadWeight"
and get both pieces.
For that specific download script, we could skip the if ((pipedirguessed)) error of the usual newopts template, so that it can be called outside of a pipeline. In some setups (probably including qunex), the pipelines folder isn't writable while the pipelines are running, so making the download script work without the full SetUpHCP... (so that it can easily be done in advance) is important. If we want qunex to support all 3 models, then we might want a --model=ALL possibility, to simplify the qunex build process (and make it future-proof).
There was a problem hiding this comment.
In fact, if someone just goes ahead and launches cluster jobs, but their pipelines clone doesn't have these (and is writable), then they will immediately have jobs stomping on each other's downloads, in a folder (and probably filesystem) that isn't where the imaging data is. Maybe the pipelines shouldn't be trying to automatically trigger the download by default.
There was a problem hiding this comment.
...which also means that a check for whether to tell the user to go download them should be separate from the download script after all...
| opts_AddOptional '--output-suffix' 'FeatureOutputName' 'name' "suffix for output files, like NoTask" | ||
| opts_AddMandatory '--classifier-training' 'TrainedFolder' 'path' "classifier training weights" | ||
| opts_AddMandatory '--classifier-version' 'ClassifierVersion' 'string' "either the matlab version 'MATLAB' or the python version 'ARENA_v1', 'ARENA_v2'" | ||
| # opts_AddOptional '--python-singularity' 'PythonSingularity' 'string' "the file path of the singularity" "$HCPPIPEDIR/ArealClassifier/singularity/hcp_python_singularity.simg" |
There was a problem hiding this comment.
If we aren't going to use a singularity container within this pipeline, we should probably remove this and all related stuff.
| #maybe _$RegName should be $RegString, but needs fixing in combine, too | ||
| #CHECK INPUT NAME, currently assuming TopographicRegression magically makes it | ||
| matlabCode="addpath '$mPath'; addpath '$mGlobalPath'; | ||
| computeMisc('${inputdtseriestxt}', \ |
There was a problem hiding this comment.
This is not a particularly good function name.
| wb_command -cifti-create-dense-from-template \ | ||
| "$HCPPIPEDIR"/global/templates/91282_Greyordinates/91282_Greyordinates.dscalar.nii \ | ||
| "${ResultsFolder}/${OutrfMRIName}/${OutrfMRIName}_Atlas_MSMAll_dropouts.dscalar.nii" \ | ||
| -metric CORTEX_LEFT "$leftdropouts" \ | ||
| -metric CORTEX_RIGHT "$rightdropouts" |
There was a problem hiding this comment.
Looks like the cifti is the only important output here, consider replacing most of this (MapVolumeToSurface.sh until here) with a call to https://github.com/Washington-University/HCPpipelines/blob/master/global/scripts/VolumeToCIFTI.sh instead.
| AreaNamesTemp="$(mktemp --tmpdir XXXXXX.areanames.txt)" | ||
| tempfiles_add "$AreaNamesTemp" |
There was a problem hiding this comment.
| AreaNamesTemp="$(mktemp --tmpdir XXXXXX.areanames.txt)" | |
| tempfiles_add "$AreaNamesTemp" | |
| tempfiles_create areanames_XXXXXX.txt AreaNamesTemp |
| wb_command -cifti-math "(var - 1) * -1" "$workdir/${tempbase}_Areas_rawinv.$dscalarend" -var var "$workdir/${tempbase}_Areas_raw.$dscalarend" | ||
| wb_command -cifti-dilate "$workdir/${tempbase}_Areas_rawinv.$dscalarend" COLUMN ${ErodeDilateDistance} ${ErodeDilateDistance} "$workdir/${tempbase}_Areas_core_inv.$dscalarend" -${hemisphereword}-surface "${T1wDownSampleFolder}/${Subject}.${Hem}.midthickness${RegSTRING}.${LowResMesh}k_fs_LR.surf.gii" -nearest | ||
| wb_command -cifti-math "(var - 1) * -1" "$workdir/${tempbase}_Areas_core.$dscalarend" -var var "$workdir/${tempbase}_Areas_core_inv.$dscalarend" |
There was a problem hiding this comment.
This will need testing, but avoids a few bits of silliness (at the time, some of this may have been needed due to a bug in -cifti-erode).
| wb_command -cifti-math "(var - 1) * -1" "$workdir/${tempbase}_Areas_rawinv.$dscalarend" -var var "$workdir/${tempbase}_Areas_raw.$dscalarend" | |
| wb_command -cifti-dilate "$workdir/${tempbase}_Areas_rawinv.$dscalarend" COLUMN ${ErodeDilateDistance} ${ErodeDilateDistance} "$workdir/${tempbase}_Areas_core_inv.$dscalarend" -${hemisphereword}-surface "${T1wDownSampleFolder}/${Subject}.${Hem}.midthickness${RegSTRING}.${LowResMesh}k_fs_LR.surf.gii" -nearest | |
| wb_command -cifti-math "(var - 1) * -1" "$workdir/${tempbase}_Areas_core.$dscalarend" -var var "$workdir/${tempbase}_Areas_core_inv.$dscalarend" | |
| wb_command -cifti-erode "$workdir/${tempbase}_Areas_raw.$dscalarend" COLUMN ${ErodeDilateDistance} ${ErodeDilateDistance} "$workdir/${tempbase}_Areas_core.$dscalarend" -${hemisphereword}-surface "${T1wDownSampleFolder}/${Subject}.${Hem}.midthickness${RegSTRING}.${LowResMesh}k_fs_LR.surf.gii" |
| wb_command -cifti-math "(var - 1) * -1" "$workdir/${tempbase}_Areas_dil$((${ErodeDilateDistance}*2))inv.$dscalarend" -var var "$workdir/${tempbase}_Areas_dil$((${ErodeDilateDistance}*2)).$dscalarend" | ||
| wb_command -cifti-dilate "$workdir/${tempbase}_Areas_dil$((${ErodeDilateDistance}*2))inv.$dscalarend" COLUMN $((${ErodeDilateDistance}*2)) $((${ErodeDilateDistance}*2)) "$workdir/${tempbase}_Areas_dil$((${ErodeDilateDistance}*2))inv_ero$((${ErodeDilateDistance}*2)).$dscalarend" -${hemisphereword}-surface "${T1wDownSampleFolder}/${Subject}.${Hem}.midthickness${RegSTRING}.${LowResMesh}k_fs_LR.surf.gii" -nearest | ||
| wb_command -cifti-math "(var - 1) * -1" "$workdir/${tempbase}_Areas_dil$((${ErodeDilateDistance}*2))_ero$((${ErodeDilateDistance}*2)).$dscalarend" -var var "$workdir/${tempbase}_Areas_dil$((${ErodeDilateDistance}*2))inv_ero$((${ErodeDilateDistance}*2)).$dscalarend" |
There was a problem hiding this comment.
Similar, could convert to -cifti-erode.
| wb_command -cifti-math "(var - 1) * -1" "$workdir/${tempbase}_Areas_dil$((${ErodeDilateDistance}*2))_ero$((${ErodeDilateDistance}*2)).$dscalarend" -var var "$workdir/${tempbase}_Areas_dil$((${ErodeDilateDistance}*2))inv_ero$((${ErodeDilateDistance}*2)).$dscalarend" | ||
|
|
||
| #Mask out core ROI from all areas and then add core back in for each area | ||
| wb_command -cifti-math "(dilero * (((coreroi > 0) - 1) * -1)) + (raw * (coreroi > 0))" "$workdir/${tempbase}_Areas_joined.$dscalarend" -var raw "$workdir/${tempbase}_Areas_raw.$dscalarend" -var dilero "$workdir/${tempbase}_Areas_dil$((${ErodeDilateDistance}*2))_ero$((${ErodeDilateDistance}*2)).$dscalarend" -var coreroi "$workdir/${tempbase}_Areas_core_roi.$dscalarend" -select 1 1 -repeat |
There was a problem hiding this comment.
| wb_command -cifti-math "(dilero * (((coreroi > 0) - 1) * -1)) + (raw * (coreroi > 0))" "$workdir/${tempbase}_Areas_joined.$dscalarend" -var raw "$workdir/${tempbase}_Areas_raw.$dscalarend" -var dilero "$workdir/${tempbase}_Areas_dil$((${ErodeDilateDistance}*2))_ero$((${ErodeDilateDistance}*2)).$dscalarend" -var coreroi "$workdir/${tempbase}_Areas_core_roi.$dscalarend" -select 1 1 -repeat | |
| wb_command -cifti-math '(dilero * (!(coreroi > 0)) + (raw * (coreroi > 0))' "$workdir/${tempbase}_Areas_joined.$dscalarend" -var raw "$workdir/${tempbase}_Areas_raw.$dscalarend" -var dilero "$workdir/${tempbase}_Areas_dil$((${ErodeDilateDistance}*2))_ero$((${ErodeDilateDistance}*2)).$dscalarend" -var coreroi "$workdir/${tempbase}_Areas_core_roi.$dscalarend" -select 1 1 -repeat |
| wb_command -cifti-math "var * (ROI > 0)" "$workdir/${tempbase}_Areas_raw_masked.$dscalarend" -var var "$workdir/${tempbase}_Areas_raw.$dscalarend" -var ROI "$workdir/${tempbase}_Areas_raw_fc.$dscalarend" | ||
|
|
||
| #Dilate and Erode to connect to connect things 4mm | ||
| wb_command -cifti-dilate "$workdir/${tempbase}_Areas_raw.$dscalarend" COLUMN $((${ErodeDilateDistance}*2)) $((${ErodeDilateDistance}*2)) "$workdir/${tempbase}_Areas_dil$((${ErodeDilateDistance}*2)).$dscalarend" -${hemisphereword}-surface "${T1wDownSampleFolder}/${Subject}.${Hem}.midthickness${RegSTRING}.${LowResMesh}k_fs_LR.surf.gii" -nearest |
There was a problem hiding this comment.
$((${ErodeDilateDistance}*2)) only works as expected for integer distances (which are in mm). This should also probably be stored in another variable, rather than recomputed in lots of filenames.
| wb_command -cifti-reduce "$workdir/${tempbase}_Areas_raw_fcsmall_fillableholes.$dscalarend" MAX "$workdir/${tempbase}_Areas_raw_fcsmall_fillableholes.$dscalarend" | ||
|
|
||
| #find parts that were zeroed by find clusters | ||
| wb_command -cifti-math "((Var > 0) - (Varfcsmall > 0)) * Fillable" "$workdir/${tempbase}_Areas_raw_fcsmallholesall.$dscalarend" -var Var "$workdir/${tempbase}_Areas_raw.$dscalarend" -var Varfcsmall "$workdir/${tempbase}_Areas_raw_fcsmall.$dscalarend" -var Fillable "$workdir/${tempbase}_Areas_raw_fcsmall_fillableholes.$dscalarend" -select 1 1 -repeat |
There was a problem hiding this comment.
Since _fcsmall can't be nonzero where _Areas_raw is zero, and _fillableholes is binary, this should equal:
| wb_command -cifti-math "((Var > 0) - (Varfcsmall > 0)) * Fillable" "$workdir/${tempbase}_Areas_raw_fcsmallholesall.$dscalarend" -var Var "$workdir/${tempbase}_Areas_raw.$dscalarend" -var Varfcsmall "$workdir/${tempbase}_Areas_raw_fcsmall.$dscalarend" -var Fillable "$workdir/${tempbase}_Areas_raw_fcsmall_fillableholes.$dscalarend" -select 1 1 -repeat | |
| wb_command -cifti-math '((Var > 0) && !(Varfcsmall > 0)) && Fillable' "$workdir/${tempbase}_Areas_raw_fcsmallholesall.$dscalarend" -var Var "$workdir/${tempbase}_Areas_raw.$dscalarend" -var Varfcsmall "$workdir/${tempbase}_Areas_raw_fcsmall.$dscalarend" -var Fillable "$workdir/${tempbase}_Areas_raw_fcsmall_fillableholes.$dscalarend" -select 1 1 -repeat |
Co-authored-by: Tim Coalson <coalsont@users.noreply.github.com>
Co-authored-by: Tim Coalson <coalsont@users.noreply.github.com>
Co-authored-by: Tim Coalson <coalsont@users.noreply.github.com>
Co-authored-by: Tim Coalson <coalsont@users.noreply.github.com>
| fi | ||
| #average dropouts across runs, map to surface, put in cifti, since nothing else uses them this way | ||
| #these temp names are a bit messy, but who cares | ||
| tempfiles_create XXXXXX.dropouts.nii.gz volmerged |
There was a problem hiding this comment.
Use a bit more descriptive name for temporary files, they end up in provenance, and make them more tab-complete friendly (for debugging) by not starting with the random characters.
| tempfiles_create XXXXXX.dropouts.nii.gz volmerged | |
| tempfiles_create classifier_dropouts_XXXXXX.nii.gz volmerged |
|
With regard to map volume to surface, I thought @coalsont that we already had that in the repo somewhere. |
| fi | ||
|
|
||
| # check classifier version | ||
| if [[ "$ClassifierVersion" == "MATLAB" || "$ClassifierVersion" == "ARENA_v1" || "$ClassifierVersion" == "ARENA_v2" ]]; then |
There was a problem hiding this comment.
I wasn't planning to release the MATLAB classifier since there is complexity with its source code.
| # download model weights if necessary | ||
| if [ "$ClassifierVersion" == "MATLAB" ]; then | ||
| filecount_model_weights=$(find "$HCPPIPEDIR"/ArealClassifier/data/mlp_classifier -maxdepth 1 -type f | wc -l) | ||
| if [[ "$filecount_model_weights" -lt 368 || "$ForceDownloadWeight" -ne 0 ]]; then | ||
| "$HCPPIPEDIR"/global/scripts/download_areal_classifier_dependencies.sh "MATLAB" | ||
| fi |
There was a problem hiding this comment.
See above about the matlab classifier.
| if [[ "$ClassifierVersion" == "MATLAB" || "$ClassifierVersion" == "ARENA_v1" || "$ClassifierVersion" == "ARENA_v2" ]]; then | ||
| log_Msg "ClassifierVersion is ${ClassifierVersion}." | ||
| else | ||
| log_Err_Abort "ClassifierVersion must be one of the followings, 'MATLAB', 'ARENA_v1', 'ARENA_v2' but gets ${ClassifierVersion}" |
There was a problem hiding this comment.
See above about matlab classifier.
No description provided.