Skip to content

Fix imread issues mentioned on matplotlib-devel #616

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

Closed
wants to merge 8 commits into from
Closed

Fix imread issues mentioned on matplotlib-devel #616

wants to merge 8 commits into from

Conversation

cgohlke
Copy link
Contributor

@cgohlke cgohlke commented Dec 9, 2011

@cgohlke
Copy link
Contributor Author

cgohlke commented Dec 9, 2011

Other possible options for handling 16 bit images:

  1. just divide by 256 before casting to uint8, possibly loosing dynamic range (10 and 12 bit images are common).
  2. allow pil_to_array() to return uint16 arrays. The docstring says that uint8 is returned.

@mdboom
Copy link
Member

mdboom commented Dec 9, 2011

I like the idea of pil_to_array returning a 16 bit array when the input image is 16-bit. It allows the caller to deal with downsampling to 8-bit in the most appropriate way. We can update the docstring to indicate that the dtype of the output depends on the input file or some such. I think it's reasonable to make small backward-incompatible changes like this on master (just add to the changelog). While we're at it, can we add a test for this? Showing that a 16-bit input image is correctly returned as a 16-bit array?

img = plt.imread(os.path.join(os.path.dirname(__file__),
'baseline_images/test_image/uint16.tif'))
assert (img.dtype == np.uint16)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this be turned into an image comparison test to ensure that the fields of the image are correctly read in, endianness works etc. Or if that's not practical, maybe assert about a few of the data values?

@mdboom
Copy link
Member

mdboom commented Dec 14, 2011

I have one small note about testing. This doesn't (I believe) change the fact that PNG images read through the _png extension (i.e. not through PIL) are still converted to 8-bit. We should fix that as well.

@jdh2358
Copy link
Collaborator

jdh2358 commented Feb 26, 2012

This PR looks useful but discussion seems to have died down. I'd like to get this in the bugfix release. @cgohlke , any chance you could change the src to v1.1.x rather than master. Do you have any feedback from @mdboom comments?

@cgohlke
Copy link
Contributor Author

cgohlke commented Mar 1, 2012

I don't think all of these changes can go into v1.1.x. Not flipping PIL images or allowing 16 bit images might break existing applications. The _png extension should be fixed in another PR. It is currently broken in master (test_png .py fails).

@mdboom
Copy link
Member

mdboom commented Jun 1, 2012

I agree this shouldn't be on 1.1.x, but it should get merged into master.

I've added an issue #915 to track adding 16-bit support to the _png extension.

As far as I can tell, the test_png.py failure is currently obsolete -- it is passing on master now.

I think the following would make this mergeable:

  1. Add a few asserts to the unit test to ensure the data is read correctly. I think assert that "sum" of the pixels is the correct value would be adequate to ensure there were no endianness problems, etc.

  2. Rebase this on current master

  3. Make sure the regression tests still pass.

@mdboom
Copy link
Member

mdboom commented Jun 5, 2012

@cgohlke -- it looks like your fork of matplotlib has been removed, making it impossible for me to review this pull request. Any idea what might have happened?

@mdboom
Copy link
Member

mdboom commented Jun 5, 2012

I was able to get the diff manually from here:

https://github.com/matplotlib/matplotlib/pull/616.diff

so all is not lost -- I should be able to verify and merge this by hand.

mdboom added a commit that referenced this pull request Jun 5, 2012
@mdboom
Copy link
Member

mdboom commented Jun 5, 2012

Merged into master

@mdboom mdboom closed this Jun 5, 2012
@cgohlke
Copy link
Contributor Author

cgohlke commented Jun 5, 2012

Thanks. My matplotlib fork got corrupted.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants