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

CLI default value appears to be ignored #498

Closed
fedorov opened this issue Jun 19, 2024 · 4 comments · Fixed by #499
Closed

CLI default value appears to be ignored #498

fedorov opened this issue Jun 19, 2024 · 4 comments · Fixed by #499
Labels

Comments

@fedorov
Copy link
Member

fedorov commented Jun 19, 2024

Despite "skip" flag assigned default value "true" in XML

https://github.com/QIICR/dcmqi/blame/master/apps/seg/itkimage2segimage.xml#L58-L65

the actual default value is set to 0

$ itkimage2segimage --help
[...]
   --skip
     Skip empty slices while encoding segmentation image. By default, empty
     slices will not be encoded, resulting in a smaller output file size.
     (default: 0)
@fedorov fedorov added the bug label Jun 19, 2024
fedorov added a commit that referenced this issue Jun 19, 2024
re #498

It appears that defaults are ignored for boolean SEM parameters. Even though
the parameter was set to true in https://github.com/QIICR/dcmqi/blob/2990dbd01d1fa61c16777950732ff19dc87318e3/apps/seg/itkimage2segimage.xml#L58-L65,
the default reported by the executable was 0.

Local tests show that integer parameters do not have this problem.
fedorov added a commit that referenced this issue Jun 19, 2024
re #498

It appears that defaults are ignored for boolean SEM parameters. Even though
the parameter was set to true in https://github.com/QIICR/dcmqi/blob/2990dbd01d1fa61c16777950732ff19dc87318e3/apps/seg/itkimage2segimage.xml#L58-L65,
the default reported by the executable was 0.

Local tests show that integer parameters do not have this problem.

Also switch to the most recent has for SlicerExecutionModel.
@fedorov fedorov linked a pull request Jun 19, 2024 that will close this issue
@lassoan
Copy link
Contributor

lassoan commented Jun 19, 2024

It is not possible to set default value for a Boolean flag. If you don't specify it then it will be always false (how else would you be able to set it to false?).

Default value is just a hint for GUI generation. I agree that the help text is confusing, it should be removed or expressed differently. For example, it could be (setting the flag is recommended).

@fedorov
Copy link
Member Author

fedorov commented Jun 19, 2024

I guess it may make sense, but it for sure confused me. I think ideally there should be some built-time check or validation that would alert the developer that <default> parameter for boolean is not allowed.

@fedorov
Copy link
Member Author

fedorov commented Jun 19, 2024

I think ideally there should be some built-time check or validation that would alert the developer that parameter for boolean is not allowed.

I see what you mean. This is super confusing. This essentially means that this specific parameter is only meaningful for GUI use of SEM, while the developer may not even be aware about GUI and only care about CLI use of the module. I understand it is not possible/difficult to fix.

@lassoan
Copy link
Contributor

lassoan commented Jun 19, 2024

The misleading help text is a bug, but that should be fixed (a simple change).

The other thing that causes confusion is that Boolean values are specified by just adding the parameter name (--myflag) instead of specifying parameter name+value (--myflag true or --myflag false). We could change the behavior but it would make the command-line interface a bit less convenient. Would it be sufficient to improve the documentation somewhere?

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

Successfully merging a pull request may close this issue.

2 participants