Conversation
|
Thank you for the pull request. I was originally assuming this to be a bug, but with a deeper inspection, I understand that this makes an adjustment to the VOI LUT function from LINEAR to LINEAR_EXACT when the width is incompatible with the default LINEAR VOI LUT function, is this the case? At this point we may be dealing with a compatibility concern rather than a bug, since we're dealing with non-compliant inputs. Is there a precedent for this behavior in other tools? |
|
The standard says:
so practically, this file is not compliant... The two other viewers I know of are:
|
|
Thank you for the clarification. This is not at all a blocker, though I find it important to recognize when a non-conformity with DICOM exists and from which side it occurs. |
|
Yup fully agree! We recently had to ingest a large amount of older files, and bumped into various of those "interesting" cases... |
| VoiLutFunction::Linear if window_level.width < 1. => { | ||
| (VoiLutFunction::LinearExact, window_level.width.max(0.)) | ||
| } | ||
| VoiLutFunction::Linear => (voi_lut_function, window_level.width), |
There was a problem hiding this comment.
@abustany, I think this arm introduces a regression: a NaN window width used to be harmless and now breaks rendering.
On master every width went through .max(1.), and f64::max returns the non-NaN operand, so NaN silently became 1.0. With this change NaN < 1. is false, the NaN falls through to this catch-all arm, and it's stored as-is.
From there:
- debug builds panic on
debug_assert!(ww >= 1.)inwindow_level_linear - release builds fail LUT construction with
CreateLutError
So a file that renders today would stop rendering.
VoiLutFunction::Linear => (voi_lut_function, window_level.width.max(1.)),There was a problem hiding this comment.
hmm that's a good catch, thanks for the review. I must admit though that I don't know how we'd end up with a NaN value in there?
If I'm not mistaken, restoring .max(1.) at line 109 in the hunk above would preserve the old behavior?
There was a problem hiding this comment.
NaNs could occur from the ingestion of invalid input, either programmatically through the library core+object API or by loading a file with illegal values for Window Width (the VR DS requires numbers to be finite, but they are not inspected during DICOM parsing). On the latter case I have plans to reduce the risk of introducing NaNs at this level by adding methods to retrieve finite floating point values, so we get a recoverable error instead of a NaN.
On a file with the following tags:
Before:

After:
