Skip to content

Do not distill and embed fonts if no text #23775

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.

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

oscargus
Copy link
Member

PR Summary

Closes #23253

With this, there is no distill to embed psfrags, if there are no psfrags. So smaller files, for the example in #23253 reduced from 161k to 800 bytes. This will primarily only make a difference when usetex=True, e.g. from a style-file, but there is no text in the plot.

I do not really know how to test this properly, but all current tests pass if nothing else.

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).

@oscargus oscargus force-pushed the smallerpsfiles branch 2 times, most recently from 4512594 to 65125a3 Compare August 30, 2022 11:02
@tacaswell tacaswell added this to the v3.7.0 milestone Aug 30, 2022
@tacaswell
Copy link
Member

I think a test that writes the empty figure to a BtyesIO buffer and either checks that font is not there in the postscript or just that it is small enough would be enough.

@oscargus
Copy link
Member Author

Yes, that is indeed one way to test it. I was actually more worried about breaking something than still producing large files, but I'll add a file size test.

@oscargus
Copy link
Member Author

There is now a test. However, the figure size selection logic before distilling is not really obvious to me. It may be that it is just needed for distilling as the resulting "paper size" is the same here for both ps and eps (the figure size), but there are for sure cases where this doesn't happen, like when the figure is larger than letter for ps. So I'm a bit doubtful if is may break something else...

@oscargus
Copy link
Member Author

oscargus commented Aug 31, 2022

Not distilling leads to that ps-images are no longer in "paper size", but in figure size. I cannot really see where this is a problem, but since this is breaking current behavior, I put this in draft stage and set it up for discussion on the dev call.

There is a quite simple fix for this, determining paper size, before writing the header, but we should probably discuss what the "correct" behavior is for different cases.

@oscargus oscargus marked this pull request as draft August 31, 2022 07:53
@anntzer
Copy link
Contributor

anntzer commented Aug 31, 2022

FWIW I think we should eventually kill the papersize kwarg and (if we want to keep an equivalent feature) let users specify e.g. figsize=mpl.get_papersize("a4") (or even figsize="a4" if we really want that conciseness) in the Figure constructor, to make things more regular across backends (and avoid e.g. mplcairo having to reproduce that logic).

def test_noembed_fonts():
"""Test that fonts are not embedded when no text is used"""
mpl.rcParams["text.usetex"] = True
fig, ax = plt.subplots()
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess you can skip axes creation (fig = plt.figure()).

@oscargus oscargus removed this from the v3.7.0 milestone Dec 15, 2022
@oscargus
Copy link
Member Author

As it seems like @QuLogic is doing an overhaul of the PS backend, I tag you in.

IIRC the discussion at the dev-call was about if someone really needs ps output anymore or if we should go all eps. This is combination with @anntzer's good remark about the use of the papersize argument above should probably be the subject of a new dev-call to possibly make a decision on the subject.

Possibly related: #22416 as it involves the eps/ps distill chain as well.

@QuLogic
Copy link
Member

QuLogic commented Apr 28, 2023

I think in principle, this makes sense, but I guess I need to do some tests to see what the exact results are.

@oscargus
Copy link
Member Author

Yes, I do not recall exactly why I put this into draft. I think it was because I did not really get the full sequence of distill-runs. But I guess that, if possible, it would make sense to avoid embedding if not used.

I also know that I wanted to add a test to make sure that one actually gets EPS or PS. Maybe I mix up these topics... Or the issue here was that unless we distilled the "EPS" would end up to be a PS. Or something...

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.

[Bug]: Font always embedded in (e)ps when text.usetex = True
4 participants