search

QIICR / dcmqi

dcmqi (DICOM for Quantitative Imaging) is a free, open source C++ library for conversion between imaging research formats and the standard DICOM representation for image analysis results
https://qiicr.gitbook.io/dcmqi-guide/
BSD 3-Clause "New" or "Revised" License
228 stars 62 forks source link

CLI default value appears to be ignored #498

Closed fedorov closed 1 month ago

fedorov commented 1 month ago

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)
lassoan commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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?