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

ENH: Allow fixed,moving masks in scripts #1406

Merged
merged 1 commit into from
Jul 29, 2022
Merged

Conversation

cookpa
Copy link
Member

@cookpa cookpa commented Jul 28, 2022

This was kind of an undocumented feature because you could do -x fixedMask.nii.gz,movingMask.nii.gz and it would work. The script would change the arg to -x [ fixedMask.nii.gz,movingMask.nii.gz, NULL ].

I changed it so that -x fixedMask.nii.gz,movingMask.nii.gz maps to -x [ fixedMask.nii.gz,movingMask.nii.gz ] and -x fixedMask.nii.gz maps to -x [ fixedMask.nii.gz, NULL ] as before.

@gdevenyi
Copy link
Contributor

May I suggest changing NULL to NOMASK, this may reduce the previously seen bug reports.

@cookpa
Copy link
Member Author

cookpa commented Jul 28, 2022

Not easily becasue NULL isn't just a script convention, it has special meaning in the C++ code.

@gdevenyi
Copy link
Contributor

The way the code works in the masking inside antsRegistration seems to be

  • check if file exists and is loadable as a mask
  • if not loadable use no mask

Ergo, you can create a file called NULL and antsRegistration will use it (ignoring the complications of file format detection...)

@cookpa
Copy link
Member Author

cookpa commented Jul 28, 2022

Oh yeah, so it does. It gets the nullptr from attempting to read the file.

I guess then it could be changed. But now I'm thinking it would be better to parse NULL without printing the error message - because the code as written will run without a mask if there is a typo in the mask file name.

@gdevenyi
Copy link
Contributor

This would cause me much pain because I've been using NOMASK for a wide swath of scripts I'd need to update.

I stil support making it error out if it can't read the mask provided.

@cookpa cookpa merged commit 0effc09 into master Jul 29, 2022
@cookpa cookpa deleted the antsRegistrationSyNMask branch November 14, 2022 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants