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

fix #154

Conversation

helghast79
Copy link
Contributor

Use unsorted dataset as source of ReferencedInstanceSequence in convertToMultiframe function to maintain ordering with other sources like segmentation labelMaps.

as refered in this discussion, when creating segmentation and using series with reversed order instances, the labelmaps will be out of sync with the instances. By using unsorted dataset as source of ReferencedInstanceSequence, ordering is maintained in all cases

…onvertToMultiframe function to maintain ordering with other sources like segmentation labelMaps
@pieper
Copy link
Collaborator

pieper commented Nov 25, 2020

Thanks for digging into this, but I don't think this is the best solution to the issue, and would probably cause side-effects.

In this code the PixelData of the multiframe is created using the sorted frames so we want the ImagePositionPatient to correspond. It's hard to know for sure, but I suspect the core issue in #144 that the segmentation positions are not matching the referenced image positions.

@pieper pieper closed this Nov 25, 2020
@helghast79
Copy link
Contributor Author

I understand @pieper but when we generate a segmentation we don't change the order or the source image right? It continues to be inverted and stored in the pacs (or wherever it is). So, does it make sense to reference a correctly ordered source image when it is not?

I believe we have 2 other options to correct the problem:

  • When creating the segmentation and upon saving it, force the source image to be resaved with proper order
  • Change image loader to reorder source images automatically

The first one completely solves the problem but can be difficult to force resaving. The second one hides the problem by correcting the source order before the user notices it.

@pieper
Copy link
Collaborator

pieper commented Dec 1, 2020

Hi @helghast79 -

Let's step back and make sure we are thinking about this the same way, and maybe you can give me an idea how you are using the data.

The dcmjs logic is to create a normalized multiframe derived from single frame instances with the frames sorted. Then the segmentation is a derivation from that. The segmentation's geometry and frame order match the multiframe, but that's should not be relied on. Instead consumers of the data should use the position and orientation information for the frames when using the segmentations with respect to referenced images. This is how we do it in Slicer so the segmentation can have completely different geometry but make sense together as long as they are in the same frame of reference. I take it this differs from your use case?

@helghast79
Copy link
Contributor Author

Hello @pieper! Sure, let's step back a little and i'll explain my use case and maybe we can figure this out.

So, I'm using Ohif viewer with modified segmentation plugin which can create and edit segmentations and save them in dicom-seg format using a modified dcmjs with the modifications made from this pull request. Everything worked fine but then, after this merged commit, @sinanmb reported that the segmentations generated were in reverse order from the source image. This, happens only for some images that for some vendor specific reason are in reverse order.

When dcmjs generateSegmentationFromDatasets function is called we pass an array of datasets composing our image source and inputLabelmaps3D with the segments, both are in sync with each other. Then the normalization function is called on the datasets and while converted to multiframe it gets sorted (no problem there) but then the ReferencedInstanceSequence is added in this loop

distanceDatasetPairs.forEach(function(pair) {
            const dataset = pair[1];

            ds.PerFrameFunctionalGroupsSequence.push({
                PlanePositionSequence: {
                    ImagePositionPatient: dataset.ImagePositionPatient
                },
                FrameVOILUTSequence: {
                    WindowCenter: dataset.WindowCenter,
                    WindowWidth: dataset.WindowWidth
                }
            });

            ds.ReferencedSeriesSequence.ReferencedInstanceSequence.push({
                ReferencedSOPClassUID: dataset.SOPClassUID,
                ReferencedSOPInstanceUID: dataset.SOPInstanceUID
            });
        });

ds refers to the dataset derived from the original dataset and the const dataset relates to the sorted dataset. Names are confusing, but you can see that the ReferencedSOPClassUID and ReferencedSOPInstanceUID are not being referenced to the source SOPClassUID and SOPInstanceUID but to the sorted SOPClassUID and SOPInstanceUID.

