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

_check_and_cast_pixel_array issue with non-contiguous numbering of segments #316

Closed
kavmar opened this issue Jan 3, 2025 · 8 comments
Closed
Assignees
Labels
wontfix This will not be worked on

Comments

@kavmar
Copy link

kavmar commented Jan 3, 2025

Hi,

I am using highdicom-0.23.1 (python-3.12) to create a DICOM SEG from a 3D numpy array where pixel values of individual labels are not contiguous, e.g. 0, 1, 10, 234, 556. This is not uncommon, e.g. FreeSurfer. Now the sop.py

if max_pixel > number_of_segments:

is complaining that there are missing SegmenetDescriptions, because number of classes is smaller than max value of the array 556 > 5
Wouldn't this be more accurate using len(np.unique(pixel_array))?

Thanks

@CPBridge
Copy link
Collaborator

CPBridge commented Jan 3, 2025

Hi @kavmar thanks for the question.

This is deliberate. The definition of Segment Number in the DICOM standard states that

Segment Number (0062,0004) shall start at a value of 1, and increase monotonically by 1

So this check enforces the standard. Arguably it is unfortunate that it has this limitation since other tools frequently do not, as you say. But that's how it is.

So basically you have two choices:

  • You can either provide segment descriptions for all the segments that are "missing" (there is no requirement that all described segments be present). However this would explicitly imply to the recipient of the file that these segments are not present in the (whereas omitting the segment description implies the presence of the segment was not assessed)
  • You can renumber the segments that you do wish to include to satisfy the requirement. If you like you can include your "original" segment number in the free text Segment Label.

Generally I would not rely on segment number to communicate the meaning of the segment when encoding things in DICOM, as this is very application specific. Better to rely on the standard terminologies used within the segmented property category and type and map to and from application-specific segment numbers when reading and writing the files, this will be much better for interoperability.

Hope this helps

@CPBridge CPBridge added the wontfix This will not be worked on label Jan 3, 2025
@CPBridge CPBridge self-assigned this Jan 3, 2025
@CPBridge
Copy link
Collaborator

CPBridge commented Jan 3, 2025

P.s. I just remembered that the newly defined labelmap segmentations do not have this constraint! The next release of highdicom will include our implementation of this,and I'm hoping to get it out over the next couple of weeks. So you could just wait for that (it will also be a much more efficient representation of your mask but will not be widely supported by other tools yet)

@CPBridge
Copy link
Collaborator

CPBridge commented Jan 3, 2025

@kavmar I am a bit confused. Are you not hitting this error first:

if not (np.diff(described_segment_numbers) == 1).all():
?

Maybe I have misunderstood the issue here...

@kavmar
Copy link
Author

kavmar commented Jan 4, 2025

Hi Chris,

in order for me to progress in my prototyping, I simply commented out the lines 2729 - 2733 and all worked fine. But indeed 2395 was not a blocker for me.

Thanks for clarification from your earlier comments.

Martin

@CPBridge
Copy link
Collaborator

CPBridge commented Jan 4, 2025

Well that is certainly concerning. If you tried to create a segmentation with non-consecutive segment numbers, it should have been caught by the test in line 2395 and this should have given you a more intuitive error message. I can't seem to recreate this problem (I always hit the check). If you are able to provide any details about how it came to be that you got past that check, I would certainly appreciate it.

@kavmar
Copy link
Author

kavmar commented Jan 6, 2025

Happy to provide you more details.
I am working with monai-deploy-app-sdk to create DICOM SEGs, which uses highdicom internally. I found that that in their
DICOMSegmentationWriterOperator they create segment_descriptions with increasing numbers and difference 1, just as the standard requires:

https://github.com/Project-MONAI/monai-deploy-app-sdk/blob/9608cd77d00a71089526cc8f46f17826ef99463c/monai/deploy/operators/dicom_seg_writer_operator.py#L216

, but they do not look whether the respective numpy array with masks also fulfills the same requirement. So your code I assume should be on a safe side.

I suppose we can close this issue

@CPBridge
Copy link
Collaborator

CPBridge commented Jan 6, 2025

Ah ok. Well I contributed that operator :) so I suppose I should probably go an fix it there... Feel free to tag me in any issue if you open one there

@CPBridge CPBridge closed this as completed Jan 6, 2025
@kavmar
Copy link
Author

kavmar commented Jan 6, 2025

Thanks a lot for looking into this. I created a Discussion in monai-deploy-app-sdk and tagged you:
Project-MONAI/monai-deploy-app-sdk#512 (comment)

And similarly to this issue, which if, I am not mistaken, should need just two lines to fix :) :
Project-MONAI/monai-deploy-app-sdk#510 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

2 participants