feat: restructure sr/templates → package; add Supplement 247 TIDs 6001/6004/6005 by oftalmos-org · Pull Request #407 · ImagingDataCommons/highdicom

oftalmos-org · 2026-04-11T05:04:26Z

Summary

Implements DICOM Supplement 247 "Eyecare Measurement Templates" (incorporated into PS3.16 2025c) and restructures highdicom/sr/templates from a single module into a package, as discussed in #355.

Templates added

TID	Class	Description
TID 6001	`OphthalmologyMeasurementsGroup`	Sub-template: single-eye measurement group; finding site constrained to Eye (81745001, SCT); laterality mandatory (Row 3 `>>` nested inside Finding Site item)
TID 6004	`CircumpapillaryRNFLKeyMeasurements`	Root template: circumpapillary RNFL thickness via OCT (CID 42x3)
TID 6005	`MacularThicknessKeyMeasurements`	Root template: ETDRS macular grid thickness via OCT (CID 42x4)

Package restructure (`sr/templates/`)

templates.py → templates/tid1500.py (rename only, no content changes)
templates/__init__.py re-exports all previous public names — fully backwards-compatible
templates/tid6000.py — new module for Supplement 247 TIDs

Provisional concept codes

Supplement 247 was balloted with several DCM codes shown as nnnXXX placeholders (final values pending WG-09 assignment). They are stored here under private scheme 99OPHTHALMO (≤ 16 chars, VR SH) with inline comments marking them as draft.

Codes pending a companion PR to pydicom (pydicom.sr.codedict):

Container titles: nnn102 ("RNFL Key Measurements"), nnn103 ("Macular Thickness Key Measurements")
CID 42x3 RNFL: nnn400–nnn404 (average/quadrant thicknesses), nnn406 (ROI radius)
CID 42x4 Macular: nnn250 ("Average macular thickness")
ETDRS grid (LOINC, stable, not yet in pydicom convenience dict): LN 57108-3 through LN 57118-2

No coding_ophthalmology.py was created, per CPBridge guidance in #406.

Tests

New file tests/test_sr_ophthalmic.py — 41 tests:

TestOphthalmologyMeasurementsGroup (14): laterality nesting (Row 3 >>), finding site default/custom, tracking identifier, multiple measurements, validation errors
TestCircumpapillaryRNFLKeyMeasurements (13): container code, template ID, bilateral, 99OPHTHALMO scheme check, full quadrant encoding
TestMacularThicknessKeyMeasurements (12): LOINC scheme check, uL unit for total volume, full 10-item ETDRS grid, bilateral
TestRoundtrip (2): TID 6004 and TID 6005 each serialised to ComprehensiveSR → dcmwrite → dcmread → values verified

All 41 new tests pass. All 294 pre-existing test_sr.py tests pass.

Validation

Validated with real clinical OCT data from a production Optopol Revo FC130 unit. CMT = 281.4 µm.

This PR also implements the templates/ package restructure discussed in #355 and designed by @CPBridge.

…1/6004/6005 Restructure ----------- - src/highdicom/sr/templates.py → src/highdicom/sr/templates/tid1500.py - New src/highdicom/sr/templates/__init__.py re-exports every name that was previously importable from highdicom.sr.templates (backwards-compat). - All 294 existing SR tests pass unchanged. Supplement 247 — Eyecare Measurement Templates ----------------------------------------------- New module src/highdicom/sr/templates/tid6000.py implements: TID 6001 OphthalmologyMeasurementsGroup Sub-template (analogous to TID 1501). Wraps one eye's measurements with mandatory Finding Site and Laterality (CID 4209 / CID 244). Container concept: (125007, DCM, "Measurement Group"). TID 6004 CircumpapillaryRNFLKeyMeasurements Root template. Wraps 1–2 TID 6001 groups with mandatory AlgorithmIdentification (TID 4019). CID 42x3 RNFL concept codes (nnn400–nnn404) are provisional (Sup247 draft) and encoded under scheme 99OPHTHALMO pending the parallel pydicom PR that will add the final CID entries. TID 6005 MacularThicknessKeyMeasurements Root template. Same structure as TID 6004. CID 42x4 macular measurements use finalised LOINC codes (57108-3 through 57118-2); average macular thickness (nnn250) is provisional. All three classes are re-exported from highdicom.sr (public API). Validated with real OCT data (Siltspook: CMT=288.49 µm, RNFL avg=121 µm). Reference: DICOM PS3.16 Supplement 247 (incorporated 2025c) https://dicom.nema.org/Dicom/News/January2025/docs/sups/sup247.pdf Closes: ImagingDataCommons#406 (draft)