Given the fact that we cannot reorder the source images correctly (because they are stored in the pacs) and we shouldn't since that is out of scope of this library. In my opinion the reference should be kept to the original source, whatever it is.
if the PlanePositionSequence and FrameVOILUTSequence can break if not aligned with the sorted dataset then, is it possible to separate the loop like the following:

distanceDatasetPairs.forEach(function(pair) {
            const dataset = pair[1];

            ds.PerFrameFunctionalGroupsSequence.push({
                PlanePositionSequence: {
                    ImagePositionPatient: dataset.ImagePositionPatient
                },
                FrameVOILUTSequence: {
                    WindowCenter: dataset.WindowCenter,
                    WindowWidth: dataset.WindowWidth
                }
            });
 });

this.datasets.forEach(function(dataset) {
            ds.ReferencedSeriesSequence.ReferencedInstanceSequence.push({
                ReferencedSOPClassUID: dataset.SOPClassUID,
                ReferencedSOPInstanceUID: dataset.SOPInstanceUID
            });
 });

Would this work?

@pieper
Copy link
Collaborator

pieper commented Dec 3, 2020

Thanks for the context, that helps me see the situation.

Regarding your last suggestion of not sorting references I'm thinking that's not going to be robust because we really do want to keep the elements grouped together: the frame contents, the position, the window level, and the instance being referenced since they form a package that should not be split.

I'm curious though what you mean when you say "we cannot reorder the source images correctly (because they are stored in the pacs) "? I would think a common workflow would be that the user selects an image somehow and the program needs to find the frame(s) of the segmentation that reference that image's uid.

Although I should say that typically we have the case that all frames of the seg need to be considered for every image you render because the geometry and orientation may not match the referenced image. The referenced series sequence is more about recording the history of the segmentation and not managing display.

Does that make sense to you?

@helghast79
Copy link
Contributor Author

I'm curious though what you mean when you say "we cannot reorder the source images correctly (because they are stored in the pacs) "? I would think a common workflow would be that the user selects an image somehow and the program needs to find the frame(s) of the segmentation that reference that image's uid.

Exactly! That program is Ohif's dicom-segmentation extension and it finds the frame of reference! But it's in reverse order. The source frame to which the dcmjs finds to be the the nr. 1 after sorting is actually nr. 321 (in my case), that is, the frame of reference found to be the first (because of sorting) has a tag "IntanceNumber" with a value which corresponds to the last frame in the series. The source is saved this way in the pacs system and there is no way to change that.

Source Frame    Instance nr.   |   Segmentation Frame     referenced Source Frame (before/after sort)
     1              321        |           1                            321 / 1
     2              320        |           2                            320 / 2

Although I should say that typically we have the case that all frames of the seg need to be considered for every image you render because the geometry and orientation may not match the referenced image. The referenced series sequence is more about recording the history of the segmentation and not managing display.

makes sense not managing display but, may I ask, what is the need to sort the source dataset in the first place? And, would it be acceptable to include the possibility to not sort at all (via userOptions)?

@pieper
Copy link
Collaborator

pieper commented Dec 10, 2020

Hi Miguel - sorry for the slow response - some deadlines...

So now I had a chance to read through and I think I understand the issue. So you are saying that the OHIF code is assuming that FrameNumber of a SEG and InstanceNumber of the series it references should mean the same thing. I don't think that's a safe thing to assume so maybe we should address that one. As I described above, it's probably more robust for the viewer to connect segmentations to background images by image position and orientation which should be available in cornerstone.

I'm reluctant to change the normalizer because the idea it to put datasets into "normalized" form so that other code can rely on certain properties and not need to put checks and special cases all over the code. Hope that balance makes sense.

@pieper
Copy link
Collaborator

pieper commented Dec 11, 2020

Oh, and to answer your specific question about sorting, the reason is that datasets often come in shuffled orders - e.g. they could come in inode order when read from disk or alphabetical order that doesn't correspond to spatial order. By sorting the frames (and the corresponding functional properties) we can ensure that if the frame array forms a proper volume then it will be laid out that way in memory, which is a common assumption for many image processing operations and also good for transferring to GPU texture memory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants