FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix imread issues mentioned on matplotlib-devel by cgohlke · Pull Request #616 · matplotlib/matplotlib · GitHub

Repository navigation

Fix imread issues mentioned on matplotlib-devel - #616

Closed
cgohlke wants to merge 8 commits into
matplotlib:masterfrom
cgohlke:master
Closed

cgohlke wants to merge 8 commits into
matplotlib:masterfrom
cgohlke:master

Conversation

cgohlke commented Dec 9, 2011

Copy link
Copy Markdown
Contributor

cgohlke commented Dec 9, 2011

Copy link
Copy Markdown
Contributor Author

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 commented Dec 9, 2011

Copy link
Copy Markdown
Member

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?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Dec 14, 2011

Copy link
Copy Markdown
Member

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 commented Feb 26, 2012

Copy link
Copy Markdown
Collaborator

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 commented Mar 1, 2012

Copy link
Copy Markdown
Contributor Author

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 commented Jun 1, 2012

Copy link
Copy Markdown
Member

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 commented Jun 5, 2012

Copy link
Copy Markdown
Member

@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 commented Jun 5, 2012

Copy link
Copy Markdown
Member

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 commented Jun 5, 2012

Copy link
Copy Markdown
Member

Merged into master

mdboom closed this Jun 5, 2012

cgohlke commented Jun 5, 2012

Copy link
Copy Markdown
Contributor Author

Thanks. My matplotlib fork got corrupted.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants


Back | FazBrowse Home | New Git URL