Add properties and methods to SR content items and templates

[hackermd]

This will allow consistent access to values of content items independent of their value type. The only exception are content items of type CONTAINER, which I would argue don't have a value.

[CPBridge]

Looks nice! Some minor suggestions

[CPBridge]

Definitely recommend running this test and any test involving the pydicom DT class with the pydicom.config.datetime_conversion config parameter set to both True and False, as speaking from experience this can cause really frustrating errors if the end user changes the config value. This can be achieved nicely with a fixture, like I did in the pydicom tests here

[hackermd]

This will need a bit of work. We currently use unittest.TestCase, which is incompatible with pytest.fixture (see docs)

[hackermd]

This particular class wouldn't need to be subclassed from unittest.TestCase, but we should then change this across the board.

[CPBridge]

Hmm... I see. Well we already have several tests that do not use unittest.TestCase, for example those in test_valuerep and test_utils, so although it's a bit messy, it doesn't seem like a little more mix and match would hurt too much?

I've just been bitten too many times before by this datetime_conversion thing and really think having the test would be a good idea...

[hackermd]

Ok. Maybe we should just rip out unittest.TestCase. This caused problems before.

[CPBridge]

Perhaps, but I don't see any urgency.

[CPBridge]

I have been having a hard time with this. The fact that the ROIs can be encoded "per value" as well as "per reference" complicates things quite a bit. I like the idea of allowing the caller to filter groups by ROI value type (SCOORD, SCOORD3D, IMAGE, COMPOSITE).

I agree this is tricky. I don't think there is a perfect solution. I see a couple of problems with the proposed value_type based filtering:

• Ideally people should be able to use the library without having to understand how exactly the objects (ImageRegion, ReferencedSegment, etc) are encoded in DICOM under the hood. This suggestion would break that and require people to know which specific value type these types of regions are recorded using, which is not at all obvious. For example it is not at all clear without significant understanding of SRs that COMPOSITE would return ROI with a referenced real world value map. If we went forward with this, we would have to have a very good explanation in the docs I think.
• Filtering by value type does not allow you to distinguish between ImageRegion3D and VolumeSurface. This is probably not so serious, but potentially a bit confusing.

Two other options I see are:

• Introduce a new enum called ROIType with values image_region, image_region_3d, volume_surface, segment, real_world_value_map, and use that as a basis for filtering. This to me would be far more intuitive for users to understand. The disadvantage is that it creates an enum which does not really relate to anything in the standard.
• Have several different user-facing methods, something like:
  • get_image_region_measurement_groups
  • get_segment_measurement_groups
  • get_volume_surface_measurement_groups
  • ...
    They would probably all call the same method under the hood. This avoids the above problem and is possibly the most intuitive, but creates a lot of extra code. It may also allow other filtering options tuned specifically to each roi type within each of the methods (e.g. get_segment_measurement_groups could have a segment_number parameter).

Any thoughts?

Even trickier in my mind is the return type of the .roi property. What are you thinking there?

[hackermd]

@CPBridge I addressed most of your comments and included your suggestions. There are a few remaining open comments that we would need to discuss.

[CPBridge]

Merged to dev branch via command line to continue refinement there alongside #74