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.
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.
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.
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.
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?
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.
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.
Okay - I’ll add that.
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.
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.
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.
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).
Looks to me like it’s calling logging.warning - why is it coming up as critical?
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).
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.