Add coordinate for Measurement in SR #307

Fedalto · 2024-09-26T14:31:51Z

This adds support for row 3 and 4 from TID 320.

Resolves #306

This adds support for row 3 and 4 from TID 320. Resolves ImagingDataCommons#306

Fedalto · 2024-09-26T14:37:06Z

src/highdicom/sr/content.py

+            self,
+            graphic_type: Union[GraphicTypeValues, str],
+            graphic_data: np.ndarray,
+            source_image: SourceImageForRegion,


This has nothing to do with ImageRegion, but I reused SourceImageForRegion here as it needs to be an image with SELECTED FROM relationship. Not sure if there's a better option.

This is an interesting question. SourceImageForRegion pretty much matches the requirements of the ImageContentItem that would be used here, but the concept name used may pose a problem.

The concept name is blank in table TID 320, but the highdicom content item class does not allow "unnamed" content items (this issue arises in a few places and we made this design decision fairly early on). SourceImageForRegion uses the concept name (DCM, 111040, Original Source), whereas it states at the bottom of TID 320 that

(260753009, SCT, "Source") may be be used as a generic Concept Name when there is a desire to avoid having an anonymous (unnamed) Content Item

so we should follow that here, which implies that SourceImageForRegion as it is currently written would not be appropriate.

However, looking at the other places that SourceImageForRegion is used (basically row six of TID 1410 and TID 1411), I notice that the same suggestion to use SCT 260753009 is given there. I am not sure why SourceImageForRegion uses DCM 111040, it appears that this goes all the way back to the beginning of the library. Perhaps @hackermd can shed some light

Given this, I favour changing SourceImageForRegion to use SCT 260753009 and then using SourceImageForRegion as you propose for TID 320.

Fedalto · 2024-09-26T14:41:23Z

src/highdicom/sr/templates.py

