fix: compatibility issue for SEG sourcing MG images

kimtaejin922 commented 1 year ago

Issue overview

I am currently working on a project that automatically generates measurement report from mammography images. It uses a DICOM web viewer based on OHIF to view its generated results.

I noticed that the viewer throws an error when it loads the output of our application: mammography and segmentation images. The issue was caused by dcmjs module trying to parse some coordinate related tags from the source image of the SEG (i.e. MG image).

화면 캡처 2023-04-29 000332

error_123

The cause

According to the standard, the segmentation IOD have 2 ways to describe its spatial relationship with the original images: one is using Frame of Reference UID along with coordinate system, and the other is having the segmentation pixel size identical to the original image and use Derivation Image Functional Group Macro to describe how each frame and original images are matched. (Reference: see page 10 ~ 12 of suplement 111 from the DICOM standard for details)

I assume that the former use case is suited for 3D based images such as CT or MRI images, while latter use case is suited for 2D based images such as MG or X-ray images.

However, it seemed that the current version of dcmjs only covers the former use case (using coordinate system) and not covering the latter use case. To be specific, many of codes tries to extract tags from the Image Plane module and the Frame Of Reference module (i.e. 3D coordinate system) without condition. This would cause an error for MG images since modules mentioned above are not included in its IOD definition, while they are mendatory for CT and MRI IODs.

Solution

This PR tries to fix the issue by adding conditional statement before trying to extract the coordinate system from sourcing images. If the SEG has Frame of Reference UID, it would extract the Image Plane module from the sourcing images as it is doing in the current version. However, if the SEG does not have Frame of Reference UID, it would simply overlap the SEG frame on top of its sourcing image.

This is how the viewer looks after the fix.

화면 캡처 2023-04-29 000432

The former use case using coordinate system (CT, MRI) also works fine after the fix.

brain_seg

However, some modification are ad-hoc to bypass the problem. This is because many of the functions receives data derived from coordinate system as their parameters, and changing those interfaces would increase the risk of degradation.

Also, I only modified the Sementation_4X.js and not the Segmentation_3X.js because I thought this would be enough to cover future deployments and minimize future maintenance overhead.

Testing

Regarding the test, I think using jest is not suited for testing this modification:

The most important part is whether it looks okay when actually displayed.
The function requires cornerstone metadata provider injection on runtime.
The test would involve DOM manipulation.

So, I made a demo web page to verify the issue.

Added a demo web page for testing. See examples/displaySegmentationSourcingMG/index.html
Added a sample dataset (MG, SEG) which is loaded by the demo web page.

To test it on the browser:

Run npm run build and copy the files inside examples/js
Run npm install --global serve and serve examples to host examples directory on local server.
Type "http://localhost:3000/displaySegmentationSourcingMG" to open the demo web page. You should be able to verify that the SEG displays fine with MG images on button click.
Run git checkout 418cd357fbc5ff3c4d8836a48d102b739c25a26f if you want to run the unmodified (the error causing) version from the demo page.

The demo web page is for temporary verification purpose just for this pull request. Also, the MG test data is quite big to be stored inside git.

So, after when this PR is approved, I propose to delete both the data and the demo page, and squash commit before actually merging it.

Any feed backs are welcome.

unit test result: Test Suites: 13 passed, 13 total Tests: 67 passed, 67 total Snapshots: 0 total Time: 5.829 s

pieper commented 1 year ago

Hi - thanks for your work on this 👍 You are right, I don't think people had tried segmentations on non-volume data before, but it's great that this is working.

I went ahead and put the data from this PR as a release asset here. Can you update the test to use that and update the PR?

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

@fedorov do you have any comments on this SEG use case?

fedorov commented 1 year ago

Overall, I am not familiar with the MG modality, no direct experience with using SEG for MG. Reading the comment, I think it makes sense. Just make sure you use the text of the standard - not the supplement - for interpreting the standard. I think this is the relevant section: https://dicom.nema.org/medical/dicom/current/output/chtml/part03/sect_A.51.5.html#sect_A.51.5.1.

kimtaejin922 commented 1 year ago

@pieper @fedorov Thank you for your feedbacks. I will keep in mind to use the text and not the supplement when referring the standard.

By the way, I am not sure if I can make the demo web page to load DICOM data from the github.com origin because of the CORS policy: Access to XMLHttpRequest at 'https://github.com/dcmjs-org/data/releases/download/mg-seg/seg-test-MG.dcm' from origin 'http://localhost:3000' has been blocked by CORS policy: No 'Access-Control-Allow-Origin' header is present on the requested resource.

Since the actual demo pages are served from https://dcmjs.netlify.app/, I doubt it would run after the deployment either.

Unless there is any way to enable the CORS from the server side, I don't think the browser app can load the data from other origins.

Actually, if you would agree, I was thinking of deleting the test data and the demo page from the commit when review is done and before the merge. The test data is already included in the commit for easier testing and reviewing of the PR. However, the data is pretty big (18 MB) and probably should not be included in the actual project.

What do you think?

pieper commented 1 year ago

The release assets on the data repository are to support testing, so they don't work with the demos. If you want to test the demo locally you would need to download with a local tool or just use the same code that's in the test code of this repo outside the browser.

Ideally we should have examples of all data in a free-to-download place but we don't have that right now.

I suggest moving the demo page to a PR on the cornerstone repo and host data in the same way it is for other cornerstone demos. It was a mistake (mine) that these adaptor classes ended up in the dcmjs repo in the first place, and eventually they should be moved away.

kimtaejin922 commented 1 year ago

I went ahead and put the data from this PR as a release asset here. Can you update the test to use that and update the PR?

If you want to test the demo locally you would need to download with a local tool or just use the same code that's in the test code of this repo outside the browser.

Hello @fedorov @igoroctaviano

I added a unit test for this use case. The test code would internally download and use the test data uploaded at: https://github.com/dcmjs-org/data/releases/tag/mg-seg

Test results: Test Suites: 13 passed, 13 total Tests: 67 passed, 67 total Snapshots: 0 total Time: 10.505 s

Also, I removed the commit that included the test data from the history which was too big to be held inside git.

Let me know if you want me to make any other changes.