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 — Use named groups in mathtext parser. by anntzer · Pull Request #21448 · matplotlib/matplotlib

Conversation

@anntzer

@anntzer anntzer commented Oct 24, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

This makes the definitions much easier to read, by removing the need to
use Group() and Suppress(): previously, these would be used to nest the
parsed tokens and remove the unnecessary ones (e.g, braces), but such
tokens can also easily be ignored by giving names to the tokens we use
(instead of accessing them by position). (See e.g. the implementation
of subsuper, which is also somewhat simplified.)

Also remove a few other pyparsing combinators that are not
needed: Combine (just concatenate the strings ourselves), FollowedBy
(use lookahead regexes), Literal (use plain strings, in particular
braces which are very common).

Also remove right_delim_safe, as self._right_delim already includes the
backslash-escaped closing brace rather than the unescaped one
(right_delim_safe dates back to when it didn't).

The error message for a^2_2^2 now changed to "double superscript" as
we now simply check for subscripts and superscripts one at a time, and
fail when we see either two subscript or two superscripts, emitting the
corresponding error.

(I played a bit with the parser to try and see what's wrong with pyparsing 3
and got there; unfortunately this patch doesn't fix the incompatibility :-()

Edit: This seems to fix support with pyparsing 3. It's not totally clear why, though...
A much more minimal fix would be

diff --git i/lib/matplotlib/_mathtext.py w/lib/matplotlib/_mathtext.py
index 73e6163944..fbf58b5df1 100644
--- i/lib/matplotlib/_mathtext.py
+++ w/lib/matplotlib/_mathtext.py
@@ -2046,7 +2046,7 @@ class Parser:
         p.accentprefixed <<= Suppress(p.bslash) + oneOf(self._accentprefixed)
         p.symbol_name   <<= (
             Combine(p.bslash + oneOf(list(tex2uni)))
-            + FollowedBy(Regex("[^A-Za-z]").leaveWhitespace() | StringEnd())
+            + Suppress(Regex("(?=[^A-Za-z]|$)").leaveWhitespace())
         )
         p.symbol        <<= (p.single_symbol | p.symbol_name).leaveWhitespace()

I still don't know whether that's a bug or an intended API change on pyparsing's side.

PR Summary

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
  • 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).

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 Oct 24, 2021
@anntzer
anntzer force-pushed the mtp branch 2 times, most recently from 3855e16 to b970df6 Compare October 24, 2021 17:17
timhoffm
timhoffm approved these changes Oct 24, 2021
View reviewed changes
Comment thread lib/matplotlib/_mathtext.py Resolved

Uh oh!

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

@anntzer anntzer removed the Maintenance label Oct 24, 2021
@anntzer
anntzer force-pushed the mtp branch from b970df6 to 33a5781 Compare October 24, 2021 20:04
@anntzer anntzer added this to the v3.5.0 milestone Oct 24, 2021
@anntzer

anntzer commented Oct 24, 2021

Copy link
Copy Markdown
Contributor Author

I'll milestone this as RC for mpl3.5, although for that release most likely we could (should) just extract the minimal patch (above, in the Edit: ...) to specifically fix compat with pyparsing 3.

All reactions

Sorry, something went wrong.

Uh oh!

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

@anntzer anntzer added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Oct 24, 2021
@anntzer
anntzer force-pushed the mtp branch from 33a5781 to b0cccbb Compare October 24, 2021 20:22
timhoffm added a commit to timhoffm/matplotlib that referenced this pull request Oct 24, 2021
@timhoffm
Fix error with pyparsing 3
4dfac9b
- Code suggestion taken from matplotlib#21448
- Removed all pyparsing pinning. If this runs through CI, the fix
  is proven to be working.
@timhoffm timhoffm mentioned this pull request Oct 24, 2021
Fix error with pyparsing 3 for 3.5.x #21454
Merged
@timhoffm timhoffm modified the milestones: v3.5.0, v3.6.0 Oct 24, 2021
@timhoffm timhoffm removed the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Oct 24, 2021
@timhoffm

timhoffm commented Oct 24, 2021

Copy link
Copy Markdown
Member

We should only backport the minimal fix as suggested. I've created #21454 for this. So we can remilestone to 3.6 and remove the "release critical" tag here.

All reactions

Sorry, something went wrong.

Uh oh!

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

timhoffm added a commit to timhoffm/matplotlib that referenced this pull request Oct 24, 2021
@timhoffm
Fix error with pyparsing 3
ba51894
- Code suggestion taken from matplotlib#21448
- Removed all pyparsing pinning. If this runs through CI, the fix
  is proven to be working.
@anntzer

anntzer commented Oct 24, 2021

Copy link
Copy Markdown
Contributor Author

I know, I was just letting someone else do the work :-)

