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

DTI metrics workflow: mask is optional, but crashes when no mask provided #1764

Closed
jchoude opened this issue Mar 13, 2019 · 6 comments · Fixed by #1835

Comments

@jchoude
Copy link
Contributor

commented Mar 13, 2019

Description

In the dipy_fit_dti workflow help message, the mask argument is said to be optional (default: no mask used). However, if no mask is provided, the script crashes and says that an argument is missing.

What should be the real behavior?

  • Make the argument really optional?
  • Leave it mandatory and change the help message?

I would prefer to have it optional, but I want your opinions.

@skoudoro @Garyfallidis

@arokem

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

+1 for refactoring to allow this to be optional

@skoudoro

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

What should be the real behavior?

The original behavior should be mandatory. The goal was to educate and give good habits to the user instead of having a blind mask generation.

But I think, we should discuss this point. Currently, it is mandatory for all models

@jchoude

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2019

Ok. I agree with not using a blind mask generation. However, I feel like in somes cases people might not want to use a mask.

@arokem

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

I guess you can always pass a volume full of 1's, if you want to do the entire volume. But that's a bit of a kludge.

@skoudoro

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

ok, maybe we can offer 2 optional parameters: --generate-mask (bool) - default false and mask_file (string) default None. If both parameters are set, priority to mask_file.

What do you think?

@jchoude

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Would the mask_file argument be in the optional section? If so, how does it handle having multiple masks (as for the syntax already supported for multiple dwi / bval / bvec)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.