Add coordinate for Measurement in SR

[Fedalto]

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

Resolves #306

[Fedalto]

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.

[CPBridge]

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]

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)

[CPBridge]

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.

[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]

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]

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

[CPBridge]

style: please wrap to one per line

Suggested change

	CoordinatesForMeasurement, FindingSite,
	CoordinatesForMeasurement,
	FindingSite,

[CPBridge]

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.