Skip to content

Allow relative paths in run_fastsurfer.sh #732

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ClePol wants to merge 3 commits into from
Closed

Allow relative paths in run_fastsurfer.sh #732

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4fdff74
Allow relative paths in run_fastsurfer.sh
ClePol Sep 29, 2025
ccdb420
simplified for compatibility with older bash versions
ClePol Sep 29, 2025
e816cdd
remove space
m-reuter Sep 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 43 additions & 0 deletions run_fastsurfer.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,6 +504,49 @@ esac
done
set -- "${POSITIONAL[@]}" # restore positional parameters


# Function to convert path to absolute if it exists and is not empty
convert_to_absolute_path() {
local var_name="$1"
local var_value
eval "var_value=\$$var_name"

if [ -n "$var_value" ]; then
if [ -e "$var_value" ] || [ "${var_value#*/}" != "$var_value" ]; then
Copy link

Copilot AI Sep 30, 2025

Choose a reason for hiding this comment

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

[nitpick] The condition [ \"${var_value#*/}\" != \"$var_value\" ] is a complex way to check if a path contains directory separators. Consider using a more readable approach like [[ \"$var_value\" == */* ]] for better maintainability.

Suggested change
if [ -e "$var_value" ] || [ "${var_value#*/}" != "$var_value" ]; then
if [ -e "$var_value" ] || [[ "$var_value" == */* ]]; then

Copilot uses AI. Check for mistakes.
# Path exists or contains directory separators - convert to absolute
local abs_path
abs_path=$(realpath "$var_value" 2>/dev/null)
if [ $? -eq 0 ] && [ -n "$abs_path" ]; then
eval "$var_name=\"$abs_path\""
else
echo "WARNING: Failed to convert $var_name to absolute path: $var_value" | tee -a "${tmpLF:-/dev/null}"
Comment on lines +510 to +522
Copy link
Member

@dkuegler dkuegler Oct 1, 2025

Choose a reason for hiding this comment

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

how you can achieve the var=$(func "$val")" syntax is by echo-ing the return value and sending warnings and error messages to stderr.

Suggested change
local var_name="$1"
local var_value
eval "var_value=\$$var_name"
if [ -n "$var_value" ]; then
if [ -e "$var_value" ] || [ "${var_value#*/}" != "$var_value" ]; then
# Path exists or contains directory separators - convert to absolute
local abs_path
abs_path=$(realpath "$var_value" 2>/dev/null)
if [ $? -eq 0 ] && [ -n "$abs_path" ]; then
eval "$var_name=\"$abs_path\""
else
echo "WARNING: Failed to convert $var_name to absolute path: $var_value" | tee -a "${tmpLF:-/dev/null}"
# usage: convert_to_absolute_path <path> [<logfile>]
local var_value="$1"
local logging=("cat")
if [[ "$#" > 1]] ; then logging=(tee -a "$2") ; fi
if [ -n "$var_value" ]; then
if [ -e "$var_value" ] || [ "${var_value#*/}" != "$var_value" ]; then
# Path exists or contains directory separators - convert to absolute
local abs_path
abs_path=$(realpath "$var_value" 2>/dev/null)
if [ $? -eq 0 ] && [ -n "$abs_path" ]; then
echo "$abs_path"
else
echo "WARNING: Failed to convert $var_name to absolute path: $var_value" | "${logging[@]}" &>2

Overall, this function should probably be moved to functions.sh.

Also, logging can be done more elegantly like indicated.

fi
fi
fi
}

# Convert path variables to absolute paths
convert_to_absolute_path "sd"
Copy link
Member

@dkuegler dkuegler Oct 1, 2025

Choose a reason for hiding this comment

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

I would prefer the way this works to be like this

Suggested change
convert_to_absolute_path "sd"
sd="$(convert_to_absolute_path "$sd")"

The other version may work, but function that affect variables as a side effect are really ugly and should always be avoided.

convert_to_absolute_path "t1"
Copy link
Member

@dkuegler dkuegler Oct 1, 2025

Choose a reason for hiding this comment

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

Technically, --t1 and --t2 are documented as follows:

  --t1  <T1_input>        T1 full head input (not bias corrected). Requires an
                            ABSOLUTE Path!
  --t2 <T2_input>         *Optional* T2 full head input (does not have to be bias
                            corrected, a mandatory biasfield correction step is
                            performed). Requires an ABSOLUTE Path!

Also forcing t1 to be an absolute path will probably break longitudinal segmentation, see

  if [[ -n "$t1" ]] && [[ "$t1" != "from-base" ]]; then
    echo "WARNING: --t1 was passed but will be overwritten with T1 from base template." | tee -a "$tmpLF"
  fi

Also, the t1 input check checks if the t1 file exists.

if [[ "$run_seg_pipeline" == "1" ]] && { [[ -z "$t1" ]] || [[ ! -f "$t1" ]]; }
then
  echo "ERROR: T1 image ($t1) could not be found. You must supply an existing T1 input"
  echo "  (full head) via --t1 <absolute path and name> for generating the segmentation."
  echo "NOTE: If running in a container, make sure symlinks are valid!"
  exit 1
fi

But this can probably be fixed by moving the conversion block to "after" the input checking and sanitization (search for comment #### START ####)

convert_to_absolute_path "t2"
convert_to_absolute_path "seg_log"
convert_to_absolute_path "conformed_name"
convert_to_absolute_path "norm_name"
convert_to_absolute_path "norm_name_t2"
convert_to_absolute_path "asegdkt_segfile"
convert_to_absolute_path "mask_name"
convert_to_absolute_path "merged_segfile"
convert_to_absolute_path "cereb_segfile"
convert_to_absolute_path "cereb_statsfile"
convert_to_absolute_path "hypo_segfile"
convert_to_absolute_path "hypo_statsfile"
convert_to_absolute_path "asegdkt_vinn_statsfile"
convert_to_absolute_path "aseg_vinn_statsfile"
convert_to_absolute_path "aseg_segfile"
Comment on lines +532 to +545
Copy link
Member

@dkuegler dkuegler Oct 1, 2025

Choose a reason for hiding this comment

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

All these should not be made absolute

Suggested change
convert_to_absolute_path "seg_log"
convert_to_absolute_path "conformed_name"
convert_to_absolute_path "norm_name"
convert_to_absolute_path "norm_name_t2"
convert_to_absolute_path "asegdkt_segfile"
convert_to_absolute_path "mask_name"
convert_to_absolute_path "merged_segfile"
convert_to_absolute_path "cereb_segfile"
convert_to_absolute_path "cereb_statsfile"
convert_to_absolute_path "hypo_segfile"
convert_to_absolute_path "hypo_statsfile"
convert_to_absolute_path "asegdkt_vinn_statsfile"
convert_to_absolute_path "aseg_vinn_statsfile"
convert_to_absolute_path "aseg_segfile"

convert_to_absolute_path "FS_LICENSE"



# make sure FastSurfer is in the PYTHONPATH
export PYTHONPATH
PYTHONPATH="$FASTSURFER_HOME$([[ -n "$PYTHONPATH" ]] && echo ":$PYTHONPATH")"
Expand Down