Closed ccosmin97 closed 1 year ago
Thanks for the report. I think it's related this code: https://github.com/dcmjs-org/dcmjs/pull/220 but with your new data the error is larger than the (somewhat arbitrary) tolerance doesn't account for the differences due to numerical issues in your round trip.
You can see from the diff you provided that the orientation numbers are similar but not close enough to be considered matching by OHIF.
- (0020, 0037) Image Orientation (Patient) DS: [1.000000e+000, -1.990110e-010, 4.961899e-011, 2.051034e-010, 9.702957e-001, -2.419219e-001]
+ (0020, 0037) Image Orientation (Patient) DS: [1, 3.04622104e-12, 2.48094791e-11, 3.04622104e-12, 0.970295727, -0.241921902]
This is probably because nifti format uses 32-bit floats for orientation while DICOM uses a decimal string which is closer to a double (64 bit float), like the Number
type in JavaScript.
Since nifti is used a lot I guess we'll end up needing to "dumb down" ohif to consider anything that's within 32-bit float rounding error to be close enough.
Heres' some more background on issues related to DS
in dicom.
https://github.com/dcmjs-org/dcmjs/issues/175
After a bit more research @ccosmin97 and I determined that there is a nifti2 with a 64 bit float option, but apparently it's not widely used. We confirmed dcmqi is writing out a nift1 32-bit header and it's not clear that ITK has an option to write a nifti2 header or that the nnunet code would be able to read a nifti2 header. He will confirm if this really is a 32-bit vs 64-bit truncation issue or whether it is an issue with trying to overlay the SEG on a series that really does have different orientation. If it's a truncation issue we can look into making OHIF/dcmjs less strict about matching orientations.
The SEG and the original MRI data do have the same orientation(the SEG references to the T2 modality) :
The SEG has green value for foreground and yellow for background, it overlaps with the T2 data.
Besides, there seems to be no numerical rounding issues of the Image Orientation Patient for the .nrrd file conversion from dcm SEG using dmcqi v1.2.5 compared to the T2 data(DS: [1.000000e+000, -1.990110e-010, 4.961899e-011, 2.051034e-010, 9.702957e-001, -2.419219e-001]):
Finally, when converting back the nrrd segment files back to dcm using dmcqi v1.2.5, and then uploading to OHIF, it works fine, no errors are reported, as opposed to doing the same process with nifti segment files.
Since the converted nifti files are using a nifti1 header, it might explain the rounding issues that are not accepted by OHIF.
The same behavior regarding nifti files issue with OHIF upload can be observed with different ProstateX PatientIDs.
Please find attached this folder containing log files, PatientID == 0004 dcm data(T2,ADC,SEG_T2) and dcm metadata comparison.
This ipynb can be used to reproduce this issue.
We hit that rounding issue so many times now, and from what I recall last time, the tolerance was lowered to 0.01. When I looked at the data with Cosmin, I did not see the differences of significance.
See here: https://github.com/OHIF/Viewers/issues/2656#issuecomment-1014682438.
I can't believe we are exceeding 0.01 tolerance while writing those segmentations. Are we? I don't think I see that in the example above.
Fortunately, @Punzo is back!
Hi, the tolerance as described in https://github.com/OHIF/Viewers/issues/2656#issuecomment-1014682438 is indeed 0.01. I will double check asap if it's indeed a tolerance issue.
@ccosmin97 can you kindly provide also the MRI, please? the studyID will be enough, thanks!
Ah just seen that you already provided everything here https://www.dropbox.com/sh/vq3oe7f2fwl5p4r/AACNbGeKfkOQSEpzEXC3OlUua?dl=0
found the issue here: https://github.com/dcmjs-org/dcmjs/blob/master/src/utilities/orientation/nearlyEqual.js#L24 this condition was too strict (Number.EPSILON is around 10^-16). This means in the case of comparing two numbers close to zero (e.g., around 10^-[11:13]), these were not considered close enough to zero. Therefore we were using relative error comparison which does not work when both numbers are close to zero.
I fixed the issue changing the cost value Number.EPSILON (10^-16) to epsilon^2 (input parameter, default is epsilon = 0.001, for OHIF-IDC is 0.01). See https://github.com/dcmjs-org/dcmjs/pull/304
I will update dcmjs version in OHIF and IDC fork as soon as https://github.com/dcmjs-org/dcmjs/pull/304 will be merged. @pieper can you please review and merge thhe dcmjs PR, please?
@pieper we discussed this further, and @Punzo and I agreed that in order to address immediate needs in this issue, it makes sense to merge https://github.com/dcmjs-org/dcmjs/pull/304 first. Effectively, that PR makes the two branches of the condition statement consistent in using the same tolerance, which one could think of as a bugfix.
Going forward, we absolutely have to rethink how those tolerances should be defined in a more meaningful way. What I suggested after you left the meeting Steve, perhaps we could do the following:
1) in the user interface indicate what is the currently used tolerance and (more importantly) what are the implications of using this tolerance for the specific image (e.g., given the extent of the image, calculate the maximum shift in the location of the annotation that is affected by the tolerance);
2) allow user to adjust the tolerance to have more or less strict checks, perhaps accompanied by a warning message on the screen when the tolerance is increased beyond the default value.
I am all for a more meaningful handling of these scenarios, but I think we will need time and iterations to arrive at that ideal solution. In the meantime, based on my understanding and discussions with Davide, I believe the PR mentioned above is a valid bugfix and will allow addressing immediate need for IDC use case development.
I just wanted to confirm that the OHIF update with the condition relaxation fixed this issue with the same data mentioned above(idc dcm -> nifti -> dcm) for PatID004, ProstateX collection:
Thank you @Punzo @fedorov @pieper !!
@ccosmin97 yesterday you mentioned that you were still experiencing SEG loading issues while using SimpleITK for conversion of the input images. Can you please reproduce that issue and provide details/sample here?
In general, going forward - if you observe that an issue that we thought is fixed appears not to be fixed, it is very important for the purposes of improvement and refinement of open source tools to document this and include samples to reproduce the issue.
@ccosmin97 please note that if you are in OHIF version 4.12.41 (however this version did not hit yet the IDC production portal, you can check the version in the Debug Info window), you can use the run time tol value for segs. Please have a look at video: https://github.com/OHIF/Viewers/pull/2924#issue-1369720769
@ccosmin97 please note that if you are in OHIF version 4.12.41 (however this version did not hit yet the IDC production portal, you can check the version in the Debug Info window), you can use the run time tol value for segs. Please have a look at video: #2924 (comment)
On this note, after changing the runtime tolerance to 1e-1(instead of default 1e-2), the segmentations objects were correctly loaded, reverting back to 1e-2 or lower fails to load the segs on the right hand panel :
@ccosmin97 for the sake of completeness, can you please share the sample that allows reproducing this issue? Can you confirm this is not the issue if you use dcm2niix
for conversion of the input image?
@ccosmin97 for the sake of completeness, can you please share the sample that allows reproducing this issue? Can you confirm this is not the issue if you use
dcm2niix
for conversion of the input image?
I compared using SimpleITK vs dcm2niix for conversion of MR data which is the input for nnunet prediction segmentations. The problematic cases throwing an error in OHIF are(t = timestamp / 2 studies for each patient/ qin-prostate-repeatability) : PCAMPMRI-0015-T1, PCAMPMRI-0007-T1, PCAMPMRI-0013-T2, PCAMPMRI-0009-T1, PCAMPMRI-00007-T1, PCAMPMRI-00006-T2.
The notebooks are here : sitk.ipynb and dcm2niix.ipynb.
The only difference between these 2 ipynbs is the use of dcm2niix or sitk for dcm data to nii conversion for the input data to the nnunet segmentation framework.
After looking at OHIF, every patIDs(cf problematic cases) nnunet seg objects is correctly rendered except PCAMPMRI-0013-T2 if we use dcm2niix instead of sitk.
(left/right : dcm2niix conversion for patID15/sitk conversion for patID15)
dcm2niixversion used : Chris Rorden's dcm2niiX version v1.0.20220505 GCC7.5.0 x86-64 (64-bit Linux) v1.0.20220505
SimpleITK version used : 2.2.0
OHIF version : 4.12.41, with default thresholding value
After looking at OHIF, every patIDs(cf problematic cases) nnunet seg objects is correctly rendered except PCAMPMRI-0013-T2 if we use dcm2niix instead of sitk.
Can you parse this a bit for me? It sounds to me like you are saying dcm2niix is correct but sitk is incorrect? And you are saying that the issue that the PatientID field is incorrect in the sitk cases? Can you say exactly what is incorrect?
Cosmin, can you attach both output segmentations, the one from sitk and dcm2niix?
After looking at OHIF, every patIDs(cf problematic cases) nnunet seg objects is correctly rendered except PCAMPMRI-0013-T2 if we use dcm2niix instead of sitk.
Can you parse this a bit for me? It sounds to me like you are saying dcm2niix is correct but sitk is incorrect? And you are saying that the issue that the PatientID field is incorrect in the sitk cases? Can you say exactly what is incorrect?
Sorry @pieper for the confusion, to clarify : nnunet segmentations obtained from dcm2niix MR data conversion as an input(here T2+ADC dcm->nii) are correctly rendered in OHIF without the 'segmentations orthogonal to the acquisition place of the source data are not supported yet.'
However, converting the same MR data with SITK instead of dcm2niix produces the OHIF loading error mentioned right above.
The patientID field is correct everywhere, sorry again for the confusion, I only mentioned patID for specific cases where using sitk dcm to nii conversion of MR data input shows this OHIF error, which does not happen when using dcm2niix.
This MR data(T2+ADC) is the input of the nnunet segmentation pipeline.
I hope that helps!
Cosmin, can you attach both output segmentations, the one from sitk and dcm2niix?
Here are the output segmentations, in dcm format, for dcm2niix and sitk dcm to nii conversion of MR input.
Thank you @ccosmin97 that clarifies things 👍
Since we have full control over this use case, I would suggest copying the ImageOrientationPatient header values from the inputs to the output dicom files as an option to work around the known limitation of 32 orientation representation in nifti.
I would suggest copying the ImageOrientationPatient header values from the inputs to the output dicom files as an option to work around
I am not sure this is needed, since we already have segmentations that work in OHIF when using dcm2niix.
But what this means it that we/others might get segmentations that will not work with the default and will require adjusting tolerance threshold to (seemingly) unreasonable values.
Hello,
I am having issues rendering dcm segmentations in OHIF Viewer(version number : 4.12.30). The segmentations come from ProstateX IDC collection, converted to nii using dcmqi, then converted back to dcm using dcmqi(v 1.2.5) again.
The dcm file originating from IDC buckets is rendered properly into OHIF, however the dcm file from dcmqi conversion from nii format is not rendered properly.
Here is the error reported by the OHIF Viewer :![error_dcm_seg_ohif (1)](https://user-images.githubusercontent.com/72577931/184412717-a8b74022-3cd5-4ba8-bfec-5b2f88d6204e.jpg)
To reproduce this issue, please see this ipynb.
Additional info :
Thank you!
@fedorov