Skip to content

Simplify _png extension by handling file open/close in Python. - #15095

Merged
jklymak merged 1 commit into
matplotlib:masterfrom
anntzer:pngfile
Aug 22, 2019
Merged

jklymak merged 1 commit into
matplotlib:masterfrom
anntzer:pngfile

Conversation

@anntzer

@anntzer anntzer commented Aug 21, 2019

Copy link
Copy Markdown
Contributor

Make functions in the (private) _png extension only take file-like
objects as arguments, rather than "filename or file-like". Filenames
were anyways handled by calling the python C-API to open/close the
files, so we may as well just do this from Python where a simple
with open(...): does the job. The public API is not changed.

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

Make functions in the (private) _png extension only take file-like
objects as arguments, rather than "filename or file-like".  Filenames
were anyways handled by calling the python C-API to open/close the
files, so we may as well just do this from Python where a simple
`with open(...):` does the job.  The public API is not changed.
fname_img = "%s-img%d.png" % (fname, self.image_counter)
self.image_counter += 1
_png.write_png(im[::-1], os.path.join(path, fname_img))
with pathlib.Path(path, fname_img).open("wb") as file:

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.

I don't quite like assigning to builtin names, but acknowledge that there's already precedence for this in the library.

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.

It's not a builtin name anymore...

@jklymak jklymak left a comment

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.

Certainly a lot simpler!

@jklymak
jklymak merged commit b779492 into matplotlib:master Aug 22, 2019
@jklymak jklymak added this to the v3.2.0 milestone Aug 22, 2019
@anntzer
anntzer deleted the pngfile branch August 22, 2019 16:22
@anntzer anntzer mentioned this pull request Aug 22, 2019
6 tasks
@moorepants

Copy link
Copy Markdown

The public API is not changed.

Even though the public API hasn't changed, i think this does seem to change the error types raised and has broken downstream code in SymPy. See this issue: sympy/sympy#18815

The offending changed lines are:

https://github.com/matplotlib/matplotlib/blame/v3.2.0/lib/matplotlib/mathtext.py#L3436

I haven't determined yet if it is ultimately SymPy's bad design for relying on a specific error but this PR might cause all Jupyter notebook printing for SymPy to break once people install matplotlib 3.2.0.

@tacaswell

Copy link
Copy Markdown
Member

Can you try with the 3.2.x branch? We just merged #16709 which I think is related.

@QuLogic

QuLogic commented Mar 10, 2020

Copy link
Copy Markdown
Member

Yes, this is the same bug; see backtrace on linked issue.

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.

6 participants