Skip to content

Simplify pdf image output. #15175

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Sep 12, 2019
Merged

Simplify pdf image output. #15175

merged 1 commit into from
Sep 12, 2019
+20 −24

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Sep 3, 2019

  • Inline _unpack into its only caller writeImages.
  • Let _writeImg infer the image size and grayscaleness from the data
    shape.

preliminary work towards #15165.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

tacaswell
tacaswell reviewed Sep 3, 2019
View reviewed changes
lib/matplotlib/backends/backend_pdf.py
@@ -1455,26 +1431,26 @@ def _writePng(self, data):
buffer.seek(length, 1)
buffer.seek(4, 1) # skip CRC

def _writeImg(self, data, height, width, grayscale, id, smask=None):
def _writeImg(self, data, id, smask=None):
Copy link
Member

Choose a reason for hiding this comment

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

do we ever actually pass in a grayscale image?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We do pass single-channel images (for alpha). As for actual grayscale I don't know (see #12871 for the similar thing in postscript), but this PR doesn't change that behavior anyways.

@tacaswell tacaswell added this to the v3.3.0 milestone Sep 3, 2019
@anntzer anntzer mentioned this pull request Sep 5, 2019
Merged
6 tasks
@timhoffm
Copy link
Member

timhoffm commented Sep 9, 2019

What is the benefit of inlining? To me this looks more cluttered now. The original _unpack seemed like a reasonable abstraction layer.

@anntzer
Copy link
Contributor Author

anntzer commented Sep 9, 2019

For me this made #15193 easier to write (even though strictly speaking it is orthogonal) -- as things stand it is a bit annoying to have to go through multiple places to track the shapes of the arrays involved.
Although I guess strictly speaking I could do without the inlining of _unpack, and just get rid of width, height and greyscale as arguments to _writeImg.

@timhoffm
Copy link
Member

timhoffm commented Sep 9, 2019

I would be happier with that 😃.

@anntzer
Copy link
Contributor Author

anntzer commented Sep 9, 2019

sure, done

QuLogic
QuLogic approved these changes Sep 11, 2019
View reviewed changes
@anntzer
Simplify pdf image output.
2a63d63 
Let _writeImg infer image size and grayscaleness from the data itself.
@timhoffm timhoffm merged commit 56cfb43 into matplotlib:master Sep 12, 2019
@anntzer anntzer deleted the pdfimage branch September 12, 2019 07:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell left review comments

@QuLogic QuLogic QuLogic approved these changes

@timhoffm timhoffm timhoffm approved these changes

Assignees
No one assigned
Labels
backend: pdf
Projects
None yet
Milestone
v3.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@anntzer @timhoffm @tacaswell @QuLogic