ENH: Change from Debug to Warning macro. #4926
Conversation
github-actions
bot
added
type:Enhancement
| itkDebugMacro("GDCMImageIO: non-DICOM and non-ITK standard key = " << key); | ||
| itkWarningMacro( | ||
| "ignoring non-DICOM and non-ITK standard key (DICOM key structure is group|element with pipe separator) = " | ||
| << key); |
There was a problem hiding this comment.
- Thanks @zivy I'm just wondering, recent pull requests seem to indicate that users want less warnings, rather than more: ENH: remove repetitive monochorme1 warning #4922 and ENH: Suppress irrelevant TIFF warnings #4909 So are you sure this one won't cause too much noise to the user?
If the problem is specifically the accidental use of a comma as a separator, would it help to check if the key has any comma inside, before producing the warning?
There was a problem hiding this comment.
Should there be a way to suppress this warning? If someone knows that not their entire metadata dictionary will be written, but don't want a warning (and don't want to clean it up)?
There was a problem hiding this comment.
By the way, I wonder why the common (XXXX,XXXX) format isn't supported anyway 🤷
There was a problem hiding this comment.
GDCM has three methods to parse the tags, only one is currently being used.
https://github.com/InsightSoftwareConsortium/ITK/blob/master/Modules/ThirdParty/GDCM/src/gdcm/Source/DataStructureAndEncodingDefinition/gdcmTag.h#L244-L259
There was a problem hiding this comment.
I'd also recommend 1 warning per file not per tag. That is something like "the following tags are non-standard and are being ignored: ...."
There was a problem hiding this comment.
There would still be the issue of the GDCM not supporting parenthesis, which are commonly used too.
There was a problem hiding this comment.
I would still like to keep the warning, but switch it to one per file.
There was a problem hiding this comment.
Thanks @zivy Of course, if we support both pipe and comma separators, there might be a tiny little chance that the very same DICOM tag is represented in two ways XXXX,XXXX and XXXX|XXXX, in one and the same image. But I guess that would be a very rare mistake, that we may ignore. Right?
There was a problem hiding this comment.
I will change the PR so that the warning is per file and not per tag and that both pipe and comma are acceptable separators.
@N-Dekker, you raised a valid corner case which I'd like to ignore for now given the expected rarity (will add it to the documentation instead of directly dealing with it and revisit in the future).
There was a problem hiding this comment.
Please add testing of the new functionality.
| itkDebugMacro("GDCMImageIO: non-DICOM and non-ITK standard key = " << key); | ||
| itkWarningMacro( | ||
| "ignoring non-DICOM and non-ITK standard key (DICOM key structure is group|element with pipe separator) = " | ||
| << key); |
There was a problem hiding this comment.
Should there be a way to suppress this warning? If someone knows that not their entire metadata dictionary will be written, but don't want a warning (and don't want to clean it up)?
DICOM group-element separator can be either comma or pipe, accept both. Also, when non DICOM and non standard ITK tags are in the metadata dictionary, issue a single warning per file.
github-actions
bot
added
the
type:Testing
|
Updated and force pushed. Writing supports both comma and pipe separators and added testing. Non-DICOM and non-ITK metadata in dictionary trigger a single warning per file which lists all these unexpected dictionary keys. |
When non DICOM and non standard ITK tags are in the metadata dictionary, issue a warning. This addresses the common mistake of using the comma as a separator in the group-element key instead of the pipe, which is what ITK/GDCM uses. These metadata dictionary entries were silently ignored. The warning should provide the user with a hint about what is happening.