Skip to content

Fix imsave output format when format keyword is None #10048

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
Closed
wants to merge 3 commits into from
Closed

Fix imsave output format when format keyword is None #10048

wants to merge 3 commits into from

Conversation

larrybradley
Copy link
Contributor

PR Summary

Calling imsave without explicitly setting the format keyword (default=None) is supposed to determine the format of the output file from the output filename extension. Currently, if format is not specified (i.e. None), imsave always saves a PNG (despite the output filename):

>>> import imghdr
>>> import matplotlib.pyplot as plt

>>> fn = 'imsave_test.jpg'
>>> plt.imsave(fn, np.zeros((10, 10)))
>>> imghdr.what(fn)    # should return 'jpeg'
'png'

This PR fixes this issue.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 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 added this to the v2.2 milestone Dec 19, 2017
@tacaswell
Copy link
Member

It looks like appveyor does not have the dependencies to save jpeg

   
        # else report error for unsupported format
        formats = sorted(self.get_supported_filetypes())
        raise ValueError('Format "%s" is not supported.\n'
                         'Supported formats: '
>                        '%s.' % (format, ', '.join(formats)))
E       ValueError: Format "jpg" is not supported.
E       Supported formats: eps, pdf, pgf, png, ps, raw, rgba, svg, svgz.
lib\matplotlib\backend_bases.py:2114: ValueError

Could you either add pillow to what is installed on appveyor or switch to testing svg (you can test that the xml header is in the output file).

@tacaswell
Copy link
Member

👍 in principle

@larrybradley
Copy link
Contributor Author

Thanks, @tacaswell. I changed the test to use svg.

tacaswell
tacaswell approved these changes Dec 19, 2017
View reviewed changes
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

modulo tests passing.

@fariza
Copy link
Member

fariza commented Dec 21, 2017 •
edited
Loading

@larrybradley travis is failing after calls to imsave

The solution would be to use the same "format check" as it is done inside backend_bases.FigureCanvasBase

if format is None:
            # get format from filename, or from backend's default filetype
            if isinstance(filename, six.string_types):
                format = os.path.splitext(filename)[1][1:]
            if format is None or format == '':
                format = self.get_default_filetype()
                ....

So, that code has to be moved somewhere else as function maybe cbook.__init_py

What about a signature style get_filename_format(filename, format)

@QuLogic QuLogic mentioned this pull request Dec 27, 2017
Closed
6 tasks
@anntzer
Copy link
Contributor

anntzer commented Dec 31, 2017

FWIW I would just get rid of the png fast path, which also does not set the dpi value. Basically

diff --git a/lib/matplotlib/image.py b/lib/matplotlib/image.py
index 8b630ce3e..e5924651e 100644
--- a/lib/matplotlib/image.py
+++ b/lib/matplotlib/image.py
@@ -1350,20 +1350,11 @@ def imsave(fname, arr, vmin=None, vmax=None, cmap=None, format=None,
     from matplotlib.backends.backend_agg import FigureCanvasAgg as FigureCanvas
     from matplotlib.figure import Figure
 
-    # Fast path for saving to PNG
-    if (format == 'png' or format is None or
-            isinstance(fname, six.string_types) and
-            fname.lower().endswith('.png')):
-        image = AxesImage(None, cmap=cmap, origin=origin)
-        image.set_data(arr)
-        image.set_clim(vmin, vmax)
-        image.write_png(fname)
-    else:
-        fig = Figure(dpi=dpi, frameon=False)
-        FigureCanvas(fig)
-        fig.figimage(arr, cmap=cmap, vmin=vmin, vmax=vmax, origin=origin,
-                     resize=True)
-        fig.savefig(fname, dpi=dpi, format=format, transparent=True)
+    fig = Figure(dpi=dpi, frameon=False)
+    FigureCanvas(fig)
+    fig.figimage(arr, cmap=cmap, vmin=vmin, vmax=vmax, origin=origin,
+                    resize=True)
+    fig.savefig(fname, dpi=dpi, format=format, transparent=True)
 
 
 def pil_to_array(pilImage):

@fariza
Copy link
Member

fariza commented Jan 8, 2018

Why does it exists that Fast path?

@anntzer
Copy link
Contributor

anntzer commented Jan 8, 2018

maybe worth timing whether it is actually faster...

@anntzer anntzer mentioned this pull request Jan 11, 2018
Merged
6 tasks
@anntzer
Copy link
Contributor

anntzer commented Jan 12, 2018

Looks like the tests are failing because imsave("foo") (with default rcParams) is now using the "full figure" codepath which gives slightly different results. Probably some tests need to explicitly set the format to png instead.

@tacaswell
Copy link
Member

Closing this as equivalent changes went in with #10231 .

Thanks for you work on this @larrybradley ! Hope to hear from you again.

@tacaswell tacaswell closed this Jan 21, 2018
@QuLogic QuLogic modified the milestones: needs sorting, v2.2.0 Feb 12, 2018
@larrybradley larrybradley deleted the fix-imsave-format branch November 13, 2018 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tacaswell tacaswell tacaswell approved these changes

Assignees
No one assigned
Labels
status: duplicate
Projects
None yet
Milestone
v2.2.0
Development

Successfully merging this pull request may close these issues.
5 participants
@larrybradley @tacaswell @fariza @anntzer @QuLogic