GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Cleanup _mathtext internal API by anntzer · Pull Request #22204 · matplotlib/matplotlib

Conversation

@anntzer

@anntzer anntzer commented Jan 11, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

They are split into individual commits

Remove _mathtext.Node.grow.

It has never been used, and the implementation was in fact buggy (if one
calls shrink many times beyond num_size_levels, .size would continue
to decrease while .width would not; if one then calls grow, .size
and .width would then both increase, but get out of sync with one
another). I can't find a reference to grow() in the TeXbook or "TeX the
program" either(?)

Stop computing glyph_name, postscript_name in mathtext font handling.

None of the MathtextBackends actually accesses these attributes anymore,
and they are the only public API that can interact with the glyph infos
returned by the Fonts classes (and there is no public API to register
new MathtextBackends).

Remove Fonts.{set_canvas_size,width,height,descent,get_results}.

We can just inline the calls directly to the backend itself at the sole
use site (in _parse_cached). The width, height, and depth attributes on
Fonts are never used either.

Remove used_characters tracking in mathtext.

Backends that need to track what characters have been used (pdf/ps, for
subsetting purposes) already have their own logic to do so, so don't do
it in mathtext as well.

Changes in Fonts are only touching private API.

Changes in the call signature of MathtextBackendAgg/MathtextBackendPath
look like they are touching public API, but it is in fact impossible for
an external user to construct a valid box parameter (this requires
classes defined in _mathtext), so get_results is effectively private
API from a caller PoV. It is also impossible to register new
MathtextBackends without touching private API.

Thus, the only difference is in the tuple returned by
MathtextBackendAgg, which now has one fewer element. Because the return
value is a tuple which is unpacked at the call site, it is not possible
to emit a proper deprecation warning (one would need to know whether the
result is being unpacked to 6 or 7 values). Callers can work around
this by e.g. unpacking the last item into *_. Note that because this
is only for MathtextBackendAgg (i.e., a raster backend), it is unlikely
that callers actually care about the used_characters info at all.

Expire rename_parameter in _mathtext.

Lift Parser.State to toplevel as standalone class.

It doesn't need to be a nested class.

Remove now unused StandardPsFonts, latex_to_{standard,cmex}, use_cmex.

Remove unused SubSuperCluster.

PR Summary

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@anntzer anntzer added topic: text/mathtext Maintenance labels Jan 11, 2022
@anntzer
anntzer force-pushed the mathtextungrow branch 2 times, most recently from 9bc7667 to e3236e9 Compare January 11, 2022 23:06
@anntzer anntzer changed the title Remove _mathtext.Node.grow. Cleanup _mathtext internal API Jan 11, 2022
@anntzer
anntzer force-pushed the mathtextungrow branch 2 times, most recently from cad2d3b to bf96d42 Compare January 12, 2022 10:41
@oscargus

oscargus commented Jan 12, 2022

Copy link
Copy Markdown
Member

Is StandardPsFont at all now? I couldn't really figure out where it was used if so.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@anntzer

anntzer commented Jan 12, 2022

Copy link
Copy Markdown
Contributor Author

Good catch, it's become unused per #18722; removed it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@anntzer
anntzer force-pushed the mathtextungrow branch 3 times, most recently from 0129d3a to a96df50 Compare January 12, 2022 19:10
@oscargus

oscargus commented Jan 12, 2022

Copy link
Copy Markdown
Member

Maybe/probably shouldn't go here, but I've just stumbled upon doc/missing-references.json and I noted that there were a few missing references to stuff ending up in _mathtext. So things that really just should/could disappear from the file, no need to edit any documentation.

Not sure if the file is generated or hand-edited though. (If you do not think it should go here, I saw it while planning to go through it, so I can get rid of it.)

Btw, as you seem to have a good idea about the mathtextparts: if you when convenient could have a quick look at #22171 and #22173 it would be appreciated. Just got my Linux machine back up, so I should have the opportunity to generate some test images as well now if the PRs looks to be on the right track.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@anntzer

anntzer commented Jan 13, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for the ping, I commented on your two PRs.

missing-references.json gets rautoegenerated with by building the docs with make O=-Dmissing_references_write_json=1 html (after make clean first), but we typically only do that when really needed (e.g., when moving stuff around that causes a need for missing references to be updated as well). Here, we could take advantage of this to remove some entries, but that's not really necessary either because there's zero chance that these missing references will get accidentally reintroduced, and not doing so (and only updating that file once in a while) saves many rather meaningless patches in the commit history just updating that file.

oscargus reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

QuLogic
QuLogic reviewed Jan 15, 2022
View reviewed changes
Comment thread lib/matplotlib/_mathtext.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread lib/matplotlib/mathtext.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@anntzer
anntzer force-pushed the mathtextungrow branch from a96df50 to a534c92 Compare January 15, 2022 18:52
QuLogic
QuLogic approved these changes Jan 18, 2022
View reviewed changes
@QuLogic QuLogic added the status: needs rebase label Jan 18, 2022
@anntzer
anntzer force-pushed the mathtextungrow branch from a534c92 to c89e2d2 Compare January 18, 2022 10:01
@anntzer anntzer removed the status: needs rebase label Jan 18, 2022
@anntzer

anntzer commented Jan 18, 2022

Copy link
Copy Markdown
Contributor Author

rebased.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

