Skip to content
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.

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

Removal of hard-coded configuration file in preprocess_segment.sh script and extraction of include_list from the configuration file directly #75

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

rohanbanerjee
Copy link
Contributor

@rohanbanerjee rohanbanerjee commented Aug 17, 2023

This PR solves the issues #72 and #73

What this PR solves:

  • In the preprocess_segment.sh, the configuration.json file was hard-coded which meant whatever we pass as a value the -config flag, it will read only the configuration.json from the script. This PR takes the configuration.json from the sct_run_batch command rather than a hard-coded value.
  • We are also passing the values to --include-list while running the preprocessing.sh using sct_run_batch even when the values are already mentioned in the include-list in the configuration.json. This PR helps the sct_run_batch to take this value directly from the configuration.json file.

Steps to reproduce this PR:

Data: duke/temp/rohan/bids_data
Config file values:

{
	"path_data": "PATH_TO_BIDS_DATA",
	"include-list": "sub-HarshmanDobby",
	"data_type": "anat",
	"contrast": "t2",
	"suffix_image": "_T2",
	"first_disc": "1",
	"last_disc": "26"
}

Run the preprocess_segment.sh using the sct_run_batch from the README of this PR.

Expected output

Running this script should create a temporary output folder with the segmentation and disc_level derivatives with the names:
{SUBJECT_NAME_CONTRAST}_label-SC_mask.nii.gz
{SUBJECT_NAME_CONTRAST}_labeled-discs.nii.gz

@rohanbanerjee rohanbanerjee changed the title updated preprocess_segment.sh for easier use Removal of hard-coded configuration file in preprocess_segment.sh script and extraction of include_list from the configuration file directly Aug 17, 2023
Copy link
Member

@joshuacwnewton joshuacwnewton left a comment

Choose a reason for hiding this comment

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

What you've done here looks good to me! 🎉

However, I had an additional idea that might make this even simpler:

  • I notice that you are forced to parse the extra keys from configuration.json yourself:

    PATH_DATA=$(echo "$json_data" | sed -n 's/.*"path_data": "\(.*\)".*/\1/p')
    DATA_TYPE=$(echo "$json_data" | sed -n 's/.*"data_type": "\(.*\)".*/\1/p')
    IMAGE_SUFFIX=$(echo "$json_data" | sed -n 's/.*"suffix_image": "\(.*\)".*/\1/p')
    CONTRAST=$(echo "$json_data" | sed -n 's/.*"contrast": "\(.*\)".*/\1/p')

  • This is awkward! But, I realize that it is necessary, because sct_run_batch ignores any "extra config keys" that do not match the original sct_run_batch arguments.

  • However, I thought of an alternative. You could write your configuration.json to use script_args directly:

    {
        "path_data": "/path/to/data/",
        "include_list": "sub-001 sub-002 sub-003",
        "#": "Args below: $DATA_TYPE, $CONTRAST, $IMAGE_SUFFIX, $LAST_DISC (parsed in script)"
        "script_args": ["anat", "t1", "_T1w", "26"]
    }

    Then, these arguments can be retrieved directly using $2, $3, etc., which removes the need to pass the configuration file twice, and removes the need to manually parse the configuration file.

    The only issue with this is that json files do not support comments. So, I added a "fake comment" to document what each of the "script_args" means. But maybe this is confusing? (Note: YAML does support comments, so that might be an option as well.)

If this is worse, then of course we can just keep the original way of doing this, too (since the json file is clearer as-is, I think).

README.md Show resolved Hide resolved
preprocess_segment.sh Show resolved Hide resolved
preprocess_segment.sh Show resolved Hide resolved
Copy link
Member

@joshuacwnewton joshuacwnewton left a comment

Choose a reason for hiding this comment

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

Thank you for responding to my feedback! LGTM, I think this is good to merge. :)

Copy link
Member

@jcohenadad jcohenadad left a comment

Choose a reason for hiding this comment

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

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants