8000 Added compression option to save TIFF images by renato-umeton · Pull Request #8531 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Added compression option to save TIFF images #8531

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 9 commits into from
Closed

Added compression option to save TIFF images #8531

wants to merge 9 commits into from

Conversation

renato-umeton
Copy link
@renato-umeton renato-umeton commented Apr 24, 2017 •
edited
Loading

PR Summary

TIFF is the scientific journals' preferred format; LZW compression is needed to avoid generating huge (500MB+) images that cannot be readily uploaded to the journal websites during the submission process. Currently we generate huge TIFF image files with MPL, then import them in PIL, save them in PIL using compression of the TIFF and then discarding the uncompressed TIFF files. This PR is about adding support for LZW compression when saving TIFF images.

#8530

PR Checklist

  • [NO TESTS FOR SAVE TIF] 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
  • [NOT MAJOR] Added an entry to doc/users/whats_new.rst if major new feature
  • [N/A] Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

Dana-Farber added 5 commits April 24, 2017 13:58
Add LZW compression option to save TIFF images
073f1b9 
TIFF is the scientific journals' preferred format; LZW compression is needed to avoid generating 500MB+ images which cannot be readily uploaded to the journal websites during the submission process
Improved adhesion to code standards
14d0c30 
minor fix in the char spacing
Added TiffImagePlugin import required in print_tif
ad850c2
Fix typo that broke building process
f4055ec
Add comments and better spacing for TIFF LZW
1ed0ef9
@story645
Copy link
Member
story645 commented Apr 25, 2017 •
edited
Loading

I think this needs a test even if .tiff doesn't have one 'cause there needs to be a way to reliably verify what's happening, 'specially since apparently compression in LZW is apparently wonky and needs the WRITE_LIBTIFF flag set first.

Can you use test_png.py as a model?

story645
story645 reviewed Apr 25, 2017
View reviewed changes
lib/matplotlib/backends/backend_agg.py Outdated
#add TIFF compression support
compressed = kwargs.pop("compressed", False)
if compressed:
libtiff_original_value = TiffImagePlugin.WRITE_LIBTIFF
Copy link
Member

Choose a reason for hiding this comment

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

Is it ok to simplify this as?

TiffImagePlugin.WRITE_LIBTIFF = compressed

Or are there instances where the user needs TiffImagePlugin.WRITE_LIBTIFF to be True even when writing out a non-compressed image?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I'll add the test part.

I'd say it is not OK to "simplify" because you may use the flag somewhere else and remain confused about it not getting back to its original state once finished saving the image.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably better to use try:... finally:... to handle the case where the saving fails.

Copy link
Author

Choose a reason for hiding this comment

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

@story645 I'm having some issues coming up with some meaningful tests since we now do a pull pass-through of the parameters to the backend. Can you suggest some tests, if needed?

Copy link
Member

Choose a reason for hiding this comment

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

Check that the arg is getting passed through properly and that what this function saves out is a valid compressed tiff file (just try one compression arg parameter, does not matter which one).

@cgohlke
Copy link
Contributor
cgohlke commented Apr 25, 2017

May I suggest to replace the compressed parameter by compression and pass that parameter to the image.save function if it is provided and not None. That way we can make use of all the TIFF compression options supported by Pillow. TIFF deflate (zlib) is usually a better compression than LZW.

@dopplershift
Copy link
Contributor

👍 to @cgohlke 's suggestion. It's cleaner, more general, and gives us functionality for free.

@renato-umeton renato-umeton changed the title Added LZW compression option to save TIFF images Added compression option to save TIFF images Apr 26, 2017
@renato-umeton
Copy link
Author

Done, I've incorporated also cgohlke's idea -- that gives us indeed free stuff. Good thinking!

story645
story645 requested changes Apr 26, 2017
View reviewed changes
lib/matplotlib/backends/backend_agg.py Outdated
8000
TiffImagePlugin.WRITE_LIBTIFF = libtiff_original_value
return return_value
#add TIFF compression support by passing the parameter to backend
compressed = kwargs.pop("compression", None)
Copy link
Member

Choose a reason for hiding this comment

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

compressed = kwargs.pop("compression", None)
should be
compression = kwargs.pop("compression", None)

lib/matplotlib/backends/backend_agg.py Outdated
return return_value
#add TIFF compression support by passing the parameter to backend
compressed = kwargs.pop("compression", None)
if compression is not None:
Copy link
Member

Choose a reason for hiding this comment

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

don't need an if/else anymore, since compression=None is a valid arg to pass through, so you can just do:
return image.save(filename_or_obj, format='tiff', dpi=dpi, compression=compression)

@QuLogic QuLogic added this to the 2.1 (next point release) milestone Apr 26, 2017
@renato-umeton
Copy link
Author

@story645 Done all the requested changes. Regarding the tests, the part that is not working is writing to io.BytesIO() -- for some reason it works for PDFs but not for TIFFs. This would explain the current lack of tests for the TIFF section... Any ideas? Thanks!

@story645
Copy link
Member

Can you post your test (here or as a gist?) having trouble understanding what you mean.

@renato-umeton
Copy link
Author
renato-umeton commented Apr 26, 2017 •
edited
Loading

@story645

This works:

x = np.random.random(20000)
y = np.random.random(20000)

fig = plt.figure()
ax = fig.add_subplot(111)
ax.plot(x, y, 'k.')
ax.set_xlim(2, 3)

pdf = io.BytesIO()
fig.savefig(pdf, format="pdf")
assert len(pdf.getvalue()) < 8000

This does not -- if I save to "/tmp/a.tiff" instead of io.BytesIO then everything goes smooth. Ideas?

x = np.random.random(20000)
y = np.random.random(20000)

fig = plt.figure()
ax = fig.add_subplot(111)
ax.plot(x, y, 'k.')
ax.set_xlim(2, 3)

tif = io.BytesIO()
fig.savefig(tif, format="tiff", compression="zlib")
assert len(tif.getvalue()) < 8000
8000

@story645
Copy link
Member

What length are you getting for len(tif.getvalue()) and what len do you get for a non compressed tiff?

@renato-umeton
Copy link
Author

I get an exception even before being able to compute length: if the filename is absent (ie, we write to io.BytesIO() then the exception is triggered. I'm not sure if I can write a test that writes to /tmp since it would pass on *nix but fail on windows. Is there a temporary directory I can write to at test-time?

@story645
Copy link
Member

Can you post the error?and your environment/mpl version?

Only because I'm not getting any error for:

y = np.random.random(20000)

fig = plt.figure()
ax = fig.add_subplot(111)
ax.plot(x, y, 'k.')
ax.set_xlim(2, 3)

tif2 = io.BytesIO()
fig.savefig(tif2, format="tiff")
len(tif2.getvalue())

@story645
Copy link
Member

Hi, just wanna follow up on this...still interested in working on it?

@tacaswell tacaswell modified the milestones: 2.1 (next point release), 2.2 (next next feature release) Aug 29, 2017
@anntzer anntzer mentioned this pull request Jan 3, 2019
Merged
6 tasks
@anntzer
Copy link
Contributor
anntzer commented Jan 17, 2019

Superseded by #13094, thanks for the PR.

@anntzer anntzer closed this Jan 17, 2019
@QuLogic QuLogic modified the milestones: needs sorting, v3.1 Jan 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@story645 story645 story645 requested changes

@anntzer anntzer anntzer left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.1.0
Development

Successfully merging this pull request may close these issues.
7 participants
@renato-umeton @story645 @cgohlke @dopplershift @anntzer @tacaswell @QuLogic
0