@@ -2386,7 +2386,7 @@ def __init__(
        finding_sites: Optional[Sequence[FindingSite]] = None,
        method: Optional[Union[CodedConcept, Code]] = None,
        properties: Optional[MeasurementProperties] = None,
-        referenced_images: Optional[Sequence[SourceImageForMeasurement]] = None,
+        referenced_images: Optional[Sequence[Union[SourceImageForMeasurement, CoordinatesForMeasurement]]] = None,


I created another option and used the same referenced_images parameter.

Another solution I see would be to expand SourceImageForMeasurement and implement all options from TID 320 in it.

If a SCOORD is given, use row 3 and 4. If SCOORD3D, then make it use row 6 (I have not implemented this). If no coordinate is passed, use row 1 (which is the current SourceImageForMeasurement functionality)

Hmm I can see that we are a little backed into a corner here if we want to preserve backward compatibility. However I think passing coordinates to a parameter named referenced_images is rather non-intuitive.

I think I would instead suggest that we add a new parameter named referenced_coordinates, and allow passing either a sequence of CoordinatesForMeasurement (rows 3 and 4 of TID 320) or a sequence of CoordinatesForMeasurement3D (row 6), which has not been implemented yet but would be a relatively straightforward subclass of Scoord3DContentItem with a sensible default concept name.

Then you would need to document and enforce that using referenced_images and referenced_coordinates are mutually exclusive.

Then you would need to document and enforce that using referenced_images and referenced_coordinates are mutually exclusive.

I don't think this is the case.
See row 13 of TID 300. Measurement, it has VM 1-n, so I understand that referenced_images and referenced_coordinates can both be used inside a single Measurement.

Hmm, yes I guess you are right. They are mutually exclusive within TID 320, but there's no requirement that multiple uses of TID320 within TID300 have the same type

CPBridge

Hi @Fedalto, thanks for the nice PR!

Generally speaking, it's rather unfortunate that the way this was originally implemented (with only row 1 of TID 320 being used) makes it a little harder to implement this in a backwards-compatible manner.

A couple of other requests, if you are willing:

Ideally we would add a referenced_coordinates property to the Measurement class to retrieve the coordinates from the object? This would be very similar to how the referenced_images property works now (though, given some of other comments, I suppose it would be best not to specify a concept name, as in fact we probably should not have done for referenced_images).
Unfortunately the Measurement class not only implements TID 300 as it is used within TID 1501 but also rows 5 and after of TID 1491 (which in turn is used within TID 1410 and TID 1411). This was to simplify code and reduce duplication, but unfortunately the two are not exactly the same, and these coordinates are not included in TID 1419. I think this is why this is why these options were not included in the first place. Though technically the template is extensible, I think it would be best for the _ROIMeasurementsAndQualitativeEvaluations class to check any measurements it is passed do not contain SCOORD or SCOORD3D content items, since those templates are supposed to work rather differently (with the region defined as part of the TID 1410 and TID 1411 templates).

Note that I also target your PR at a new v0.24.0 branch rather than master.

CPBridge · 2024-09-29T23:19:46Z

src/highdicom/sr/content.py

+            self,
+            graphic_type: Union[GraphicTypeValues, str],
+            graphic_data: np.ndarray,
+            source_image: SourceImageForRegion,


This is an interesting question. SourceImageForRegion pretty much matches the requirements of the ImageContentItem that would be used here, but the concept name used may pose a problem.

The concept name is blank in table TID 320, but the highdicom content item class does not allow "unnamed" content items (this issue arises in a few places and we made this design decision fairly early on). SourceImageForRegion uses the concept name (DCM, 111040, Original Source), whereas it states at the bottom of TID 320 that

(260753009, SCT, "Source") may be be used as a generic Concept Name when there is a desire to avoid having an anonymous (unnamed) Content Item

so we should follow that here, which implies that SourceImageForRegion as it is currently written would not be appropriate.

However, looking at the other places that SourceImageForRegion is used (basically row six of TID 1410 and TID 1411), I notice that the same suggestion to use SCT 260753009 is given there. I am not sure why SourceImageForRegion uses DCM 111040, it appears that this goes all the way back to the beginning of the library. Perhaps @hackermd can shed some light

Given this, I favour changing SourceImageForRegion to use SCT 260753009 and then using SourceImageForRegion as you propose for TID 320.

CPBridge · 2024-09-29T23:31:02Z

src/highdicom/sr/content.py

+            name=CodedConcept(
+                value='121112',
+                meaning='Source of Measurement',
+                scheme_designator='DCM'
+            ),


I think hard-coding this is too restrictive. The template allows any purpose, and the note for row 13 in table TID 300 explicitly suggests an alternative ("Arm of Angle" in that case, which is clearly not general).

I would suggest that we add a purpose parameter, accepting a CodedConcept or pydicom.sr.coding.Code. I would probably make sense to have "Source of Measurement" as a default value to simplify things for users. (I know SourceImageForMeasurement does hard code the purpose, but I can see more cases where you would want to convey a more fine-grained purpose for coordinates).

src/highdicom/sr/templates.py

CPBridge · 2024-09-30T00:18:38Z

src/highdicom/sr/templates.py

@@ -2386,7 +2386,7 @@ def __init__(
        finding_sites: Optional[Sequence[FindingSite]] = None,
        method: Optional[Union[CodedConcept, Code]] = None,
        properties: Optional[MeasurementProperties] = None,
-        referenced_images: Optional[Sequence[SourceImageForMeasurement]] = None,
+        referenced_images: Optional[Sequence[Union[SourceImageForMeasurement, CoordinatesForMeasurement]]] = None,


Hmm I can see that we are a little backed into a corner here if we want to preserve backward compatibility. However I think passing coordinates to a parameter named referenced_images is rather non-intuitive.

I think I would instead suggest that we add a new parameter named referenced_coordinates, and allow passing either a sequence of CoordinatesForMeasurement (rows 3 and 4 of TID 320) or a sequence of CoordinatesForMeasurement3D (row 6), which has not been implemented yet but would be a relatively straightforward subclass of Scoord3DContentItem with a sensible default concept name.

Then you would need to document and enforce that using referenced_images and referenced_coordinates are mutually exclusive.

Co-authored-by: Chris Bridge <[email protected]>

Extract the `CoordinatesForMeasurement` in its own parameter. Add `CoordinatesForMeasurement.from_dataset`.

This follows the recommendation in the spec

CPBridge · 2024-10-03T21:24:59Z

@Fedalto is this ready for another review?

Fedalto · 2024-10-04T08:45:11Z

Most of it.

I just realized that TID 1411 also does not use TID 300, which I thought it did. So the check I did in 1410 (Planar ROI Measurements and Qualitative Evaluations) needs indeed to be in _ROIMeasurementsAndQualitativeEvaluations as you had suggested.
I'll make this change today and will let you know

…Evaluations` TID 1410 (Planar ROI Measurements and Qualitative Evaluations) and TID 1411 (Volumetric ROI Measurements and Qualitative Evaluations) make use of TID 1419 (ROI Measurements) which does not include coordinates like TID 300 (Measurements).

Fedalto · 2024-10-04T12:15:09Z

Hi @CPBridge. This should be ready for another review. I tried to separate each change request in each commit, so it should be easier to review commit by commit.

Thanks.

CPBridge

Hi @Fedalto, thanks for your work on this. There are a few comments above.

Also, if we are adding referenced coordinates I would prefer to add the SCOORD3D case at the same time (though I'm not concerned about the "R-INFERRED" variants, which honestly confuse me). Would you be happy to add this?

I think it would be pretty straightforward since it does not need the nested source image content item. We could just pass a plain Scoord3DContentItem into the Measurement init method, i.e. make referenced_coordinates of type Optional[Sequence[Union[CoordinatesForMeasurement, Scoord3DContentItem]]], and also have the corresponding property search for and return items of value type SCOORD or SCOORD3D.

But since this would force the user to pick a concept name/purpose and I think having sane defaults is extremely important in this highly complex SR stuff, I would actually lean towards defining a new CoordinatesForMeasurement3D class, which is basically just a Scoord3DContentItem derived class with a little default value for the purpose (just like the existing CoordinatesForMeasurement class. I'm also happy to work on this if you prefer.

CPBridge · 2024-10-05T18:09:00Z

src/highdicom/sr/content.py

+    ) -> None:
+        graphic_type = GraphicTypeValues(graphic_type)


Could you add a docstring for the __init__ method?

src/highdicom/sr/content.py

CPBridge · 2024-10-05T18:19:58Z

tests/test_sr.py

+                tracking_identifier=self._tracking_identifier,
+                referenced_region=self._region,
+                measurements=[measurement],
+            )


Can we add a test somewhere that the new referenced_coordinates property works as expected, and returns the right number of items and correct type?

CPBridge · 2024-10-05T18:48:25Z

src/highdicom/sr/templates.py

@@ -2431,6 +2433,8 @@ def __init__(
            and an indication of its selection from a set of measurements
        referenced_images: Union[Sequence[highdicom.sr.SourceImageForMeasurement], None], optional
            Referenced images which were used as sources for the measurement
+        referenced_coordinates: Union[Sequence[highdicom.sr.CoordinatesForMeasurement], None], optional


Oh one more thing, can we add a quick note to say that this is not valid for measurements that will later be included in Planar/Volumetric ROI Measurement groups?

Fedalto · 2024-10-15T13:45:52Z

Hi @CPBridge.
I made the requested changes. I believe this is ready for another review.
Thanks

CPBridge

Thanks @Fedalto, a nice contribution!

I have tweaked things a bit to use the CoordinatesForMeasurement3D class instead of the Scoord3DContentItem as this will be less confusing for users I think. However a Scoord3DContentItem should still work

src/highdicom/sr/templates.py

tests/test_sr.py

Add coordinate for Measurement in SR

65eaf3b

This adds support for row 3 and 4 from TID 320. Resolves ImagingDataCommons#306

Fedalto mentioned this pull request Sep 26, 2024

Add SCOORD to a Measurement in SR (TID 320) #306

Closed

Fedalto commented Sep 26, 2024

View reviewed changes

CPBridge changed the base branch from master to v0.24.0dev September 30, 2024 00:52

CPBridge requested changes Sep 30, 2024

View reviewed changes

Fedalto and others added 4 commits October 3, 2024 06:58

fix import style

73452a1

Co-authored-by: Chris Bridge <[email protected]>

Add Measurement.referenced_coordinates parameter

ab6bb55

Extract the `CoordinatesForMeasurement` in its own parameter. Add `CoordinatesForMeasurement.from_dataset`.

Add purpose parameter to CoordinatesForMeasurement.__init__

867cee4

Change SourceImageForRegion concept name to codes.SCT.Source

81dd8fb

This follows the recommendation in the spec

Fedalto force-pushed the add-coordinates-measurements-sr branch from 3580c65 to 9f6d673 Compare October 4, 2024 12:13

CPBridge requested changes Oct 5, 2024

View reviewed changes

CPBridge reviewed Oct 5, 2024

View reviewed changes

Add CoordinatesForMeasurement3D

f97fe3a

Change casting to use cls

1012fad

CPBridge approved these changes Oct 19, 2024

View reviewed changes

Favour CoordinatesForMeasurement3D over Scoord3DContentItem

3aca8ff

CPBridge merged commit ded30b1 into ImagingDataCommons:v0.24.0dev Oct 19, 2024

Fedalto deleted the add-coordinates-measurements-sr branch October 24, 2024 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add coordinate for Measurement in SR #307

Add coordinate for Measurement in SR #307

Fedalto commented Sep 26, 2024

Fedalto Sep 26, 2024

CPBridge Sep 29, 2024

Fedalto Sep 26, 2024

CPBridge Sep 30, 2024 •

edited

Loading

Fedalto Oct 3, 2024

CPBridge Oct 3, 2024

CPBridge left a comment

CPBridge Sep 29, 2024

CPBridge Sep 29, 2024

CPBridge Sep 30, 2024 •

edited

Loading

CPBridge commented Oct 3, 2024

Fedalto commented Oct 4, 2024

Fedalto commented Oct 4, 2024 •

edited

Loading

CPBridge left a comment

CPBridge Oct 5, 2024

CPBridge Oct 5, 2024

CPBridge Oct 5, 2024

Fedalto commented Oct 15, 2024

CPBridge left a comment

Add coordinate for Measurement in SR #307

Add coordinate for Measurement in SR #307

Conversation

Fedalto commented Sep 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPBridge Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPBridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CPBridge Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

CPBridge commented Oct 3, 2024

Fedalto commented Oct 4, 2024

Fedalto commented Oct 4, 2024 • edited Loading

CPBridge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fedalto commented Oct 15, 2024

CPBridge left a comment

Choose a reason for hiding this comment

CPBridge Sep 30, 2024 •

edited

Loading

CPBridge Sep 30, 2024 •

edited

Loading

Fedalto commented Oct 4, 2024 •

edited

Loading