Allow users to specify Content Label and ensure the value is formatted correctly #130
Conversation
Also ensure the value is formatted according to VR CS
|
@hackermd could you please clarify where the set of values you are using comes from? Is it from the standard or is it something you are proposing? |
There are no enumerated values defined in the standard for this attribute. This is something I am proposing after a conversation with @dclunie. I hope that these enumerated values give users some idea of what values could be used (note that users can specify their own custom values). |
|
I might avoid using the term "Enumerated Values" to describe these, since that implies that it does come from the standard and further that it is a required and fixed set of values, as opposed to being a set that is specific to this implementation, and only a suggestion. |
src/highdicom/ann/enum.py
Outdated
|
|
||
| class ContentLabelValues(Enum): | ||
|
|
||
| """Enumerated values for attribute Content Label.""" |
There was a problem hiding this comment.
I think it would be a good idea to make it clear in the documentation that these are suggested values and not part of the standard
There was a problem hiding this comment.
Removed enumerations via fa56561
src/highdicom/pm/enum.py
Outdated
|
|
||
| class ContentLabelValues(Enum): | ||
|
|
||
| """Enumerated values for attribute Content Label.""" |
There was a problem hiding this comment.
Again, I think it would be a good idea to make it clear in the documentation that these are suggested values and not part of the standard
There was a problem hiding this comment.
Removed enumerations via fa56561
src/highdicom/valuerep.py
Outdated
| 'Code string must not start with a number of an underscore.' | ||
| ) | ||
|
|
||
| if re.match(r'[A-Z0-9_]+[^-]$', value) is None: |
There was a problem hiding this comment.
From my reading of the standard, whitespace is allowed too:
Uppercase characters, "0"-"9", the SPACE character, and underscore "_", of the Default Character Repertoire
Also, I don't understand the need for the [^-], this will allow any non-hyphen character at the end, for example !, @, %, etc, which is not correct. I.e. INSTANCE_SEG!, INSTANCE_SEG?, INSTANCE_SEGs would match this currently...
Therefore I think it's something like
| if re.match(r'[A-Z0-9_]+[^-]$', value) is None: | |
| if re.match(r'[A-Z0-9_\s]+$', value) is None: |
There was a problem hiding this comment.
Further question: where is the requirement "Code string must not start with a number or an underscore" specified? It seems sensible, it's just not in Table 6.2-1
There was a problem hiding this comment.
I added a note to the function in c39000d.
I think we should encode those values such that they could be used as fields of an enumeration (in C or syntactically similar languages).
|
@CPBridge after further considerations, I removed the enumerations for the Content Label attribute, since these opinionated options may be more misleading than helpful. |
This PR adds a
content_labelattribute to theSegmentation,ParametricMapandMicroscopyBulkSimplyAnnotationSOP classes to allow users to set the value of the Content Label attribute.Since the meaning of that attribute is not immediately obvious (thanks @dclunie for clarification), this PR further adds enumerated values for specific SOP classes. However, users do not have to choose a value from the enum, but can also supply custom values. To ensure the values are formatted correctly, a
check_code_string()function is added that asserts values are formatted according to the constraints of the CS VR.