GeezerVison is showing github.com — Cleanup _mathtext internal API by anntzer · Pull Request #22204 · matplotlib/matplotlib
Conversation
anntzer
commented
Jan 11, 2022
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
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.
anntzer
changed the title
Remove _mathtext.Node.grow.
Cleanup _mathtext internal API
Jan 11, 2022
|
Is StandardPsFont at all now? I couldn't really figure out where it was used if so.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Good catch, it's become unused per #18722; removed it.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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.
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
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
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
|
Edit: also removed the unused postscript_name glyph attribute (as part of the second commit, edited accordingly).
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
I think that SubSuperCluster is also unused:
|
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__([]) |
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
oscargus
commented
Feb 17, 2022
•
edited
Loading
Uh oh!
There was an error while loading. Please reload this page.
|
Restarting CI. (Edit: that didn't restart all tests...)
|
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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(?)
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).
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.
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.
It doesn't need to be a nested class.
QuLogic
added a commit
to QuLogic/matplotlib
that referenced
this pull request
Aug 3, 2023
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
added a commit
to QuLogic/matplotlib
that referenced
this pull request
Aug 3, 2023
This is never called, is on an internal class, and the attribute it
accesses doesn't exist as it was removed in matplotlib#22204.
Reviewers
QuLogic
QuLogic approved these changes
oscargus
oscargus approved these changes
Uh oh!
There was an error while loading. Please reload this page.
Assignees
No one assigned
Development
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
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.
|