search

DIAGNijmegen / rse-panimg

Conversion of medical images to MHA and TIFF.
Apache License 2.0
13 stars 5 forks source link

Group DICOM files differently #77

Closed nlessmann closed 2 years ago

nlessmann commented 2 years ago

The DICOM image builder groups the individual files currently by study instance UID and in-plane dimensions of the image. It does that instead of just using the series instance UID because the individual 3D frames of a 4D volumes have different series instance UIDs. But this is clearly not ideal, there is no reason that there could not be two series with the same dimensions in the same study. This PR changes this to either use the StackID (mandatory DICOM tag in 4D images) or the series instance UID otherwise.

Additionally, I've come across a dataset where some additional reconstruction parameters were stored under the same series instance UID but with a different SOP class UID. This made the image builder crash. To fix this, this PR also adds the SOP class UID to the variables that identify a single image. The builder will then only crash for the non-image file and will work fine for the image itself.

chrisvanrun commented 2 years ago

FYI. CI is failing because something went awry with the SimpleITK in PyPi:

blowekamp commented 2 years ago

FYI. CI is failing because something went awry with the SimpleITK in PyPi:

The source distribution was remove, pip should download 2.1.1. Please try the CI again.

nlessmann commented 2 years ago

@blowekamp Thanks - it seems that it still fails in our case because we use poetry instead of pip, which seems to not recognize that no binaries are available for the latest version of SimpleITK and our OS and Python version combination.

blowekamp commented 2 years ago

OK, 2.1.1.1 has been yanked. Please try the CI again.

chrisvanrun commented 2 years ago

Thanks @blowekamp. TIL 'yanking' is a real technical term.

Alas, poetry (at this time) does not seem to support detection of a yanked package:

There is no big rush on this PR and no changes planned, so the most stable course of actions is to wait for PyPi admins to allocate more resources.

jmsmkn commented 2 years ago

@chrisvanrun I have excluded the yanked release in this branch. Could you review? Would be good to get this out as due to the combination of these two things it is not possible for users of panimg to lock their dependencies with poetry without them also excluding the yanked SimpleITK release.

Thank you for your help @blowekamp and great work on ITK, we're able to build so many things with it!

chrisvanrun commented 2 years ago

@chrisvanrun I have excluded the yanked release in this branch. Could you review? Would be good to get this out as due to the combination of these two things it is not possible for users of panimg to lock their dependencies with poetry without them also excluding the yanked SimpleITK release.

Thank you for your help @blowekamp and great work on ITK, we're able to build so many things with it!

First instinct was to do this as well. But then related things didn't wanted to get build locally. I see that the CI doesn't have that problem. Will dive into the details of the PR now.

jmsmkn commented 2 years ago

I guess that you still have a poetry.lock file locally, deleting it would solve that.