fix: compatibility issue for SEG sourcing MG images #351
Conversation
|
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? |
|
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. |
|
@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: Since the actual demo pages are served from 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? |
|
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. |
| imagePlaneModule.columnCosines.z | ||
| ]; | ||
| const hasCoordinateSystem = "FrameOfReferenceUID" in multiframe; | ||
| if (hasCoordinateSystem) { |
There was a problem hiding this comment.
You need a more robust check than just frame of reference uid otherwise rowCosines or other values dependent on orientation might fail.
Maybe checks for PixelMeasuresSequence, PlanePositionSequence, and PlaneOrientationSequence which is used inside the coordinate check in this line 813
There was a problem hiding this comment.
Hello @igoroctaviano
If you are referring to the conditional statements such as in line 301 and 302,
I think simply checking the presence of FrameOfReferenceUID should be enough.
-
This conditional statement is a direct interpretation from the DICOM standard:
When a Frame of Reference UID is present the segment shall be specified within that coordinate system, ...
If the Frame of Reference UID is not present, each pixel of the segmentation shall correspond to a pixel in a referenced image, using the Derivation Image Functional Group
(reference: https://dicom.nema.org/medical/dicom/current/output/chtml/part03/sect_A.51.5.html#sect_A.51.5.1) -
The Frame of Reference UID check is to determine whether it should use the coordinate system approach or the Derivation Functional Group approach, not whether the data is valid for each approach. Hence, even in the case we would want to validate the data more thoroughly, such check should be done after first deciding which approach to apply because the validation logics would differ depending on each approaches.
-
I am using highdicom package from Python for generating the SEG data from MG images. The highdicom project had similar issue as we are discussing now. Before, it could only generate SEG from 3D based images like CT and MRI, and not from 2D based images like MG. However, highdicom project now support latter use case as well and I believe its logic is fairly same with the rules mentioned above: first check the presence of Frame of Reference UID, and then apply corresponding validation and processing logics for each approach.
(reference: Allow segmentations of images without a frame of reference UID ImagingDataCommons/highdicom#183)
(reference: https://github.com/ImagingDataCommons/highdicom/blob/v0.21.1/src/highdicom/seg/sop.py#L1138)
OR, If you meant to point out that we should enhance validation logic prior to the actual processing (such as checking presence of patient orientation for coordinate system approach), I agree with you. By doing so, we can let users know whether the error is caused by the data not following the standard or by some bug from the app.
However, since the goal of this PR is to fix the app from throwing an error when loading SEG sourcing MG images, I propose to keep the processing/validation logic as-is, and only modify where it's causing the error.
So, if it is okay, I wish to separate the validation enhancement task for different PR as a future TODO. It would be much more complicated to extra implement/test such features.
kimtaejin922
force-pushed
the
fix/addMGCompatibility
branch
from
418cd35
to
97bda34
Compare
Hello @fedorov @igoroctaviano I added a unit test for this use case. Test results: 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. |
|
@pieper LGTM, any objection to merging it as is? |
|
No objection from me @igoroctaviano . Thanks for reviewing this. |
|
Publishing failed with this - can someone check why? https://github.com/dcmjs-org/dcmjs/actions/runs/7644858615/job/20830112948 |
|
🎉 This PR is included in version 0.30.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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).
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.
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.
The former use case using coordinate system (CT, MRI) also works fine after the fix.
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.
Regarding the test, I think using jest is not suited for testing this modification:
So, I made a demo web page to verify the issue.
examples/displaySegmentationSourcingMG/index.htmlTo test it on the browser:
npm run buildand copy the files insideexamples/jsnpm install --global serveandserve examplesto host examples directory on local server.git checkout 418cd357fbc5ff3c4d8836a48d102b739c25a26fif 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