…w 3 >>)

oftalmos-org · 2026-04-21T05:53:59Z

@CPBridge This PR is ready for review. All 41 new ophthalmic SR tests pass alongside the 294 existing test_sr.py tests. Happy to address any feedback.

CPBridge · 2026-04-22T14:09:46Z

Great! I will take a look at this soon, sorry if it takes a few weeks as I have a lot on my plate at the moment

CPBridge

Hi @oftalmos-org

I'm sorry it took me so long to get to this. The last few months have been very busy with personal things, but now I have my head around the templates and what you are trying to do here, I hope future iterations will be much quicker.

I have three high-level comments, and then a bunch of smaller ones below.

Firstly, and most importantly, it appears that Supplement 247 was approved into the standard in version 2025b (according to David Clunie's page). It seems your implementation was based on an earlier draft and there have been some changes. Most notably the TIDs have changed:
- TID 6001 in the draft become TID 2120
- TID 6004 became TID2123
- TID 6005 became TID2124
Would you please update this in line with the approved and released version of the standard?

Relatedly, I would quite like to avoid releasing the codes required as part of the public API of highdicom, since the intention would be to use them only temporarily. To that end, what is the status of the PRs to add required codes to pydicom? I took a look and was not able to see one.
TID 2120 (formally TID 6001 in the draft version) is described as "a proper subset of TID 1501 with some optional extensions". Highdicom already has class representing TID1501, namely highdicom.sr.MeasurementsAndQualitativeEvaluations, so I'm wondering whether this class should be a subclass of that class (or possibly a subclass of its base class highdicom.sr._MeasurementsAndQualitativeEvaluations). That would seem to remove some redundant code and allow this class to take advantage of some of the optional parts that are already implemented. It would also fix some of the other issues I highlight below
It took me a while to get my head around what I think is a new pattern in the standard of invoking templates with entire context groups defining the required measurements that must be present within a measurement group. It is a little tricky to fit this pattern into the way we built templates in the past. It seems that the approach that you have taken is to document which measurements are required within the top level classes (CircumpapillaryRNFLKeyMeasurement and MacularThicknessKeyMeasurements). I am concerned that this is insufficient, and that many (most?) SRs produced would likely be non-compliant due to a user failing to follow these requirements. This is at odds with our philosophy that if a highdicom constructor finishes successfully, the new object is compliant with the standard.

I see two potential ways around this:
1. We keep things more or less as they are currently structured, but add logic to the constructors of these two top level classes to check that the measurement groups passed to them meet all the requirements for the template, and give clear error messages if they do not. This would be easier to implement, but I do think putting the whole burden on the user to pass the right combination of parameters will be quite confusing to the user.
2. We implement one specialized subclass of the OpthalmologyMeasurementsGroup (or MeasurementsAndQualitativeEvaluations, in line with the second point above) for each invocation of that template. These classes would accept as parameters the measurement values and place them into measurements with the correct codes. We might want to remove some of other optional parameters of the Measurement (TID300) class, or assume they are shared between all measurements in the group, to simplify things a bit. Then the top-level classes accept these specialized subclasses, rather than any MeasurementGroup. While this would be more complex to implement (and there would certainly be more details to figure out), I think the result would be a much more simplified process for the user. What do you think?

CPBridge · 2026-06-07T19:56:39Z

+    ContainerContentItem,
+    ContentSequence,
+    TextContentItem,
+    UIDRefContentItem,


Unused import

Suggested change

UIDRefContentItem,

CPBridge · 2026-06-07T19:59:23Z

+#: UCUM microliter — used for macular total volume (``LN 57118-2``).
+#: Not exposed as a convenience attribute in pydicom 3.x; defined here until
+#: pydicom adds it to ``codes.UCUM``.
+UCUM_MICROLITER: Code = Code('uL', 'UCUM', 'microliter')


I would rather not have this as part of highdicom's public API.

CPBridge · 2026-06-07T20:27:19Z

+        laterality: CodedConcept | Code,
+        measurements: Sequence[Measurement],
+        finding_site: CodedConcept | Code | None = None,
+        tracking_identifier: str | None = None,


We already have an implementation of TID 4108 "Tracking Identifier" template in what is now tid1500.py. It would seem to make more sense to use that here, since it includes both the text and UID identifiers.

CPBridge · 2026-06-07T20:30:18Z

+        if finding_site is None:
+            finding_site = codes.cid4209.Eye


My reading of the template is that this value is fixed as "Eye" and it would be wrong to use anything else (this is the meaning of having an EV, =enumerated value in the value set constraint). So I think this should just be hardcoded with no finding_site parameter allowing the user to change it

CPBridge · 2026-06-07T22:31:16Z

+        measurements: Sequence[Measurement],
+        finding_site: CodedConcept | Code | None = None,
+        tracking_identifier: str | None = None,
+    ) -> None:


