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

Simplify pdf image output. by anntzer · Pull Request #15175 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Simplify pdf image output. - #15175

Merged
timhoffm merged 1 commit into
matplotlib:masterfrom
anntzer:pdfimage
Sep 12, 2019
ConversationCommits (1)ChecksFiles changed
Merged

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

Conversation

anntzer commented Sep 3, 2019

Copy link
Copy Markdown
Contributor
  • 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 reviewed Sep 3, 2019
View reviewed changes
Comment thread 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):

tacaswell Sep 3, 2019

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

do we ever actually pass in a grayscale image?

anntzer Sep 3, 2019

Copy link
Copy Markdown
Contributor Author

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

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 added this to the v3.3.0 milestone Sep 3, 2019
anntzer mentioned this pull request Sep 5, 2019
Merged
6 tasks

timhoffm commented Sep 9, 2019

Copy link
Copy Markdown
Member

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

anntzer commented Sep 9, 2019

Copy link
Copy Markdown
Contributor Author

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 commented Sep 9, 2019

Copy link
Copy Markdown
Member

I would be happier with that 😃.

anntzer commented Sep 9, 2019

Copy link
Copy Markdown
Contributor Author

sure, done

QuLogic approved these changes Sep 11, 2019
View reviewed changes
Comment thread lib/matplotlib/backends/backend_pdf.py Outdated
Simplify pdf image output.
2a63d63 
Let _writeImg infer image size and grayscaleness from the data itself.
timhoffm merged commit 56cfb43 into matplotlib:master Sep 12, 2019
anntzer deleted the pdfimage branch September 12, 2019 07:00
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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

tacaswell tacaswell left review comments
QuLogic QuLogic approved these changes
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

Back | FazBrowse Home | New Git URL