OHIF / Viewers

OHIF zero-footprint DICOM viewer and oncology specific Lesion Tracker, plus shared extension packages
https://docs.ohif.org/
MIT License
3.37k stars 3.39k forks source link

[Bug] No Normalizer Class error while Exporting DICOM Seg for OCT based DICOM images #4293

Open karanrane96 opened 4 months ago

karanrane96 commented 4 months ago

Describe the Bug

We are able to draw Segmentations on the OCT image, however when we try to click on "Export DICOM SEG", it gives an error in Console- "No Normalizer Class for "

Steps to Reproduce

  1. Open an OCT DICOM image in the OHIFv3 Viewer.
  2. Add a segmentation.
  3. Try to export the SEG dicom.

The current behavior

when we try to click on "Export DICOM SEG", it gives an error in Console- "No Normalizer Class for " image image

The expected behavior

The DICOM SEG should be exported successfully.

OS

Windows 10

Node version

18.16.0

Browser

Chrome 124.0.6367.61 (Official Build) (64-bit)

sedghi commented 3 months ago

Hmm, weird, do you have data?

Could you kindly provide the data if it has been anonymized and you can confirm that there is no patient health information present in any of the headers or embedded within the pixel data?

igorsimko commented 2 months ago

Hey guys,

I'm facing same issue with OCT (Media Storage SOP Class UID 1.2.840.10008.5.1.4.1.1.77.1.5.4). What I can see for now is that this UID is not supported by dcmjs since it's not part of UID map https://github.com/dcmjs-org/dcmjs/blob/faa0f86e674798fecfa6ef10ef42570adb5816aa/src/normalizers.js#L33 . Would that mean that OCT is not supported by OHIF? I can still use Basic Viewer but Segmentation doesn't really work.

@sedghi unfortunately I can't share dicom files on which is that happening. I found some at https://www.dicomlibrary.com/meddream/?study=1.3.6.1.4.1.44316.6.102.1.2023091383923657.429661656218407401559 but for these I get different error non-reconstructible displaysets so not sure how much that helps. As soon as I have some OCT I can share, I'll provide you file.

pieper commented 2 months ago

@igorsimko it would be great if you could propose appropriate normalizer code for OCT to the dcmjs project. I don't think many people have direct experience with that kind of data. If you can provide code, sample data, and tests it would be a nice contribution to the community.

fedorov commented 2 months ago

Adding Dennis @denbonte, who should at least be somewhat experienced, or maybe at least interested to monitor this!

denbonte commented 2 months ago

Thanks @fedorov for tagging me!

who should at least be somewhat experienced, or maybe at least interested to monitor this!

Unfortunately, I haven't started working with OCT images yet - but, since I have several colleagues who do, I will happily keep an eye on this 🙃

igorsimko commented 2 months ago

I made it work for now with following workaround:

