Skip to content

Simplify pdf image output.#15175

Merged
timhoffm merged 1 commit intomatplotlib:masterfrom
anntzer:pdfimage
Sep 12, 2019
Merged

Simplify pdf image output.#15175
timhoffm merged 1 commit intomatplotlib:masterfrom
anntzer:pdfimage

Conversation

@anntzer
Copy link
Copy Markdown
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
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
Copy Markdown
Member

@tacaswell tacaswell Sep 3, 2019

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
Copy Markdown
Contributor Author

@anntzer anntzer Sep 3, 2019

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Member

timhoffm commented Sep 9, 2019

I would be happier with that 😃.

@anntzer
Copy link
Copy Markdown
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