Fix code and improve unit tests which prevent fail-over to the default decoders when nvimgcodec is tested #573
Conversation
Signed-off-by: M Q <mingmelvinq@nvidia.com>
There was a problem hiding this comment.
Pull request overview
This PR adds functionality to optionally save decoded DICOM image frames as PNG files for deeper visual inspection during performance testing, even when the nvimgcodec decoder produces numerically equivalent output to pydicom's built-in decoders.
Key Changes:
- Added three helper functions to iterate through frames, prepare pixel data for PNG format, and save frames as PNG files
- Extended
performance_test_nvimgcodec_decoder_against_defaultswith an optionalpng_output_dirparameter to enable PNG export - Modified variable names in the performance test to capture decoded pixel arrays for both baseline and nvimgcodec decoders
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| arr_min = float(arr_float.min()) | ||
| arr_max = float(arr_float.max()) |
There was a problem hiding this comment.
In the grayscale path, arr_min and arr_max are computed twice from different arrays (lines 77-78 from arr, then lines 87-88 from arr_float). The first computation (lines 77-78) is used only for the integer dtype checks (lines 81-84) but is then discarded. This is inefficient and potentially confusing. Consider removing the first computation or restructuring the logic to avoid redundant calculations.
| arr_min = float(arr_float.min()) | |
| arr_max = float(arr_float.max()) |
| PILImage = None # type: ignore[assignment] | ||
|
|
||
| if TYPE_CHECKING: | ||
| from PIL.Image import Image as PILImageType |
There was a problem hiding this comment.
Import of 'PILImageType' is not used.
| from PIL.Image import Image as PILImageType | |
| pass |
MMelQin
changed the title
MMelQin
changed the title
Signed-off-by: M Q <mingmelvinq@nvidia.com>
| else: | ||
| for index in range(arr.shape[0]): | ||
| frame = arr[index] | ||
| is_color = frame.ndim == 3 and frame.shape[-1] in (3, 4) |
There was a problem hiding this comment.
frame.ndim == 3 will always be false, right? Because arr.ndim is 3, you select a channel with arr[index], so you remove one dimension. So you can just set is_color to False.
One sanity check: can there be a multi channel image in pydicom with more that 4 channels? Because here you assumes that only if num channels is 3 or 4, then this is color image, and otherwise the first channel is the for frame indexing, and then there is width and height. But it could be as well width, height and many channels. Or such mutlichannel images are not allowed?
| if arr.ndim == 4: | ||
| for index in range(arr.shape[0]): | ||
| frame = arr[index] | ||
| is_color = frame.ndim == 3 and frame.shape[-1] in (3, 4) |
There was a problem hiding this comment.
Similar to my comment above, frame.ndim == 3 will always be true, right? Due to how you extract frame. So you can just leave part with frame.shape check
Signed-off-by: M Q <mingmelvinq@nvidia.com>
MMelQin
changed the title
|
When deeper inspection is needed to review the decoded pixels, even when the nvimgcodec produces the same output as pydicom built-in decoders, deeper inspection might be needed.
So adding a option to save each decoded image frame in PNG, in addition to already supported optional custom DICOM test dataset folder.