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.

Sign up for GitHub

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

Jump to bottom

Add properties and methods to SR content items and templates #69

Closed
wants to merge 114 commits into from
Closed

Add properties and methods to SR content items and templates #69

wants to merge 114 commits into from

Conversation

hackermd
Copy link
Collaborator

@hackermd hackermd commented May 7, 2021

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.

@hackermd hackermd requested a review from CPBridge May 7, 2021 13:26
CPBridge
CPBridge reviewed May 11, 2021
View reviewed changes
Copy link
Collaborator

@CPBridge CPBridge left a comment

Choose a reason for hiding this comment

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

Looks nice! Some minor suggestions

src/highdicom/sr/value_types.py Outdated Show resolved Hide resolved
src/highdicom/sr/value_types.py Outdated Show resolved Hide resolved
tests/test_sr.py
@@ -377,6 +391,8 @@ def test_datetime_item_construction_from_datetime(self):
assert i.ValueType == 'DATETIME'
assert i.ConceptNameCodeSequence[0] == name
assert i.DateTime == DT(value)
assert i.name == CodedConcept(*name)
assert i.value == value
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@CPBridge CPBridge Jul 8, 2021 •
edited

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps, but I don't see any urgency.

@hackermd hackermd added the enhancement New feature or request label May 11, 2021
@hackermd hackermd marked this pull request as draft May 11, 2021 17:53
@hackermd hackermd requested a review from CPBridge May 11, 2021 17:53
@hackermd hackermd changed the title Add "value" property to SR content items Add properties and methods to SR content items and templates May 11, 2021
@hackermd hackermd mentioned this pull request May 14, 2021
Merged
@hackermd hackermd mentioned this pull request May 19, 2021
Open
@CPBridge
Copy link
Collaborator

CPBridge commented Jul 3, 2021

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 hackermd requested a review from CPBridge July 6, 2021 17:15
@hackermd
Copy link
Collaborator Author

hackermd commented Jul 6, 2021

@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 CPBridge changed the base branch from master to dev July 26, 2021 22:55
@CPBridge
Copy link
Collaborator

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

@CPBridge CPBridge closed this Jul 26, 2021
@CPBridge CPBridge mentioned this pull request Jul 26, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CPBridge CPBridge Awaiting requested review from CPBridge

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

SR templates do not support Image Library Should PixelOriginInterpetation be required?
3 participants
@hackermd @CPBridge @seandoyle