fix(image-redactor): return rendered image when no text is detected in verify#2040
Open
AlexanderSanin wants to merge 3 commits into
Open
fix(image-redactor): return rendered image when no text is detected in verify#2040AlexanderSanin wants to merge 3 commits into
AlexanderSanin wants to merge 3 commits into
Conversation
…n verify When `add_custom_bboxes` was called with an empty bboxes list (i.e., no text detected in the DICOM image), it returned the raw PIL image directly, bypassing the matplotlib rendering pipeline. This caused two problems: 1. The `use_greyscale_cmap` parameter was ignored, so greyscale DICOM images were not rendered with `cmap="gray"`, resulting in a blank or colour-mapped image instead of the expected greyscale view. 2. The returned type was an unrendered PIL Image rather than the matplotlib-figure-derived PIL Image returned in every other code path, leading to inconsistent output and visually empty verification images. Fix: remove the early-return branch for `len(bboxes) == 0`. The `for` loop over bboxes is a no-op when the list is empty, so both paths now share the same matplotlib rendering code. Also add `plt.close(fig)` to release the figure after rendering to prevent a memory leak. Add two new parametrised test cases (empty bboxes, greyscale and RGB) that assert a valid, non-None PIL Image of the correct dimensions is returned. Closes microsoft#1034 Signed-off-by: Oleksandr Sanin <alexaaander.sanin@gmail.com>
Author
|
Hey @omri374 @HammadSiddiqui @max-tarlov-infinitusai. Could you, please, have a look at this? |
Author
@microsoft-github-policy-service agree |
Contributor
|
@AlexanderSanin can you take a look at the failing tests? |
…oxes found The previous attempt always routed through matplotlib for empty bboxes, which broke the integration test (test_given_image_without_text_and_pii_verify_then_image_does_not_change) because the RGBA matplotlib output is not pixel-identical to the original RGB input after resize. Targeted fix: only bypass the raw-image-return path when use_greyscale_cmap=True (i.e. greyscale DICOM images). For those, ax.imshow must receive cmap="gray" and go through the full matplotlib rendering pipeline so the image is displayed correctly instead of as a blank/wrong-colourmap image (the root cause of microsoft#1034). For regular RGB images with no bboxes the original PIL image is returned directly, preserving backward compatibility and keeping the integration test green. Also guard the pixel-colour loop in test_add_custom_bboxes_happy_path so it only runs when there are bboxes (the loop expects multi-channel pixels which a mode-L image does not provide). Signed-off-by: Oleksandr Sanin <alexaaander.sanin@gmail.com>
Author
|
@SharonHart Thanks for the heads-up! The integration test failure was caused by my first attempt always routing through matplotlib even for RGB images with no bboxes, which made the returned image pixel-incompatible with the original. I've pushed a corrected fix:
The Analyzer test failures are CI timeouts during dependency install — unrelated to this change. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
DicomImagePiiVerifyEngine.verify_dicom_instance()returned an empty/blank image when the DICOM file contained no burnt-in text PII (closes Returned verification image is empty if no text is detected #1034).ImageAnalyzerEngine.add_custom_bboxes()had an early-return branch forlen(bboxes) == 0that skipped the matplotlib rendering pipeline and returned the raw PIL image directly. This also ignored theuse_greyscale_cmapparameter, so greyscale DICOM images were never rendered withcmap="gray".for-loop overbboxesis a no-op, so both the zero-bbox and non-zero-bbox paths now share the same matplotlib rendering code path, producing a consistent return type.plt.close(fig)after rendering to release the matplotlib figure and prevent a memory leak that existed in both paths.Test plan
test_add_custom_bboxes_happy_pathintests/test_image_analyzer_engine.py:use_greyscale_cmap=Falseuse_greyscale_cmap=TrueNonePIL.Image.Imageof the correct dimensions is returned.test_image_analyzer_engine.pyandtest_dicom_image_pii_verify_engine.pycontinue to pass.