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

Sorting of image planes for legacy converted objects #5

Open
fedorov opened this issue Feb 5, 2020 · 11 comments
Open

Sorting of image planes for legacy converted objects #5

fedorov opened this issue Feb 5, 2020 · 11 comments
Assignees
Labels
question Further information is requested

Comments

@fedorov
Copy link
Member

fedorov commented Feb 5, 2020

As implemented, it appears that legacy converter does not sort frames geometrically. Arguably, it should be the responsibility of the reader to sort them, but I wanted to confirm this was a deliberate decision not to sort them. @hackermd can you comment?

As aside, Slicer does not sort frames while reading resulting multiframe (tested with the 2020-01-12 nightly):

image

@hackermd
Copy link
Collaborator

hackermd commented Feb 6, 2020

As implemented, it appears that legacy converter does not sort frames geometrically. Arguably, it should be the responsibility of the reader to sort them, but I wanted to confirm this was a deliberate decision not to sort them. @hackermd can you comment?

Correct, this was a design decision. Frames are included in the DICOM data set in the order in which users provide them.

@hackermd hackermd self-assigned this Feb 6, 2020
@hackermd hackermd added the question Further information is requested label Feb 6, 2020
@hackermd hackermd closed this as completed Feb 6, 2020
@fedorov
Copy link
Member Author

fedorov commented Feb 21, 2020

I have to say though, that this decision strikes me as not quite consistent with the "high" part of "highdicom".

It will be inevitable that:

  • source frames will not be in the correct order
  • users may not appreciate the importance of ordering frames (e.g., I see it quite often that even users experienced with DICOM may use InstanceNumber or something like this to order individual instances)
  • commonly used viewers may not be robust to sort frames (as became evident from the early experience here ImageOrientationPatient is not read for multiframe input rordenlab/dcm2niix#372 (comment) - neither Slicer, nor Horos, nor (by default) OsiriX do this)
  • more advanced users will be re-implementing this sorting functionality

The more I think about it, the more I become convinced that I personally would vote for re-considering this design decision.

@hackermd
Copy link
Collaborator

commonly used viewers may not be robust to sort frames (as became evident from the early experience here rordenlab/dcm2niix#372 (comment) - neither Slicer, nor Horos, nor (by default) OsiriX do this)

Well, that's not really the fault of highdicom, is it? These applications are making assumptions that they shouldn't be making, since there is no guarantee that frames contained in a multi-frame image are in a specific order. The position of each frame is specified in the Per Frame Functional Groups Sequence and receiving applications should be capable of interpreting this information and sort frames accordingly.

The more I think about it, the more I become convinced that I personally would vote for re-considering this design decision.

I am open to having this discussion. Have a look at 14f42b9 whether this is what you were thinking.

@hackermd hackermd reopened this Feb 22, 2020
@fedorov
Copy link
Member Author

fedorov commented Feb 22, 2020

commonly used viewers may not be robust to sort frames
Well, that's not really the fault of highdicom, is it?

No, not at all. BUT highdicom would share the fault if/when after all the confusion and despair, the users will throw their hands up in the air and give up on the very idea of using those objects.

In defense of those applications, fixing those issues may involve dependencies in external libraries (e.g., ITK for Slicer), which may be non-trivial. Considering how uncommon those legacy enhanced, or even non-legacy enhanced, objects are, it can be hard to justify the effort (or to convince someone that the limited examples that can be provided are correct, or representative). Inevitably, we won't have control over many popular applications, but we would want to increase the chances of interoperability with those "as is" without sacrificing validity of the objects.

Yes, the sorting features you mentioned look like what I had in mind! cc: @afshinmessiah, who just implemented something similar in his highdicom-based conversion code - should he test your branch?

@hackermd
Copy link
Collaborator

In defense of those applications, fixing those issues may involve dependencies in external libraries (e.g., ITK for Slicer), which may be non-trivial.

Fair enough. As long as the objects highdicom creates are compliant with the standard, we can accommodate to the requirements of other applications. However, I think we need to be careful that we don't overfit to a particular application.

Yes, the sorting features you mentioned look like what I had in mind! cc: @afshinmessiah, who just implemented something similar in his highdicom-based conversion code - should he test your branch?

Would be great if you could test.

@pieper
Copy link
Member

pieper commented Feb 22, 2020

My $0.02 - highdicom should provide a set of SOPClass-aware utilities that allow sorting of frames in ways that may be useful to the consumer of multiframe instances.

I'd want to be sure we don't hard-code a single way of sorting. For example, I would envision using highdicom in an application that consumes MF instances (e.g. Slicer) and would want to have appropriate sorting utilities for things like dual-echo, diffusion, fMRI, etc that did not rely on the producing application to put them in the order the user needs for further visualization or analysis, such as creating well-formed vtk or itk images.

We have a lot of this sorting code in python already in Slicer's DICOMPlugins and could migrate it to highdicom. Maybe highdicom can have a mechanism like Slicer's to deal with user intent when there are multiple ways of interpreting the data.

@hackermd
Copy link
Collaborator

My $0.02 - highdicom should provide a set of SOPClass-aware utilities that allow sorting of frames in ways that may be useful to the consumer of multiframe instances. I'd want to be sure we don't hard-code a single way of sorting.

That was the motivation for not taking on sorting upon conversion and letting users sort the data sets. I like the idea of providing utility functions and providing good examples on how to use them.

However, this approach would not solve the interoperability issues with existing applications. But ultimately, they should be fixed anyways if they claim compliance..

@pieper
Copy link
Member

pieper commented Feb 22, 2020

However, this approach would not solve the interoperability issues with existing applications. But ultimately, they should be fixed anyways if they claim compliance..

Very true - my thought would be that when converting to multiframe you could optionally apply a sorting method that might be needed to work around limitations of consuming apps. This would be analogous to uncompressing jpeg or storing little endian.

In a similar vein, we also want to have other compatibility routines, e.g. to make single frame instances (and even nrrd and nifti) from MF so we can use the data with legacy code.

@hackermd
Copy link
Collaborator

Very true - my thought would be that when converting to multiframe you could optionally apply a sorting method that might be needed to work around limitations of consuming apps.

I like the idea of providing a Callable as an optional constructor argument (e2f952d). @pieper @fedorov @afshinmessiah what do you think?

@pieper
Copy link
Member

pieper commented Feb 24, 2020

It can be helpful for the user to provide a sorter, but it's also likely that the pattern can be deduced from the content/context of the multiple frames of the acquisition. For example see the discussion here at this dcmjs issue where there are multiple contrast phases within a single series (similar issues happen in DWI).

When the software might be able to deduce the correct dimension sequence then I think it should do so by default if possible. Of course it's tricky to deal with ambiguity; in an interactive application like Slicer you can ask the user to pick which interpretation is intended, but for a batch job you may need to express the desired interpretation in advance.

From a software engineering point of view, I think custom subclasses of the SOPClass class that deal with specific contexts makes the most sense for highdicom.

@fedorov
Copy link
Member Author

fedorov commented Feb 24, 2020

I do like the Callable optional argument. I also agree that if possible at all, it will be helpful when this order is deduced automatically.

I think there should be some component of highdicom that analyzes repeated patters across individual frames to sort things into shared/per-frame buckets, and then try to deduce from the per-frame buckets how to group things. I think SOPClass-specific hints could help, but I don't know if they justify separate classes.

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

No branches or pull requests

3 participants