-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Fix tests for Image.fromarray() #1824
Conversation
Changes Unknown when pulling 85c3f36 on mairsbw:moar_tests into * on python-pillow:master*. |
Changes Unknown when pulling 93a44df on mairsbw:moar_tests into * on python-pillow:master*. |
_test_image(0, "Tests/images/uint16_1_0.tif") | ||
_test_image(255, "Tests/images/uint16_1_255.tif") | ||
_test_image(65280, "Tests/images/uint16_1_65280.tif") | ||
_test_image(65535, "Tests/images/uint16_1_65535.tif") |
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.
I think all of these can be coalesced into a single save/load of a small image with data == [0,255, 65280, 65535].
Thinking about the way that these can fail, I'm not sure if this test would catch the file being saved and reloaded as an int32, as that should be what the storage mode is.
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.
The problem here is that there are a ton of stages to saving and loading images with Pillow. On loading there's the initial reading of the fileformat, the conversion to an internal array, the conversion to a proper Pillow image class. Now on saving there's the conversion of the internal array to the format specified by the image class and then writing that in an appropriate format to disk.
I've been having a lot of trouble trying to get all of these different pieces in order because there definitely seems to be some lacking support, or incorrect behavior, in various pieces of the chain. This is why I'm interested in adding tests that just improve coverage and should be correct as we'll need a bunch more than this one to test that a uint16 image can be loaded as a uint16 array and then written back out as one.
So are you suggesting that this test be dropped, replaced, or extended with other tests? As I believe the latter is appropriate and what I plan to do once I get this first round of changes done.
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.
Let me add the comment that I'd like to keep the image the same repeated uint16 value tiled over a 10x10 image for consistency with other tests. I'll condense this to a single image, however.
Currently failing for int16LE as of this commit.
I've incorporated your comments about using a single image to test uint16 reading and writing and moving that constant to module-scope. Hopefully these tests are more amenable. |
You may already be aware, but I'll just point out here that there are merge conflicts in this branch. |
@radarhere I'll fix these tests if you think they'd be worth pulling in. Otherwise, I'll just close this PR. |
It looks like it's a minor merge issue, and the failing test is due to not having an int64 type in pillow:
in this:
This test will likely pass on a 32 bit system. |
This also makes explicit the existing image support for different datatypes fed to
Image.fromarray()
.This is part of the base work to resolve #1514 and #1819.