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

Allow users to specify Content Label and ensure the value is formatted correctly #130

Merged
merged 7 commits into from
Nov 5, 2021
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/highdicom/ann/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from highdicom.ann.enum import (
AnnotationCoordinateTypeValues,
AnnotationGroupGenerationTypeValues,
ContentLabelValues,
GraphicTypeValues,
PixelOriginInterpretationValues,
)
Expand All @@ -16,6 +17,7 @@
'AnnotationCoordinateTypeValues',
'AnnotationGroup',
'AnnotationGroupGenerationTypeValues',
'ContentLabelValues',
'GraphicTypeValues',
'Measurements',
'MicroscopyBulkSimpleAnnotations',
Expand Down
7 changes: 7 additions & 0 deletions src/highdicom/ann/enum.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,3 +119,10 @@ class PixelOriginInterpretationValues(Enum):
(1,1) pixel of the Total Pixel Matrix of the entire image.

"""


class ContentLabelValues(Enum):

"""Enumerated values for attribute Content Label."""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed enumerations via fa56561


DETECTED_OBJECTS = 'DETECTED_OBJECTS'
hackermd marked this conversation as resolved.
Show resolved Hide resolved
16 changes: 14 additions & 2 deletions src/highdicom/ann/sop.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,14 @@
from highdicom.ann.enum import (
AnnotationCoordinateTypeValues,
AnnotationGroupGenerationTypeValues,
ContentLabelValues,
GraphicTypeValues,
PixelOriginInterpretationValues,
)
from highdicom.ann.content import AnnotationGroup
from highdicom.base import SOPClass
from highdicom.sr.coding import CodedConcept
from highdicom.valuerep import check_person_name
from highdicom.valuerep import check_person_name, check_code_string


class MicroscopyBulkSimpleAnnotations(SOPClass):
Expand All @@ -50,6 +51,7 @@ def __init__(
str,
PixelOriginInterpretationValues
] = PixelOriginInterpretationValues.VOLUME,
content_label: Optional[Union[str, ContentLabelValues]] = None,
**kwargs: Any
) -> None:
"""
Expand Down Expand Up @@ -90,6 +92,8 @@ def __init__(
transfer_syntax_uid: str, optional
UID of transfer syntax that should be used for encoding of
data elements.
content_label: Union[str, highdicom.ann.ContentLabelValues, None], optional
Content label
**kwargs: Any, optional
Additional keyword arguments that will be passed to the constructor
of `highdicom.base.SOPClass`
Expand Down Expand Up @@ -139,7 +143,15 @@ def __init__(
self.copy_patient_and_study_information(src_img)

# Microscopy Bulk Simple Annotations
self.ContentLabel = 'Annotation'
if content_label is not None:
try:
content_label = ContentLabelValues(content_label)
self.ContentLabel = content_label.value
except ValueError:
check_code_string(content_label)
self.ContentLabel = content_label
else:
self.ContentLabel = 'ANN'
self.ContentDescription = content_description
if content_creator_name is not None:
check_person_name(content_creator_name)
Expand Down
4 changes: 3 additions & 1 deletion src/highdicom/pm/__init__.py
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
"""Package for creation of Parametric Map instances."""

from highdicom.pm.sop import ParametricMap
from highdicom.pm.enum import ContentLabelValues
from highdicom.pm.content import DimensionIndexSequence, RealWorldValueMapping
from highdicom.pm.sop import ParametricMap

SOP_CLASS_UIDS = {
'1.2.840.10008.5.1.4.1.1.30', # Parametric Map
}

__all__ = [
'ContentLabelValues',
'DimensionIndexSequence',
'ParametricMap',
'RealWorldValueMapping',
Expand Down
12 changes: 12 additions & 0 deletions src/highdicom/pm/enum.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
"""Enumerate values specific to the Parametric Map IOD."""
from enum import Enum


class ContentLabelValues(Enum):

"""Enumerated values for attribute Content Label."""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed enumerations via fa56561


ACTIVATION_MAP = 'ACTIVATION_MAP'
ATTENTION_MAP = 'ATTENTION_MAP'
FEATURE_MAP = 'FEATURE_MAP'
SALIENCY_MAP = 'SALIENCY_MAP'
hackermd marked this conversation as resolved.
Show resolved Hide resolved
17 changes: 15 additions & 2 deletions src/highdicom/pm/sop.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,10 @@
)
from highdicom.enum import CoordinateSystemNames
from highdicom.frame import encode_frame
from highdicom.pm.enum import ContentLabelValues
from highdicom.pm.content import RealWorldValueMapping
from highdicom.pm.content import DimensionIndexSequence
from highdicom.valuerep import check_person_name
from highdicom.valuerep import check_person_name, check_code_string
from pydicom import Dataset
from pydicom.uid import (
UID,
Expand Down Expand Up @@ -63,6 +64,7 @@ def __init__(
pixel_measures: Optional[PixelMeasuresSequence] = None,
plane_orientation: Optional[PlaneOrientationSequence] = None,
plane_positions: Optional[Sequence[PlanePositionSequence]] = None,
content_label: Optional[Union[str, ContentLabelValues]] = None,
**kwargs,
):
"""
Expand Down Expand Up @@ -181,6 +183,8 @@ def __init__(
number of frames in `source_images` (in case of multi-frame source
images) or the number of `source_images` (in case of single-frame
source images).
content_label: Union[str, highdicom.pm.ContentLabelValues, None], optional
Content label
**kwargs: Any, optional
Additional keyword arguments that will be passed to the constructor
of `highdicom.base.SOPClass`
Expand Down Expand Up @@ -356,7 +360,16 @@ def __init__(
self.RecognizableVisualFeatures = 'YES'
else:
self.RecognizableVisualFeatures = 'NO'
self.ContentLabel = 'ISO_IR 192' # UTF-8

if content_label is not None:
try:
content_label = ContentLabelValues(content_label)
self.ContentLabel = content_label.value
except ValueError:
check_code_string(content_label)
self.ContentLabel = content_label
else:
self.ContentLabel = 'MAP'
self.ContentDescription = content_description
if content_creator_name is not None:
check_person_name(content_creator_name)
Expand Down
3 changes: 2 additions & 1 deletion src/highdicom/seg/__init__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
"""Package for creation of Segmentation (SEG) instances."""
from highdicom.seg.sop import Segmentation, segread
from highdicom.seg.enum import (
ContentLabelValues,
SegmentAlgorithmTypeValues,
SegmentationTypeValues,
SegmentationFractionalTypeValues,
Expand All @@ -15,10 +16,10 @@

SOP_CLASS_UIDS = {
'1.2.840.10008.5.1.4.1.1.66.4', # Segmentation
'1.2.840.10008.5.1.4.1.1.66.5', # Surface Segmentation
}

__all__ = [
'ContentLabelValues',
'DimensionIndexSequence',
'Segmentation',
'segread',
Expand Down
5 changes: 3 additions & 2 deletions src/highdicom/seg/content.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,9 @@ def __init__(
Anatomic region(s) into which segment falls,
e.g. ``Code("41216001", "SCT", "Prostate")``
(see :dcm:`CID 4 <part16/sect_CID_4.html>`
"Anatomic Region", :dcm:`CID 4031 <part16/sect_CID_4031.html>` "Common Anatomic Regions", as
as well as other CIDs for domain-specific anatomic regions)
"Anatomic Region", :dcm:`CID 4031 <part16/sect_CID_4031.html>`
"Common Anatomic Regions", as as well as other CIDs for
domain-specific anatomic regions)
primary_anatomic_structures: Union[Sequence[Union[highdicom.sr.Code, highdicom.sr.CodedConcept]], None], optional
Anatomic structure(s) the segment represents
(see CIDs for domain-specific primary anatomic structures)
Expand Down
8 changes: 8 additions & 0 deletions src/highdicom/seg/enum.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,11 @@ class SegmentsOverlapValues(Enum):
YES = 'YES'
UNDEFINED = 'UNDEFINED'
NO = 'NO'


class ContentLabelValues(Enum):

"""Enumerated values for attribute Content Label."""
hackermd marked this conversation as resolved.
Show resolved Hide resolved

INSTANCE_SEG = 'INSTANCE_SEG'
SEMANTIC_SEG = 'SEMANTIC_SEG'
hackermd marked this conversation as resolved.
Show resolved Hide resolved
17 changes: 15 additions & 2 deletions src/highdicom/seg/sop.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
SegmentDescription,
)
from highdicom.seg.enum import (
ContentLabelValues,
SegmentationFractionalTypeValues,
SegmentationTypeValues,
SegmentsOverlapValues,
Expand All @@ -47,7 +48,7 @@
)
from highdicom.seg.utils import iter_segments
from highdicom.sr.coding import CodedConcept
from highdicom.valuerep import check_person_name
from highdicom.valuerep import check_person_name, check_code_string
from highdicom.uid import UID as hd_UID


Expand Down Expand Up @@ -86,6 +87,7 @@ def __init__(
plane_orientation: Optional[PlaneOrientationSequence] = None,
plane_positions: Optional[Sequence[PlanePositionSequence]] = None,
omit_empty_frames: bool = True,
content_label: Optional[Union[str, ContentLabelValues]] = None,
**kwargs: Any
) -> None:
"""
Expand Down Expand Up @@ -232,6 +234,8 @@ def __init__(
omit_empty_frames: bool, optional
If True (default), frames with no non-zero pixels are omitted from
the segmentation image. If False, all frames are included.
content_label: Union[str, highdicom.seg.ContentLabelValues, None], optional
Content label
**kwargs: Any, optional
Additional keyword arguments that will be passed to the constructor
of `highdicom.base.SOPClass`
Expand Down Expand Up @@ -369,7 +373,16 @@ def __init__(
self.SamplesPerPixel = 1
self.PhotometricInterpretation = 'MONOCHROME2'
self.PixelRepresentation = 0
self.ContentLabel = 'Segmentation'

if content_label is not None:
try:
content_label = ContentLabelValues(content_label)
self.ContentLabel = content_label.value
except ValueError:
check_code_string(content_label)
self.ContentLabel = content_label
else:
self.ContentLabel = 'SEG'
self.ContentDescription = content_description
if content_creator_name is not None:
check_person_name(content_creator_name)
Expand Down
44 changes: 43 additions & 1 deletion src/highdicom/valuerep.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
"""Functions for working with DICOM value representations."""
import re
from typing import Union

from pydicom.valuerep import PersonName


def check_person_name(person_name: Union[str, PersonName]) -> None:
"""Check for problems with person name strings.
"""Check the value representation person name strings.

The DICOM Person Name (PN) value representation has a specific format with
multiple components (family name, given name, middle name, prefix, suffix)
Expand Down Expand Up @@ -59,3 +60,44 @@ def check_person_name(person_name: Union[str, PersonName]) -> None:
'name is really intended, add a trailing caret character to '
'disambiguate the name.'
)


def check_code_string(value: str) -> None:
hackermd marked this conversation as resolved.
Show resolved Hide resolved
"""Check the value representation person name strings.

Parameters
----------
value: str
Code string

Raises
------
TypeError
When `value` is not a string
ValueError
When `value` has zero or more than 16 characters or when `value`
contains characters that are invalid for the value representation

"""
if not isinstance(value, str):
raise TypeError('Invalid type for a code string.')

if len(value) == 0:
raise ValueError('Code string must not be empty.')

if len(value) > 16:
raise ValueError('Code string must have maximally 16 characters.')

if not value.isupper():
raise ValueError('Code string must be all upper case.')

if re.match(r'[0-9_]', value) is not None:
raise ValueError(
'Code string must not start with a number of an underscore.'
hackermd marked this conversation as resolved.
Show resolved Hide resolved
)

if re.match(r'[A-Z0-9_]+[^-]$', value) is None:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
if re.match(r'[A-Z0-9_]+[^-]$', value) is None:
if re.match(r'[A-Z0-9_\s]+$', value) is None:

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

raise ValueError(
'Code string must contain only capital letters, numbers, or '
'underscores.'
hackermd marked this conversation as resolved.
Show resolved Hide resolved
)
5 changes: 4 additions & 1 deletion tests/test_pm.py
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ def test_multi_frame_sm_image_single_native(self):
intercept=0,
slope=1
)
content_label = 'MY_MAP'
pmap = hd.pm.ParametricMap(
[self._sm_image],
pixel_array,
Expand All @@ -245,7 +246,8 @@ def test_multi_frame_sm_image_single_native(self):
contains_recognizable_visual_features=False,
real_world_value_mappings=[real_world_value_mapping],
window_center=window_center,
window_width=window_width
window_width=window_width,
content_label=content_label
)
assert pmap.SOPClassUID == '1.2.840.10008.5.1.4.1.1.30'
assert pmap.SOPInstanceUID == self._sop_instance_uid
Expand All @@ -257,6 +259,7 @@ def test_multi_frame_sm_image_single_native(self):
assert pmap.DeviceSerialNumber == self._device_serial_number
assert pmap.StudyInstanceUID == self._sm_image.StudyInstanceUID
assert pmap.PatientID == self._sm_image.PatientID
assert pmap.ContentLabel == content_label
assert np.array_equal(pmap.pixel_array, pixel_array)

def test_multi_frame_sm_image_ushort_native(self):
Expand Down
25 changes: 24 additions & 1 deletion tests/test_seg.py
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,7 @@ def setUp(self):
self._device_serial_number = '1-2-3'
self._content_description = 'Test Segmentation'
self._content_creator_name = 'Robo^Doc'
self._content_label = 'MY_SEG'

# A single CT image
self._ct_image = dcmread(
Expand Down Expand Up @@ -773,7 +774,8 @@ def test_construction(self):
self._manufacturer,
self._manufacturer_model_name,
self._software_versions,
self._device_serial_number
self._device_serial_number,
content_label=self._content_label
)
assert instance.SeriesInstanceUID == self._series_instance_uid
assert instance.SeriesNumber == self._series_number
Expand All @@ -798,6 +800,7 @@ def test_construction(self):
assert instance.SegmentationType == 'FRACTIONAL'
assert instance.SegmentationFractionalType == 'PROBABILITY'
assert instance.MaximumFractionalValue == 255
assert instance.ContentLabel == self._content_label
assert instance.ContentDescription is None
assert instance.ContentCreatorName is None
with pytest.raises(AttributeError):
Expand Down Expand Up @@ -1636,6 +1639,26 @@ def test_construction_empty_source_image(self):
device_serial_number=self._device_serial_number
)

def test_construction_invalid_content_label(self):
with pytest.raises(ValueError):
Segmentation(
source_images=[self._ct_image],
pixel_array=self._ct_pixel_array,
segmentation_type=SegmentationTypeValues.FRACTIONAL.value,
segment_descriptions=(
self._segment_descriptions
),
series_instance_uid=self._series_instance_uid,
series_number=self._series_number,
sop_instance_uid=self._sop_instance_uid,
instance_number=self._instance_number,
manufacturer=self._manufacturer,
manufacturer_model_name=self._manufacturer_model_name,
software_versions=self._software_versions,
device_serial_number=self._device_serial_number,
content_label='invalid-content-label'
)

def test_construction_mixed_source_series(self):
with pytest.raises(ValueError):
Segmentation(
Expand Down
Loading