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

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

Closed
karanrane96 opened this issue Jul 17, 2024 · 22 comments
Labels
Awaiting Reproduction Can we reproduce the reported bug?

Comments

@karanrane96
Copy link

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)

@karanrane96 karanrane96 added the Awaiting Reproduction Can we reproduce the reported bug? label Jul 17, 2024
@sedghi
Copy link
Member

sedghi commented Jul 26, 2024

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
Copy link

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
Copy link
Member

pieper commented Aug 30, 2024

@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
Copy link
Member

fedorov commented Aug 30, 2024

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

@denbonte
Copy link

denbonte commented Sep 2, 2024

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
Copy link

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
Copy link

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
Copy link
Member

sedghi commented Sep 9, 2024

@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
Copy link
Member

pieper commented Sep 9, 2024

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
Copy link
Member

sedghi commented Sep 10, 2024

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

@igorsimko
Copy link

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

and the second dcmjs-org/dcmjs#403 for OP modality which is often referenced by OCT.

@sedghi
Copy link
Member

sedghi commented Sep 18, 2024

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

@igorsimko
Copy link

@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
Copy link
Member

pieper commented Sep 18, 2024

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

@igorsimko
Copy link

@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
Copy link
Member

pieper commented Sep 19, 2024

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
Copy link

igorsimko commented Sep 30, 2024

@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
Copy link
Member

pieper commented Sep 30, 2024

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
Copy link

@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
Copy link
Member

pieper commented Oct 1, 2024

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
Copy link

@pieper I opened discussion regarding OP in dcmjs-org/dcmjs#403 (comment) to not spam this thread, would you (or anyone in this thread) mind sharing your thoughts there?

@igorsimko
Copy link

This should be now resolved by upgrading dcmjs to 0.36.0. See dcmjs-org/dcmjs#402

@sedghi sedghi closed this as completed Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting Reproduction Can we reproduce the reported bug?
Projects
None yet
Development

No branches or pull requests

7 participants