oscargus
oscargus reviewed Jan 20, 2022
View reviewed changes
Comment thread lib/matplotlib/_mathtext.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

@anntzer
anntzer force-pushed the mathtextungrow branch from c89e2d2 to 5c6c60c Compare January 20, 2022 12:43
@anntzer

anntzer commented Jan 25, 2022

Copy link
Copy Markdown
Contributor Author

Edit: also removed the unused postscript_name glyph attribute (as part of the second commit, edited accordingly).

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@anntzer
anntzer force-pushed the mathtextungrow branch from 5c6c60c to 35259b3 Compare January 25, 2022 12:57
@oscargus

oscargus commented Jan 31, 2022

Copy link
Copy Markdown
Member

I think that SubSuperCluster is also unused:

matplotlib/lib/matplotlib/_mathtext.py

Lines 1611 to 1623 in 6eba0af

class SubSuperCluster(Hlist):
"""
A hack to get around that fact that this code does a two-pass parse like
TeX. This lets us store enough information in the hlist itself, namely the
nucleus, sub- and super-script, such that if another script follows that
needs to be attached, it can be reconfigured on the fly.
"""
def __init__(self):
self.nucleus = None
self.sub = None
self.super = None
super().__init__([])

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@anntzer

anntzer commented Jan 31, 2022

Copy link
Copy Markdown
Contributor Author

Good catch, removed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@oscargus

oscargus commented Feb 17, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Restarting CI. (Edit: that didn't restart all tests...)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@oscargus oscargus closed this Feb 17, 2022
@oscargus oscargus reopened this Feb 17, 2022
anntzer added 7 commits February 17, 2022 14:13
@anntzer
Remove _mathtext.Node.grow.
2ad10de
It has never been used, and the implementation was in fact buggy (if one
calls `shrink` many times beyond num_size_levels, `.size` would continue
to decrease while `.width` would not; if one then calls `grow`, `.size`
and `.width` would then both increase, but get out of sync with one
another).  I can't find a reference to grow() in the TeXbook or "TeX the
program" either(?)
@anntzer
Stop computing glyph_name in mathtext font handling.
4df33d2
None of the MathtextBackends actually accesses the .glyph_name
attribute anymore, and they are the only public API that can interact
with the glyph infos returned by the Fonts classes (and there is no
public API to register new MathtextBackends).
@anntzer
Remove used_characters tracking in mathtext.
a892892
Backends that need to track what characters have been used (pdf/ps, for
subsetting purposes) already have their own logic to do so, so don't do
it in mathtext as well.

Changes in Fonts are only touching private API.

Changes in the call signature of MathtextBackendAgg/MathtextBackendPath
look like they are touching public API, but it is in fact impossible for
an external user to construct a valid `box` parameter (this requires
classes defined in `_mathtext`), so `get_results` is effectively private
API from a caller PoV.  It is also impossible to register new
MathtextBackends without touching private API.

Thus, the only difference is in the tuple returned by
MathtextBackendAgg, which now has one fewer element.  Because the return
value is a tuple which is unpacked at the call site, it is not possible
to emit a proper deprecation warning (one would need to know whether the
result is being unpacked to 6 or 7 values).  Callers can work around
this by e.g. unpacking the last item into `*_`.  Note that because this
is only for MathtextBackendAgg (i.e., a raster backend), it is unlikely
that callers actually care about the used_characters info at all.
@anntzer
Remove Fonts.{set_canvas_size,width,height,descent,get_results}.
2ecbb56
We can just inline the calls directly to the backend itself at the sole
use site (in _parse_cached).  The width, height, and depth attributes on
Fonts are never used either.
@anntzer
Expire rename_parameter in _mathtext.
c00ae8e
@anntzer
Lift Parser.State to toplevel as standalone class.
afe8d9e
It doesn't need to be a nested class.
@anntzer
Remove now unused StandardPsFonts, latex_to_{standard,cmex}, use_cmex.
519d1eb
@anntzer
anntzer force-pushed the mathtextungrow branch from 69aec89 to 519d1eb Compare February 17, 2022 13:13
oscargus
oscargus approved these changes Feb 17, 2022
View reviewed changes
@QuLogic
QuLogic merged commit b03479a into matplotlib:main Feb 17, 2022
@QuLogic QuLogic added this to the v3.6.0 milestone Feb 17, 2022
@anntzer
anntzer deleted the mathtextungrow branch February 17, 2022 22:27
@anntzer anntzer mentioned this pull request Feb 17, 2022
Some more maintenance for mathtext internal implementation. #22488
Merged
6 tasks
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Aug 3, 2023
@QuLogic
Remove Fonts.get_used_characters
cf21f9b
This is never called, is on an internal class, and the attribute it
accesses doesn't exist as it was removed in matplotlib#22204.
@QuLogic QuLogic mentioned this pull request Aug 3, 2023
Fix some bugs found by typing #26444
Merged
3 tasks done
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Aug 3, 2023
@QuLogic
Remove Fonts.get_used_characters
f6793ba
This is never called, is on an internal class, and the attribute it
accesses doesn't exist as it was removed in matplotlib#22204.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@QuLogic QuLogic QuLogic approved these changes
@oscargus oscargus oscargus approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Maintenance topic: text/mathtext

Projects

None yet

Milestone

v3.6.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

@anntzer @oscargus @QuLogic
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.