While generally I don't think we need to include every optional row of the template's table, I think the "source of measurement" (row 11) would be a particularly important one to include such that measurements can be traced back to the image in which they were made

CPBridge · 2026-06-07T22:44:59Z

+        if len(measurement_groups) > 2:
+            raise ValueError(
+                "Argument 'measurement_groups' must contain at most two items "
+                "(one per eye)."
+            )


Why this restriction? The VM for this row is 1-n and the notes explicitly state

TID 2120 may be invoked once for each eye, or multiple times for a trend series for a single eye

CPBridge · 2026-06-07T22:48:47Z

+
+    def __init__(
+        self,
+        algorithm_id: AlgorithmIdentification,


Since we already have a class for TID1002 ObserverContext, it would be straightforward to add this as an optional attribute

CPBridge · 2026-06-07T23:05:19Z

+# ---------------------------------------------------------------------------
+# TID 6004 (Sup247 TID 60x4) — Retinal Nerve Fiber Layer Key Measurements
+# ---------------------------------------------------------------------------


I would prefer not to have these superfluous "header" comments

CPBridge · 2026-06-07T23:14:41Z

+        if len(measurement_groups) > 2:
+            raise ValueError(
+                "Argument 'measurement_groups' must contain at most two items "
+                "(one per eye)."
+            )


As above, I don't think that this restriction is in line with the standard

CPBridge · 2026-06-07T23:39:40Z

+        for group in measurement_groups:
+            item.ContentSequence.extend(group)
+
+        super().__init__([item], is_root=True)


How about row 7 (the symmetry measurement)?

oftalmos-org · 2026-06-09T00:21:55Z

Hi @CPBridge ,

Thank you for the detailed review — and no worries about the time, I really appreciate you working through this carefully and myself as well can surely relate to having a busy mind/life.

I have mainly three responses:

TID renumbering: Confirmed, we were working from the draft. We will audit the full PR against the ratified 2025b TIDs (2120/2123/2124) and update all references before the next iteration. Regarding the pydicom PR for the required codes — we have not opened one yet. We will do that in parallel and link it here.
Inheritance from TID 1501: We agree that subclassing MeasurementsAndQualitativeEvaluations (or its base _MeasurementsAndQualitativeEvaluations) is the right approach since TID 2120 is defined as a proper subset of TID 1501. Before we commit to one or the other: do you have a preference between the public class and the private base? Our instinct is the private base gives more flexibility for the ophthalmic-specific extensions, but we want your guidance.
Conformance enforcement: We prefer Option B — specialized subclasses for each template invocation that accept measurement values directly (e.g. rnfl_superior, rnfl_inferior as float parameters) rather than pre-built Measurement objects. The principle that a successful constructor guarantees a compliant object is the right design goal and Option A would leave a confusing API. We understand this is more work and we are committed to it.

We will come back with the updated TIDs, the pydicom PR linked, and a revised architecture implementing Option B with proper inheritance from TID 1501.

Thanks again,
Noel

oftalmos-org added 4 commits April 10, 2026 19:47

fix: correct TID 6001 laterality nesting per Supplement 247 table (Ro…

4dc3de1

…w 3 >>)

test: add ophthalmic SR tests (TID 6001/6004/6005)

26c53bb

fix: remove redundant super().__init__() in TID 6004/6005 constructors

8c899dc

oftalmos-org marked this pull request as ready for review April 21, 2026 05:52

CPBridge requested changes Jun 8, 2026

View reviewed changes

Conversation

oftalmos-org commented Apr 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Templates added

Package restructure (sr/templates/)

Provisional concept codes

Tests

Validation

Uh oh!

oftalmos-org commented Apr 21, 2026

Uh oh!

CPBridge commented Apr 22, 2026

Uh oh!

CPBridge left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oftalmos-org commented Jun 9, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

oftalmos-org commented Apr 11, 2026 •

edited

Loading

Package restructure (`sr/templates/`)

CPBridge left a comment •

edited

Loading