didymo / OnkoDICOM

OnkoDICOM was created with Radiation Oncologists to allow Radiation Oncologists to do research on DICOM standard image sets (DICOM-RT, CT, MRI, PET) using open source technologies, such as pydicom, dicompyler-core, PySide6, PIL, and matplotlib. OnkoDICOM is cross platform, open source software, and welcomes contributions. OnkoDICOM was inspired by dicompyler.
https://onkodicom.com.au
GNU Lesser General Public License v2.1
62 stars 42 forks source link

Study ID #261

Closed didymo closed 1 year ago

didymo commented 1 year ago

Does OnkoDICOM generally handle dicoms with no Study ID well or is it an unusual use case scenario to have a dicom with no Study ID?

didymo commented 1 year ago

As far as I am aware, the DICOM standard requires a study id hence passing this to Dr. Miller and Dr. Swerdloff.

sjswerdloff commented 1 year ago

Study ID (0020,0010) is type 2 https://dicom.innolitics.com/ciods/ct-image/general-study/00200010

So it is not mandatory that it have a value, but it is required that the attribute be encoded. If the attribute is not encoded at all, that is a DICOM conformance failure, and it is not reasonable to expect an application to operate on that data.

sjswerdloff commented 1 year ago

$ grep -r StudyID Model/ROI.py: Name, and Description, which are set to "OnkoDICOM" plus the StudyID Model/ROI.py: Tag("StudyID"), Model/ROI.py: rt_ss.StructureSetDescription = "OnkoDICOM rtss of " + rt_ss.StudyID Model/DICOM/Structure/DICOMPatient.py: :param study_uid: StudyID to check. Model/DICOM/DICOMStructuredReport.py: Tag("StudyID"), Model/ImageFusion.py: Tag("StudyID"),

So it's possible that construction of the StructureSetDescription string will fail if rt_ss.StudyID raises an error/exception due to lack of a value, or it might just leave that empty. The file DICOMPatient.py contains a typographical error... the parameter is study_uid, the docstring for it should say "StudyUID to check" or more formally "Study Instance UID to check".

The lines containing Tag("StudyID") are for construction of lists of elements that are going to be deep copied, and that should be safe with a type 2 element (the element will get copied, with no value).

While StudyID is typically populated with a value, I have seen data sets without a value.

oismaster commented 1 year ago

Hello Peter

Firstly thank you for working on the code. This last round has left things on a bit of disarray with python installs, and with the EXE.

At a high level, as far as I know every clinical case will have a study I'd set by the generating machines and software. Stuart can give more detail on this.

I don't believe that the study I'd identifies anything except if you have the original identified files.

As such, and unless there is a use case that I am unaware of, I would have thought that the study I'd is inevitable and therefore necessary in clinical files. So maybe OnkoDICOM has no business working with unidentified files.

What do you think Stuart?

A

On Thu, 2 Feb 2023, 18:26 Peter Qian, @.***> wrote:

If a non-conformant DICOM were to be passed, what would the expected behaviour of the application be? In my experience, the application just froze instead of throwing an error.

— Reply to this email directly, view it on GitHub https://github.com/didymo/OnkoDICOM/issues/261#issuecomment-1413266368, or unsubscribe https://github.com/notifications/unsubscribe-auth/AL5KJHJYF3APH3BPISWGOZDWVNOQXANCNFSM6AAAAAAUNATHTA . You are receiving this because you were assigned.Message ID: @.***>

sjswerdloff commented 1 year ago

Study ID is mandatory for the IHE Scheduled Workflow profile, which is what is used to drive the acquisition of a large fraction of CT (more specifically, support for it in terms of Query Keys and responses is mandatory, but I don't see how one could fulfil that if the CT itself doesn't have the Study ID, and the CT modality is required to gather the information from the DICOM modality work list provider, which is required to provide that information).

IHE-RO requires that the Study ID be re-used from the input objects in the generation of output objects created as part of numerous profiles. Having OnkoDICOM support CT without Study ID should only be a requirement if the product owner insists on it, and would probably need to provide a highly motivating use case for it.

In short, it's not worth the bother to try to support CT without the Study ID (0020,0010). It would likely lead to further difficulty (any output not to mention the re-use of the input CT is unlikely to be accepted in consuming systems).

If a non-conformant DICOM were to be passed, what would the expected behaviour of the application be? In my experience, the application just froze instead of throwing an error.

If at some point we want to add strong input validation, this would be one of the things to check and reject (with clear messaging as to why). In the meantime, errors should be getting logged, so if the application freezes, the first thing to do is to go look at the logs to see why.

oismaster commented 1 year ago

So that settles it, no Study ID, no open!

A

On Fri, 3 Feb 2023 at 18:12, sjswerdloff @.***> wrote:

Study ID is mandatory for the IHE Scheduled Workflow profile, which is what is used to drive the acquisition of a large fraction of CT (more specifically, support for it in terms of Query Keys and responses is mandatory, but I don't see how one could fulfil that if the CT itself doesn't have the Study ID, and the CT modality is required to gather the information from the DICOM modality work list provider, which is required to provide that information).

IHE-RO requires that the Study ID be re-used from the input objects in the generation of output objects created as part of numerous profiles. Having OnkoDICOM support CT without Study ID should only be a requirement if the product owner insists on it, and would probably need to provide a highly motivating use case for it.

In short, it's not worth the bother to try to support CT without the Study ID (0020,0010). It would likely lead to further difficulty (any output not to mention the re-use of the input CT is unlikely to be accepted in consuming systems).

If at some point we want to add strong input validation, this would be one of the things to check and reject (with clear messaging as to why).

— Reply to this email directly, view it on GitHub https://github.com/didymo/OnkoDICOM/issues/261#issuecomment-1415193058, or unsubscribe https://github.com/notifications/unsubscribe-auth/AL5KJHOPOHTDW64YTCCFTKTWVSVXPANCNFSM6AAAAAAUNATHTA . You are receiving this because you were assigned.Message ID: @.***>

-- Prof. Andrew Miller B.Med, B.Sc, Grad.Dip.Ed, M.Inf.Comm.Tech(Res), FRANZCR, FAIDH, PHF


“Men occasionally stumble over the truth, but most of them pick themselves up and hurry off as if nothing ever happened.” ― Winston S. Churchill

A committee is a cul-de-sac down which ideas are lured and then quietly strangled.

sjswerdloff commented 1 year ago

So that settles it, no Study ID, no open! A

Should this be interpreted to mean a) We don't take responsibility for handling data without Study ID or b) We are to check if Study ID is not populated and reject the data explicitly.

oismaster commented 1 year ago

B

On Sun, 5 Feb 2023, 09:31 sjswerdloff, @.***> wrote:

So that settles it, no Study ID, no open! A … <#m7746078385905387989>

Should this be interpreted to mean a) We don't take responsibility for handling data without Study ID or b) We are to check if Study ID is not populated and reject the data explicitly.

— Reply to this email directly, view it on GitHub https://github.com/didymo/OnkoDICOM/issues/261#issuecomment-1416864247, or unsubscribe https://github.com/notifications/unsubscribe-auth/AL5KJHKPV5FJIRT5SLH5H2DWV3KC7ANCNFSM6AAAAAAUNATHTA . You are receiving this because you were assigned.Message ID: @.***>

sjswerdloff commented 1 year ago

See #265 ...