Skip to content

Replace MathtextBackend mechanism. #22506

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
Aug 19, 2022
Merged

Replace MathtextBackend mechanism. #22506

merged 1 commit into from
Aug 19, 2022

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Feb 19, 2022

The MathtextBackend ("MB") mechanism was previously used to let actual
backends customize how they received mathtext results -- either as lists
of glyphs and rectangles (for vector backends: MathtextBackendPath),
or a bitmap (for raster backends: MathtextBackendAgg); in both cases,
metrics are also provided. MBs also controlled font hinting. Note that
the MB mechanism was not publically user-extendable (this would require
touching the private MathTextParser._backend_mapping dict), so third
parties could not meaningfully provide their own backends.

MBs were attached to _mathtext.Fonts objects, which were central to
the "shipping" stage of the parse (ship(), which converts the nested
parse tree created by pyparsing into flat calls to render_glyph and
render_rect_filled). This led to a slightly curious API, where
the old MathtextBackendAgg.get_results() (for example) calls
_mathtext.ship(0, 0, box) and this somehow magically mutates self --
this is because self is indirectly attached to sub-elements of box.

This PR changes the implementation to instead detach output logic
from Fonts (which become restricted to providing glyph metrics and
related info), and makes ship() instead return a simple Output object
(lists of glyphs and rects) which is itself able either to convert to
a VectorParse or a RasterParse -- namedtuples that are backcompatible
with the tuples previously returned by MathTextParser.parse(). (While
technically these are "new" classes in the API, they are simply there to
(slightly) better document the return value of MathtextBackend.parse().)

In summary, this patch

  • removes the non-extensible MB system,
  • detaches output logic from Fonts objects, thus avoiding "action at
    distance" where ship(0, 0, box) would mutate the calling MB,
  • (weakly) documents the return value of MathtextBackend.parse().

Unrelatedly, also deprecate the unused MathTextWarning.

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

@anntzer anntzer added this to the v3.6.0 milestone Feb 19, 2022
@anntzer anntzer force-pushed the umb branch 2 times, most recently from 6f9692a to 57e7898 Compare February 19, 2022 17:30
Copy link
Member

@oscargus oscargus left a comment

Choose a reason for hiding this comment

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

The [source]-link for VectorParseResult and RasterParseResult does not appear to be working (as opposed to get_unicode_index). Not sure, but maybe something like VectorParseResult.__module__ = __name__ should be done? (Only difference I could spot).

The documentation for those are a bit sporadic "alias for field 0" etc, but maybe more a problem that it says that than it is not explicitly documented, if anything. (Feel free to ignore.)

Overall, I think it makes sense, but maybe one can also add a bit more information in the deprecation notice along the lines "MathTextParser.parse() will instead return VectorParseResult or RasterParseResult." so that people seeing the deprecation warning and checking the notes will get some help into what to do instead.

@anntzer
Copy link
Contributor Author

anntzer commented Feb 20, 2022

The [source]-link for VectorParseResult and RasterParseResult does not appear to be working (as opposed to get_unicode_index). Not sure, but maybe something like VectorParseResult.module = name should be done? (Only difference I could spot).

__module__ is already set correctly (via module=... in the namedtuple declaration). We actually just can't link namedtuples at all, see e.g. https://67022-1385122-gh.circle-artifacts.com/0/doc/build/html/api/dviread.html#matplotlib.dviread.PsFont which doesn't work either.

The documentation for those are a bit sporadic "alias for field 0" etc, but maybe more a problem that it says that than it is not explicitly documented, if anything. (Feel free to ignore.)

Better docs could be nice, but previously the return type (well, the fields of the returned tuple) of MathTextParser.parse were not documented at all, so I think this is already an improvement; further documentation can be done later.

Overall, I think it makes sense, but maybe one can also add a bit more information in the deprecation notice along the lines "MathTextParser.parse() will instead return VectorParseResult or RasterParseResult." so that people seeing the deprecation warning and checking the notes will get some help into what to do instead.

There is no deprecation; the Parse results (now renamed to VectorParse/RasterParse) are fully backcompatible with the old ones (they inherit from tuple).

@oscargus
Copy link
Member

There is no deprecation; the Parse results (now renamed to VectorParse/RasterParse) are fully backcompatible with the old ones (they inherit from tuple).

But there is?

``MathtextBackend``, ``MathtextBackendAgg``, ``MathtextBackendPath``, ``MathTextWarning``
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
... are deprecated with no replacement.

Not sure how they worked out, but they were public in a public module, so in case someone use them directly (for unknown reasons), there is currently no information about how to replace them. If I understand correctly, it only matters if creating them directly or checking the type of the return object? So it is unlikely that anyone will really need to change anything in their code?

@anntzer
Copy link
Contributor Author

anntzer commented Feb 21, 2022

While users can create MathtextBackend objects directly, there is no public API whatsoever that accepts or returns them (they are constructed internally by MathTextParser.parse, not taken as arguments or returned), nor is it possible for the end users to call their main API method (get_results), because get_results takes a Box object, which is an instance of a private class. Even calling render_glyph would be quite difficult, as the third parameter (info) is a SimpleNamespace which needs specific (undocumented) fields. So they can't do anything useful with the instance they have, and checking for that type isn't useful either.

Altogether, I believe this means that just noting that the classes are gone is sufficient and more info would actually make the changelog more confusing for readers, not less; moreover, there is simply no replacement for these APIs anyways.

@anntzer
Copy link
Contributor Author

anntzer commented Jul 5, 2022

rebased.

The MathtextBackend ("MB") mechanism was previously used to let actual
backends customize how they received mathtext results -- either as lists
of glyphs and rectangles (for vector backends: MathtextBackendPath),
or a bitmap (for raster backends: MathtextBackendAgg); in both cases,
metrics are also provided.  MBs also controlled font hinting.  Note that
the MB mechanism was not publically user-extendable (this would require
touching the private MathTextParser._backend_mapping dict), so third
parties could not meaningfully provide their own backends.

MBs were attached to _mathtext.Fonts objects, which were central to
the "shipping" stage of the parse (ship(), which converts the nested
parse tree created by pyparsing into flat calls to render_glyph and
render_rect_filled).  This led to a slightly curious API, where
the old MathtextBackendAgg.get_results() (for example) calls
`_mathtext.ship(0, 0, box)` and this somehow magically mutates self --
this is because self is indirectly attached to sub-elements of box.

This PR changes the implementation to instead detach output logic
from Fonts (which become restricted to providing glyph metrics and
related info), and makes ship() instead return a simple Output object
(lists of glyphs and rects) which is itself able either to convert to
a VectorParse or a RasterParse -- namedtuples that are backcompatible
with the tuples previously returned by MathTextParser.parse().  (While
technically these are "new" classes in the API, they are simply there to
(slightly) better document the return value of MathtextBackend.parse().)

In summary, this patch
- removes the non-extensible MB system,
- detaches output logic from Fonts objects, thus avoiding "action at
  distance" where `ship(0, 0, box)` would mutate the calling MB,
- (weakly) documents the return value of MathtextBackend.parse().

Unrelatedly, also deprecate the unused MathTextWarning.
@QuLogic QuLogic merged commit b54b335 into matplotlib:main Aug 19, 2022
@anntzer anntzer deleted the umb branch August 19, 2022 20:04
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.

4 participants