I just used local version of dcmjs (replaced all dcmjs deps in all package.json to local file) and I did following changes to make segmentation work also for OPT modality:

  // dcmjs/src/DicomMetaDictionary.js
  DicomMetaDictionary.sopClassNamesByUID = {
    ...
    '1.2.840.10008.5.1.4.1.1.77.1.5.4': "OphthalmicTomographyImage",
    '1.2.840.10008.5.1.4.1.1.77.1.5.1': "OphthalmicPhotography8BitImage"
// dcmjs/src/normalizers.js

static normalizerForSOPClassUID(sopClassUID) {
  ...
  sopClassUIDMap[toUID.OphthalmicTomographyImage] = OCTImageNormalizer;
  sopClassUIDMap[toUID.OphthalmicPhotography8BitImage] = OPImageNormalizer;
  ...

static isMultiframeSOPClassUID(sopClassUID) {
  const toUID = DicomMetaDictionary.sopClassUIDsByName;
  const multiframeSOPClasses = [
      ...
      toUID.OphthalmicTomographyImage
  ];
  ...

convertToMultiframe() {
...
// Whole moving of pixel data is not really needed for my files and since files have only 8 BitsAllocated, 
whole pixel moving fails so I commented that out for now.

// ds.PixelData = new ArrayBuffer(ds.NumberOfFrames * frameSize);
// let frame = 0;
// distanceDatasetPairs.forEach(function (pair) {
//    ...
// });

class OCTImageNormalizer extends ImageNormalizer {
    normalize() {
        super.normalize();
    }
}

class OPImageNormalizer extends ImageNormalizer {
    normalize() {
        super.normalize();
    }
}
...
export { OCTImageNormalizer };
export { OPImageNormalizer };

I'd like to propose this to dcmjs as @pieper brought up but I really don't know whether that's correct approach and what consequences there can be.

However, right now that works for my case, but I can see some further limitation of OCT/OP such as reference lines being off on OP.

ahlaughland commented 2 months ago

igorsimko I added a normalizer for the modality Nuclear Medicine (NM) via PR with the team at dcmjs. Here is a link to the PR I submitted - https://github.com/dcmjs-org/dcmjs/pull/384/files Maybe you will find this helpful.

sedghi commented 2 months ago

@pieper @fedorov I'm reviewing this issue currently. I want to export a SEG on CR, but it's reporting that there's no normalizer for it in dcmjs.

Is there any documentation or guide on writing a normalizer? I've noticed many normalizers are quite simple and just call super.normalize.

What are the criteria for a normalizer to be accepted and merged?

pieper commented 2 months ago

Hi @sedghi - there's no design document for the normalizers, but maybe we should write something up to clarify the concept.

The idea is that code that consumes 'normalized' datasets of a particular class should be able to rely on certain properties always being true, like the CT class will always be an Enhanced Multiframe and the consuming code doesn't have to have special cases for whether the window/level is present since that would have already be fixed by the normalizer. The plan is/was to have specialized subclasses for any dicom variant seen in the wild (e.g. converting data from private tags into the standard form, like diffusion MR sequences).

The motivation is that code like GDCM is littered with special case code trying to handle non-standard dicom instances and I want that to be handled in a more organized way in dcmjs.

Where things stand now if there's not a clear thing to be done then yes, it should just call the superclass as a more-or-less passthrough.

sedghi commented 2 months ago

@pieper That is great explanation, thanks, I will probably add the US one

igorsimko commented 2 months ago

@ahlaughland thanks for your PR, I opened 2 PRs, one related to OCT and this issue: https://github.com/dcmjs-org/dcmjs/pull/402

and the second https://github.com/dcmjs-org/dcmjs/pull/403 for OP modality which is often referenced by OCT.

sedghi commented 2 months ago

@igorsimko Great work indeed, could you update the PRs with a sample data and generated SEG as well?

igorsimko commented 2 months ago

@sedghi Unfortunately I can't provide data I was using. I'll try to search for some open data but that might take a while.

pieper commented 2 months ago

@igorsimko perhaps you could make a dummy dataset following the structure of the data you can't share?

igorsimko commented 2 months ago

@pieper yes that would work but I have to change pixel data and compression as well, but I suppose that's okay for this case. Let me add it to PRs then.

pieper commented 2 months ago

Ideally you can make a very small file, but if it's larger than a few k we can add it here for use in testing:

https://github.com/dcmjs-org/data

igorsimko commented 1 month ago

@pieper I added both to related PRs. One (OP) has few KBs but other (OCT) has ~2MB, should I keep it in examples/data or put it to https://github.com/dcmjs-org/data?

pieper commented 1 month ago

I put the file in a data release:

https://github.com/dcmjs-org/data/releases/tag/oct

@igorsimko can you update to use this? Thanks.

igorsimko commented 1 month ago

@pieper I added small test for OCT normalizer where I download the file from dcmjs-org/data. Is that enough or you meant some other place?

pieper commented 1 month ago

Yes, https://github.com/dcmjs-org/dcmjs/pull/402/files is what I had in mind. It would be great if https://github.com/dcmjs-org/dcmjs/pull/403/files could also have a test.

Then could someone on this thread who uses or is familiar with these classes comment on whether the PRs are ready to merge?

igorsimko commented 1 month ago

@pieper I opened discussion regarding OP in https://github.com/dcmjs-org/dcmjs/pull/403#issuecomment-2387914880 to not spam this thread, would you (or anyone in this thread) mind sharing your thoughts there?