DICOM scalar volume load: irregular geometry warning overly stringent?

dicom

(Andrey Fedorov) #1

I noticed that geometry mismatch reporting is overly stringent in reporting errors:

Irregular volume geometry detected, but maximum error is within 
   tolerance (maximum error of 5.17578e-08 mm, tolerance 
   threshold is 0.001 mm).

I suggest that if the error is less than the limit, there should be no notification at all.


(Steve Pieper) #2

I think it’s helpful to know if there’s a nonzero geometry issue, even when it is below the threshold. It could make sense to define an epsilon to account for numerical issues, but it can be hard to pick a valid value for that so letting the user look at the value and, as in this case, ignore it because it’s small.

To put it another way, the Examine step should probably generate some kind of “report” or “assessment” data structure that would describe how well the data matches the requirements of the reader. The user could choose to look at this or not.

Currently it’s true that the 0.001 mm threshold is very arbitrary and not exposed to the user.


(Andrey Fedorov) #3

There will always be a nonzero (ie, machine precision) geometry issue, which means there will always be that warning/error message. If we report warning/error every time, chances are it will be ignored, and important issues may be un-noticed. I am not sure it is helpful to know that there is an error of 5e-08. That is not an error.


(Steve Pieper) #4

What I’m saying is that there are really two thresholds of interest - one is the tolerance for “effectively zero” and once is the tolerance for non-zero but not significant. Would you want to ignore all values less than 1e-6? Perhaps we should add preference settings for both values.


(Andrey Fedorov) #5

Yes, I agree, it makes sense to have 2 thresholds. I guess the difference between the 2 would be warning in the console, or warning in the loadables list?


(Steve Pieper) #6

Yes, that’s what I would think. I guess the only question is the best default values for these, since most people wouldn’t be expected to change them in normal practice.


(Andrey Fedorov) #7

I think adding the second threshold and setting it to 1e-6 should be harmless, even if it is not exposed in the preferences. All it will do is reduce the amount of red in the python console.


(Steve Pieper) #8

Okay - I’ll add that.


(Steve Pieper) #9

Committed:

http://viewvc.slicer.org/viewvc.cgi/Slicer4?view=revision&revision=27350


(Andras Lasso) #10

Are these tolerances exposed in the application settings GUI? Tolerances may be different for a microCT or pre-clinical data sets, so it would be better if the user had control over it. We already expose some DICOM settings in the application settings, it should not be difficult to add some more parameters.


(Steve Pieper) #11

It could make sense to add them to the settings under the choice of applying the regularization or not. In reality the setting is more dataset-specific than global. We might think about expanding the concept of the DICOMPlugins to include a small user interface to select interpretation options as an alternative to generating a long list of loadables to select among.


(Steve Pieper) #12

For now the tolerances are optional at the python class level, so maybe it’s enough to provide an example of how to use it from the scripting level.


(Andrey Fedorov) #13

Not a huge deal, but I just noticed that somehow the tolerance warning is also reported as CRITICAL in the log:

[WARNING][Python] 01.11.2018 10:19:02 [Python] (/Applications/Slicer.app/Contents/lib/Slicer-4.9/qt-scripted-modules/DICOMScalarVolumePlugin.py:681) - Irregular volume geometry detected, but maximum error non-zero but is within tolerance (maximum error of 0.000165356 mm, tolerance threshold is 0.001 mm).
[CRITICAL][Stream] 01.11.2018 10:19:02 [] (unknown:0) - Irregular volume geometry detected, but maximum error non-zero but is within tolerance (maximum error of 0.000165356 mm, tolerance threshold is 0.001 mm).

(Steve Pieper) #14

Looks to me like it’s calling logging.warning - why is it coming up as critical?


(Andras Lasso) #15

logging.warning messages are stored in the application log correctly as a warning.

However, the warning message is also printed on the console as standard error. Anything that is printed as standard error is stored in the application log as critical error (we have no way of knowing what kind of error was that or it was just a warning).


(Steve Pieper) #16

Okay - makes sense - maybe the dicom loadables should have the option to return both warnings and errors, and then they would be printed to stdout and stderr respectively.