All reactions

Sorry, something went wrong.

Uh oh!

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

timhoffm added a commit to timhoffm/matplotlib that referenced this pull request Oct 24, 2021
@timhoffm
Fix error with pyparsing 3
90c7afd
- Code suggestion taken from matplotlib#21448
- Removed all pyparsing pinning. If this runs through CI, the fix
  is proven to be working.
@timhoffm

timhoffm commented Oct 24, 2021

Copy link
Copy Markdown
Member

I suggest you remove the pyparsing pinning in this PR as well to make sure it solves the issue.

All reactions

Sorry, something went wrong.

Uh oh!

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

@anntzer

anntzer commented Oct 24, 2021

Copy link
Copy Markdown
Contributor Author

This is already done in the second commit.

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

@anntzer
anntzer force-pushed the mtp branch 3 times, most recently from fb6fe24 to bd30b7b Compare October 25, 2021 17:52
@anntzer anntzer mentioned this pull request Oct 26, 2021
[MNT]: mathtext.MathTextParser is slow #20821
Open
@anntzer anntzer added the Performance label Oct 26, 2021
@anntzer

anntzer commented Oct 26, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

Seems also a bit faster, see #20821 (comment).

Edit: self-contained perf test:

MPLBACKEND=agg python -c 'import time; from pylab import *; from matplotlib.tests.test_mathtext import math_tests; fig = figure(figsize=(3, 10)); fig.text(0, 0, "\n".join(filter(None, math_tests)), size=6); start = time.perf_counter(); [fig.canvas.draw() for _ in range(10)]; print((time.perf_counter() - start) / 10)'

before: ~1.02s; after: ~0.81s.

All reactions

Sorry, something went wrong.

Uh oh!

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

@anntzer
anntzer force-pushed the mtp branch from bd30b7b to 1dd1490 Compare October 29, 2021 18:28
@anntzer anntzer mentioned this pull request Oct 29, 2021
Refix for pyparsing compat. #21501
Merged
7 tasks
@anntzer
anntzer force-pushed the mtp branch from 1dd1490 to 720ac9a Compare October 30, 2021 11:30
@anntzer anntzer mentioned this pull request Nov 14, 2021
BLD: limit support of pyparsing to <3 #21606
Closed
6 tasks
@anntzer
anntzer force-pushed the mtp branch from 720ac9a to bfe1a94 Compare November 14, 2021 20:57
@QuLogic QuLogic added the status: needs rebase label Dec 16, 2021
@anntzer
anntzer force-pushed the mtp branch from bfe1a94 to af5e927 Compare January 13, 2022 23:25
@anntzer
Use named groups in mathtext parser.
5392fc1
This makes the definitions much easier to read, by removing the need to
use Group() and Suppress(): previously, these would be used to nest the
parsed tokens and remove the unnecessary ones (e.g, braces), but such
tokens can also easily be ignored by giving names to the tokens we use
(instead of accessing them by position).  (See e.g. the implementation
of subsuper, which is also somewhat simplified.)

Also remove a few other pyparsing combinators that are not
needed: Combine (just concatenate the strings ourselves), FollowedBy
(use lookahead regexes), Literal (use plain strings, in particular
braces which are very common).

Also remove right_delim_safe, as self._right_delim already includes the
backslash-escaped closing brace rather than the unescaped one
(right_delim_safe dates back to when it didn't).

The error message for `a^2_2^2` now changed to "double superscript" as
we now simply check for subscripts and superscripts one at a time, and
fail when we see either two subscript or two superscripts, emitting the
corresponding error.
@anntzer
anntzer force-pushed the mtp branch from af5e927 to 5392fc1 Compare January 13, 2022 23:55
@anntzer anntzer removed the status: needs rebase label Jan 17, 2022
@anntzer anntzer mentioned this pull request Apr 15, 2022
Fix spacing after mathtext operators with sub/superscripts #22839
Closed
2 tasks done
oscargus
oscargus approved these changes Apr 21, 2022
View reviewed changes
@oscargus
oscargus merged commit c6d4a29 into matplotlib:main Apr 21, 2022
@anntzer
anntzer deleted the mtp branch May 1, 2022 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@timhoffm timhoffm timhoffm 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

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

4 participants

@anntzer @timhoffm @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.