Repository navigation
Conversation
apply_voi() allocated the index array with the dtype taken from the LUT entry depth, so an 8-bit LUT with more than 256 entries wrapped every index above 255. apply_modality_lut() allocated it with the input dtype, so subtracting a negative first mapped value could overflow before the clip. In both cases np.clip() ran after the narrowing cast and so had nothing left to clamp. Size the index array from the number of LUT entries and clip before casting into it. Closes pydicom#2381
|
Hi @mrbean-bremen / @darcymason — checking in on this one. CI is green and it's a small, self-contained fix (two sibling functions sharing one index-clamping bug) with regression tests included. Happy to make any changes if you'd like something done differently. Thanks for taking a look! |
|
Hi @mrbean-bremen / @darcymason — following up again, it's been a few days with no activity. CI is still green and the fix/tests haven't needed any changes. Happy to address anything, just let me know. Thanks for your time! |
|
My time has been very limited, and will be for another week or so. I did have a very quick look at this and appreciate the general idea, but it will probably be several days before I could look at it in detail. |
|
Thanks for the update — no rush at all, I appreciate you taking a look given the limited time. Whenever you get a chance works for me. |
mrbean-bremen
left a comment
There was a problem hiding this comment.
Looks good to me, thanks!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2382 +/- ##
==========================================
+ Coverage 98.36% 98.41% +0.05%
==========================================
Files 77 77
Lines 14170 14168 -2
==========================================
+ Hits 13938 13944 +6
+ Misses 232 224 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Thanks for the review and approval, @mrbean-bremen! CI is green and codecov is clean — ready to merge whenever convenient. |
|
@darcymason - I think this can be merged without problems. |
Closes #2381
Description
Both
apply_voi()andapply_modality_lut()allocate the LUT index array with a dtype that cannot hold the full index range, then callnp.clip()after the values have already been cast into it. The cast wraps, so the clip has nothing left to clamp and the functions return wrong pixel values without warning.The two functions pick the wrong dtype in different ways:
apply_voi()uses the dtype derived from the LUT entry depth (LUT Descriptor value 3). The number of entries is independent of the depth of each entry, so an 8-bit LUT with 4096 entries - the ordinary way to display 12-bit data on an 8-bit display - wraps every index above 255.apply_modality_lut()usesarr.dtype, soarr - first_mapcan overflow when LUT Descriptor value 2 is negative, which is routine for CT. An overflowed value wraps negative and is then clamped to the first entry rather than the last.This sizes the index array from the number of LUT entries and clips before casting into it, so out-of-range input values map to the last entry as described in PS3.3 C.11.1.1.1 and C.11.2.1.1.
The existing
test_lut_sequence_zero_entriesbehaviour is unchanged: a LUT Descriptor value 1 of 0 still means 65536 entries, and indexing past the end of a shorterLUTDatastill raisesIndexError.Tests
Two regression tests added, both failing before the change and passing after:
TestApplyVOI::test_voi_8_bit_entries_more_than_256_entriesTestModalityLUT::test_lut_sequence_iv_exceeds_input_dtypepytest tests/pixelspasses (1452 passed, 309 skipped), andruff check/ruff formatreport no new findings.