Skip to content

Support not embedding glyphs in svg mathtests. #22881

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

Merged
merged 1 commit into from
Jun 12, 2022

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Apr 23, 2022

PR Summary

Redo of #19201 (decrease size of mathtext svg baselines, by switching to embedding text as text, not as paths), but without removing the other formats or restricting fonts. Suggested as a preliminary to #22852 (which would just need to move the affected tests to the new svgastext_math_tests list.

Intentionally contains an example test in a second commit, which is reverted by the third commit: checkout the second commit to test locally. I will remove these two commits from history (and hence get rid of the "unclean PR" lint) if this PR gets approved.

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).

This reverts commit ce14a1b.

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.

I guess I'm no clear on this - how do we normally embed fonts in SVGs? If as shapes, then shouldn't we test that?

@anntzer
Copy link
Contributor Author

anntzer commented May 18, 2022

As paths, but all the existing mathtext tests already test that. The point is not to convert the old tests to embed-as-glyphs (which would indeed affect the testing of the embed-as-shape path), but to make the new ones use the embed-as-glyphs codepath. If we ever get to a point where all the old tests have been removed (seems unlikely), we can always rediscuss how to restore some embed-as-paths tests.

@jklymak
Copy link
Member

jklymak commented May 18, 2022

OK, but I don't understand the motivation, other than a moderate optimization of the repo size. If we embed paths, that seems to be what we should test, even for new tests.

@timhoffm
Copy link
Member

I think most of the time,we're not interested in testing the shape of the glyph path (and the algorithm of that). This glyph should be in that position is enough, and potentially more stable. If the path generation changes all tests would be broken even when the are testing primarily other aspects.

@anntzer
Copy link
Contributor Author

anntzer commented Jun 11, 2022

@jklymak Are you happy with the explanation given by @timhoffm (which I agree with) and willing to merge this? (If you approve this, I will squash out the second and third commits, which are making the pr_clean check fail -- see top message.)

@timhoffm
Copy link
Member

Can self-merge after squashing.

@anntzer
Copy link
Contributor Author

anntzer commented Jun 12, 2022

squashed

@anntzer anntzer force-pushed the mathtextsvgtests branch from 681e264 to 7402350 Compare June 12, 2022 08:20
@oscargus oscargus merged commit 10b26c9 into matplotlib:main Jun 12, 2022
@anntzer anntzer deleted the mathtextsvgtests branch June 12, 2022 09:46
@QuLogic QuLogic added this to the v3.6.0 milestone Jun 15, 2022
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