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.

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

Fixes to dimension indices #92

Merged
merged 18 commits into from
Jul 30, 2021
Merged

Fixes to dimension indices #92

merged 18 commits into from
Jul 30, 2021

Conversation

CPBridge
Copy link
Collaborator

This PR fixes a few known issues with the way highdicom deals with the dimension indices in the Segmentation Image IOD. The following changes are implemented:

  • Dimension Organization UIDs are now created for every segmentation image, rather than the old, incorrect behaviour of using a fixed literal UID for every instance highdicom creates.
  • A fix to the way dimension index values are calculated: previously this was wrong if there were empty frames in the segmentation pixel array
  • In order to make the above fix, it was necessary to remove the add_segments() method of the Segmentation class because information about all segments is required when the instance is created in order to correctly calculate the dimension index value for every segment. To make this possible, the constructor can now accept a 4D array, where multiple segments (that would have previously been added sequentially via add_segments) may be concatenated along the fourth (final) dimension. This has a nice side effect of bringing the constructor in line with the "decoding" behaviour in Segmentation Parsing #74, which returns 4D arrays with the same interpretation.
  • An option to turn off the behaviour that omits empty frames from the segmentation image was added
  • More unit tests to ensure that the dimension index values are correct

@CPBridge CPBridge requested a review from hackermd July 26, 2021 22:31
@CPBridge
Copy link
Collaborator Author

Also added a release notes page to the docs that should guide users through the backwards-incompatible removal of add_segments. Calling add_segments now raises a deprecation error pointing to that page

@CPBridge CPBridge added bug Something isn't working invalid This doesn't seem right labels Jul 26, 2021
Copy link
Collaborator

@hackermd hackermd left a comment

Choose a reason for hiding this comment

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

Thanks @CPBridge. Looks nice! The constructor has become quite a beast now and I am wondering whether we could factor some of the code out into separate methods. It's tricky because instance attributes are being set all over the place, but some parts could probably be separated into functions with no (or only very few) side effects. What do you think?

src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
@CPBridge
Copy link
Collaborator Author

@hackermd I have moved some of the low hanging fruit to methods, unfortunately I don't think there's much more that can be done without passing huge parameter list around

@hackermd
Copy link
Collaborator

@hackermd I have moved some of the low hanging fruit to methods, unfortunately I don't think there's much more that can be done without passing huge parameter list around

Yeah, I also found this difficult on other classes. The changes that you've made are nice. The constructor has already lost quite a bit of weight.

Copy link
Collaborator

@hackermd hackermd left a comment

Choose a reason for hiding this comment

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

Looks very! Thanks @CPBridge. I only have one minor additional suggestion.

src/highdicom/seg/sop.py Outdated Show resolved Hide resolved
Co-authored-by: Markus D. Herrmann <hackermd@users.noreply.github.com>
@hackermd hackermd merged commit 743f23e into master Jul 30, 2021
@hackermd hackermd deleted the bug/dimension_index_sequence branch July 30, 2021 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working invalid This doesn't seem right
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants