Skip to content

Conversation

marychrisgo
Copy link
Contributor

PR Summary

This is solving a request in issue##17708 by @timhoffm. I added an explanation of the rasterization_demo.py.

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/next_api_changes/* if API changed in a backward-incompatible way

@@ -4,50 +4,57 @@
==================

"""

# Rasterization is a method where an image described in a vector graphics format is being converted into a raster image (pixels).
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a sentence about how we can flag particular parts of a Figure to be rasterized and some comments about why you would want to do this? Something like:

This demo is showing how you may want to balance most things (text, lines, ...) being vectorized (where you get all of the benefits of vector graphics), but turning some of the artists into rasterized images (because embedding a 2k by 2k mesh in an SVG results in a huge file that takes a long time to both save and load).

?

@tacaswell
Copy link
Member

Thanks for working on this @marychrisgo !

We enforce a sub-set of the flake8 code style checks on all of the Python source in our repo, hopefully the linter's comments are clear?

@tacaswell tacaswell added this to the v3.4.0 milestone Jul 22, 2020
@marychrisgo
Copy link
Contributor Author

Hello @tacaswell ! I've read all the comments and resolved all the issues: Flake8 guide and your suggested paragraph. Thanks!

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Sorry, quite a few comments, which you can take or leave. However, I guess I'd really ask that the reference to set_rasterization_zorder be removed. That just seems like a terrible API...

ax4.text(0.5, 0.5, "Text", alpha=0.2,
zorder=-15,
va="center", ha="center", size=50, transform=ax4.transAxes)

# Set zorder value below which artists will be rasterized
Copy link
Member

Choose a reason for hiding this comment

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

What? I don't even think this should be a thing, and we definitely should not advertise it. The artists we want rasterized should explicitly get a rasterized=True kwarg, not depend on zorder, which should not have anything to do with rasterization.

Copy link
Contributor

Choose a reason for hiding this comment

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

set_rasterization_zorder was meant to be used for the ps backend family which does not support transparency, i.e., rasterizing an artist may affect the appearance of other artists with lower zorder. So the idea was rasterizing all artists below certain zorder and then add non-rasterized artists on top of it (like texts).

ax2.set_aspect(1)
ax2.set_title("Rasterization")

m = ax2.pcolormesh(xx, yy, d)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is better spelled as ax2.pcolormesh(xx, yy, d, rasterized=True)

# ax2.title.set_rasterized(True) # should display a warning

# Save files in pdf and eps format
plt.savefig("test_rasterization.pdf", dpi=150)
Copy link
Member

Choose a reason for hiding this comment

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

Why are we saving these?

Can you also compare how big they are w/o the rasterization to with the rasterization?

@marychrisgo
Copy link
Contributor Author

Hello @jklymak ! Thank you for the comments. I've edited the paragraph you suggested. I won't edit the coding part as this PR is mainly for the documentation.

@timhoffm
Copy link
Member

I won't edit the coding part as this PR is mainly for the documentation.

In the end, the whole example is documentation (the code is for illustration, it's not part of the library). We encourage to also make the example code more concise where possible.

@marychrisgo
Copy link
Contributor Author

Thank you @timhoffm ! Changing some based on your comments, pushing a new request now.

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Thanks, I will take care of the code changes requested by @jklymak later.

@timhoffm timhoffm merged commit e54cf8f into matplotlib:master Jul 29, 2020
@tacaswell
Copy link
Member

Thinks for this work @marychrisgo ! If I recall correctly this was your first Matplotlib pr 🎉 ! Hopefully we will hear from you again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants