-
Notifications
You must be signed in to change notification settings - Fork 1.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
TST: Uncomment an assert #3053
base: main
Are you sure you want to change the base?
TST: Uncomment an assert #3053
Conversation
Also remove an underscore from a test name.
Also remove an underscore from a test name, and modify some comments.
Also remove an underscore from a test name, and modify some comments.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3053 +/- ##
=======================================
Coverage 96.36% 96.36%
=======================================
Files 52 52
Lines 8770 8770
Branches 1596 1596
=======================================
Hits 8451 8451
Misses 191 191
Partials 128 128 ☔ View full report in Codecov by Sentry. |
Also remove an underscore from a test name, define bytes directly, and modify some comments.
Also remove an underscore from a test name, define bytes directly, and modify some comments.
Also remove an underscore from a test name, define bytes directly, and modify some comments.
@@ -475,7 +473,7 @@ def test_index_lookup(): | |||
assert data.image.mode == "RGB" | |||
assert image_similarity(data.image, refimg) > 0.999 | |||
# indexed CMYK images | |||
# currently with a TODO as we convert to RBG the palette | |||
# currently with a TODO as we convert to RGB the palette |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment still sounds a bit odd ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what the comment means. Do you think it's to do with _handle_flate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There apparently is/was a TODO comment somewhere as the color palette is/was converted to RGB in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pubpub-zz need your help here. The comment four lines below also please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is your issue here? We just convert the palette to RGB - see
pypdf/pypdf/_xobj_image_helpers.py
Lines 245 to 257 in c87f75f
# TODO : cf https://github.com/py-pdf/pypdf/pull/2039 | |
# this is a work around until PIL is able to process CMYK images | |
elif mode == "CMYK": | |
_rgb = [] | |
for _c, _m, _y, _k in ( | |
lookup[n : n + 4] for n in range(0, 4 * (len(lookup) // 4), 4) | |
): | |
_r = int(255 * (1 - _c / 255) * (1 - _k / 255)) | |
_g = int(255 * (1 - _m / 255) * (1 - _k / 255)) | |
_b = int(255 * (1 - _y / 255) * (1 - _k / 255)) | |
_rgb.append(bytes((_r, _g, _b))) | |
lookup = b"".join(_rgb) | |
mode = "RGB" |
Pillow>=10.1.0
, this might be changed: python-pillow/Pillow@5957f10
What happens if you enable the assertion in line 480?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FAILED tests/test_filters.py::test_index_lookup - AssertionError: assert 'RGBA' == 'PA'
- PA
+ RGBA
This may be outdated, "PA" is palette? But now we use "P"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay - I must have missed this. The original comment is still correct - we currently use RGB here instead (or RGBA, if you consider the alpha channel). Apparently, it should be P/PA instead (P is a color mode which uses a (custom) palette to map to another color mode, PA is the variant with an alpha channel).
Upstream documentation of color modes: https://pillow.readthedocs.io/en/stable/handbook/concepts.html#modes
Also define bytes directly, and modify some comments.
Also remove an underscore from a test name, define bytes directly, and modify some comments.