Fix: Support 'jpg' format in keras.utils.save_img() #21683
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello @utsab345, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a compatibility issue within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses an issue with saving images in JPG format by mapping 'jpg' to 'jpeg', which is the format name Pillow expects. The change is a good step, but the logic for handling RGBA to RGB conversion for JPEGs is incomplete as it doesn't account for cases where the file format is inferred from the path. I've suggested a more robust implementation for this. Additionally, the new tests could be improved by using parametrization to reduce duplication and by adding more comprehensive assertions to verify the saved image content. My feedback includes suggestions to address these points.
keras/src/utils/image_utils.py
Outdated
| if file_format is not None and file_format.lower() == 'jpg': | ||
| file_format = 'jpeg' | ||
| img = array_to_img(x, data_format=data_format, scale=scale) | ||
| if img.mode == "RGBA" and (file_format == "jpg" or file_format == "jpeg"): | ||
| if img.mode == "RGBA" and file_format in ["jpeg", "jpg"]: |
There was a problem hiding this comment.
This change correctly handles file_format='jpg', but it misses the case where file_format is None and the path ends with .jpg. This will cause a crash when saving an RGBA image. The logic can be made more robust by also inspecting the path extension to determine if the output is a JPEG. Also, the condition on line 181 becomes partially redundant after converting jpg to jpeg.
| if file_format is not None and file_format.lower() == 'jpg': | |
| file_format = 'jpeg' | |
| img = array_to_img(x, data_format=data_format, scale=scale) | |
| if img.mode == "RGBA" and (file_format == "jpg" or file_format == "jpeg"): | |
| if img.mode == "RGBA" and file_format in ["jpeg", "jpg"]: | |
| _format = file_format | |
| if _format is None and isinstance(path, (str, pathlib.Path)): | |
| _format = pathlib.Path(path).suffix[1:].lower() | |
| if file_format is not None and file_format.lower() == 'jpg': | |
| file_format = 'jpeg' | |
| img = array_to_img(x, data_format=data_format, scale=scale) | |
| if img.mode == "RGBA" and _format in ("jpeg", "jpg"): |
integration_tests/test_save_img.py
Outdated
| import numpy as np | ||
| import os | ||
| from keras.utils import save_img | ||
|
|
||
| def test_save_jpg_rgb(tmp_path): | ||
| img = np.random.randint(0, 256, size=(50, 50, 3), dtype=np.uint8) | ||
| path = tmp_path / "rgb.jpg" | ||
| save_img(path, img, file_format="jpg") | ||
| assert os.path.exists(path) | ||
|
|
||
| def test_save_jpg_rgba(tmp_path): | ||
| img = np.random.randint(0, 256, size=(50, 50, 4), dtype=np.uint8) | ||
| path = tmp_path / "rgba.jpg" | ||
| save_img(path, img, file_format="jpg") | ||
| assert os.path.exists(path) |
There was a problem hiding this comment.
These tests are a good start, but they can be improved. The two test functions are very similar and can be combined into a single parameterized test using pytest.mark.parametrize for better maintainability. Also, the assertions only check for file existence. It would be more robust to load the saved image and verify its content, especially to confirm that RGBA images are correctly converted to RGB.
import os
import numpy as np
import pytest
from keras.utils import img_to_array, load_img, save_img
@pytest.mark.parametrize(
"shape, name",
[
((50, 50, 3), "rgb.jpg"),
((50, 50, 4), "rgba.jpg"),
],
)
def test_save_jpg(tmp_path, shape, name):
img = np.random.randint(0, 256, size=shape, dtype=np.uint8)
path = tmp_path / name
save_img(path, img, file_format="jpg")
assert os.path.exists(path)
# Check that the image was saved correctly and converted to RGB if needed.
loaded_img = load_img(path)
loaded_array = img_to_array(loaded_img)
assert loaded_array.shape == (50, 50, 3)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21683 +/- ##
========================================
Coverage 82.57% 82.58%
========================================
Files 572 572
Lines 58037 58325 +288
Branches 9077 9134 +57
========================================
+ Hits 47923 48166 +243
- Misses 7799 7828 +29
- Partials 2315 2331 +16
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks for the PR! Please fix the code format and address the relevant comments from Gemini. |
|
You can fix the code format via |
Summary
This PR fixes issue #21678.
The
save_imgfunction previously failed with.jpgextension because Pillow only supports 'JPEG'.Now,
.jpgis automatically mapped to 'jpeg'.Changes
test_save_img_with_jpgin integration_tests/test_save_img.py.Notes