Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

Cleanup _mathtext internal API#22204

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
QuLogic merged 7 commits intomatplotlib:mainfromanntzer:mathtextungrow
Feb 17, 2022
Merged

Conversation

anntzer
Copy link
Contributor

@anntzeranntzer commentedJan 11, 2022
edited
Loading

They are split into individual commits

Remove _mathtext.Node.grow.

It has never been used, and the implementation was in fact buggy (if one
callsshrink many times beyond num_size_levels,.size would continue
to decrease while.width would not; if one then callsgrow,.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 validbox parameter (this requires
classes defined in_mathtext), soget_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 (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

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

@anntzeranntzerforce-pushed themathtextungrow branch 2 times, most recently from9bc7667 toe3236e9CompareJanuary 11, 2022 23:06
@anntzeranntzer changed the titleRemove _mathtext.Node.grow.Cleanup _mathtext internal APIJan 11, 2022
@anntzeranntzerforce-pushed themathtextungrow branch 2 times, most recently fromcad2d3b tobf96d42CompareJanuary 12, 2022 10:41
@oscargus
Copy link
Member

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

@anntzer
Copy link
ContributorAuthor

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

@anntzeranntzerforce-pushed themathtextungrow branch 3 times, most recently from0129d3a toa96df50CompareJanuary 12, 2022 19:10
@oscargus
Copy link
Member

Maybe/probably shouldn't go here, but I've just stumbled upondoc/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 themathtextparts: 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.

@anntzer
Copy link
ContributorAuthor

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

missing-references.json gets rautoegenerated with by building the docs withmake O=-Dmissing_references_write_json=1 html (aftermake 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

@anntzer
Copy link
ContributorAuthor

rebased.

@anntzer
Copy link
ContributorAuthor

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

@oscargus
Copy link
Member

I think thatSubSuperCluster is also unused:

classSubSuperCluster(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__([])

@anntzer
Copy link
ContributorAuthor

Good catch, removed.

@oscargus
Copy link
Member

oscargus commentedFeb 17, 2022
edited
Loading

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

It has never been used, and the implementation was in fact buggy (if onecalls `shrink` many times beyond num_size_levels, `.size` would continueto decrease while `.width` would not; if one then calls `grow`, `.size`and `.width` would then both increase, but get out of sync with oneanother).  I can't find a reference to grow() in the TeXbook or "TeX theprogram" either(?)
None of the MathtextBackends actually accesses the .glyph_nameattribute anymore, and they are the only public API that can interactwith the glyph infos returned by the Fonts classes (and there is nopublic API to register new MathtextBackends).
Backends that need to track what characters have been used (pdf/ps, forsubsetting purposes) already have their own logic to do so, so don't doit in mathtext as well.Changes in Fonts are only touching private API.Changes in the call signature of MathtextBackendAgg/MathtextBackendPathlook like they are touching public API, but it is in fact impossible foran external user to construct a valid `box` parameter (this requiresclasses defined in `_mathtext`), so `get_results` is effectively privateAPI from a caller PoV.  It is also impossible to register newMathtextBackends without touching private API.Thus, the only difference is in the tuple returned byMathtextBackendAgg, which now has one fewer element.  Because the returnvalue is a tuple which is unpacked at the call site, it is not possibleto emit a proper deprecation warning (one would need to know whether theresult is being unpacked to 6 or 7 values).  Callers can work aroundthis by e.g. unpacking the last item into `*_`.  Note that because thisis only for MathtextBackendAgg (i.e., a raster backend), it is unlikelythat callers actually care about the used_characters info at all.
We can just inline the calls directly to the backend itself at the soleuse site (in _parse_cached).  The width, height, and depth attributes onFonts are never used either.
It doesn't need to be a nested class.
@QuLogicQuLogic merged commitb03479a intomatplotlib:mainFeb 17, 2022
@QuLogicQuLogic added this to thev3.6.0 milestoneFeb 17, 2022
@anntzeranntzer deleted the mathtextungrow branchFebruary 17, 2022 22:27
QuLogic added a commit to QuLogic/matplotlib that referenced this pull requestAug 3, 2023
This is never called, is on an internal class, and the attribute itaccesses doesn't exist as it was removed inmatplotlib#22204.
@QuLogicQuLogic mentioned this pull requestAug 3, 2023
3 tasks
QuLogic added a commit to QuLogic/matplotlib that referenced this pull requestAug 3, 2023
This is never called, is on an internal class, and the attribute itaccesses doesn't exist as it was removed inmatplotlib#22204.
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic approved these changes

@oscargusoscargusoscargus approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.

3 participants
@anntzer@oscargus@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp