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

Add -owarpinv to sct_register_multimodal #3507

Merged
merged 6 commits into from Aug 25, 2021

Conversation

joshuacwnewton
Copy link
Member

@joshuacwnewton joshuacwnewton commented Aug 24, 2021

Checklist

GitHub

PR contents

Description

This PR add the ability to specify the filename of the output inverse warping field. This is relevant for the SCT Course, since we often would rename the files using mv (because the auto-generated filenames were long and awkward).

For this PR, I've tried to mimic the existing programming conventions in sct_register_multimodal (basically just matching -owarp without making any further improvements), just to keep the changes easier to understand/review.

Linked issues

Fixes #1530.

@joshuacwnewton joshuacwnewton added sct_register_multimodal context: feature category: new functionality labels Aug 24, 2021
@joshuacwnewton joshuacwnewton added this to the 5.4 milestone Aug 24, 2021
@joshuacwnewton joshuacwnewton self-assigned this Aug 24, 2021
Copy link
Member

@sandrinebedard sandrinebedard left a comment

Choose a reason for hiding this comment

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

I tested it out with -owarpinv and it works well! However, when I don't specify the flag, the inverse warping field is not created, but it should be... this may explain the batch pocessing test failling

@joshuacwnewton
Copy link
Member Author

I tested it out with -owarpinv and it works well! However, when I don't specify the flag, the inverse warping field is not created, but it should be... this may explain the batch pocessing test failling

Oops. There was a typo on my part. Sorry about that. 😅

Copy link
Member

@sandrinebedard sandrinebedard left a comment

Choose a reason for hiding this comment

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

Works well now! 🎉

One suggestion, we could also remove the lines renaming the warping fileds in batch_processing.sh and use -owarp and -owarpinv as we want to do in the tutorials 😊

@joshuacwnewton
Copy link
Member Author

joshuacwnewton commented Aug 24, 2021

One suggestion, we could also remove the lines renaming the warping fileds in batch_processing.sh and use -owarp and -owarpinv as we want to do in the tutorials 😊

Ahhh, that's a very good point! Thank you for the reminder. It's easy to forget. 😅

As an aside, it might be worth investigating how much overlap there is between batch_processing.sh and batch_single_subject.sh from the tutorials. If there was a way to use a single script for both purposes, that could save us some maintenance effort, since we could update the tutorials AND batch_processing.sh simultaneously in a single place.

@joshuacwnewton joshuacwnewton merged commit 666237a into master Aug 25, 2021
@joshuacwnewton joshuacwnewton deleted the jn/1530-register-multimodal-owarpinv branch August 25, 2021 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature category: new functionality sct_register_multimodal context:
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add -owarpinv argument to specify output name of inverse warping field
2 participants