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 — Fix spacing after mathtext operators with sub/superscripts by henrybeUM · Pull Request #22839 · matplotlib/matplotlib

Conversation

@henrybeUM

@henrybeUM henrybeUM commented Apr 13, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

PR Summary

A thin space should follow a math operator except when it is followed by something like a parenthesis or a bracket. When the math operator has a sub/super script, a thin space should be inserted after the sub/super script. This pull request improves upon that of previous pull request #17890 and should fix issue #17852

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

  • [N/A] New features are documented, with examples if plot related.
  • [N/A] New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
  • [N/A] 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
henrybeUM and others added 4 commits April 12, 2022 22:28
@henrybeUM
Initial Investigation into the issue and a potential fix
c545354
@henrybeUM
Merge branch 'matplotlib:main' into bugfix-for-issue-17852
99d0801
@henrybeUM
Cleaned up _mathtext.py comments, added relavant testing
a96e530
@henrybeUM
Merge branch 'bugfix-for-issue-17852' of https://github.com/henrybeUM…
c9c5e62 
…/matplotlib into bugfix-for-issue-17852
github-actions[bot]
github-actions Bot reviewed Apr 13, 2022
View reviewed changes

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@tacaswell tacaswell added this to the v3.6.0 milestone Apr 13, 2022
tacaswell
tacaswell reviewed Apr 13, 2022
View reviewed changes
Comment thread lib/matplotlib/_mathtext.py Outdated
}

# To add space to nucleus operators after sub/superscripts
_subsuper_flag = False

@tacaswell tacaswell Apr 13, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I think this should be instance not class state?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@tacaswell

tacaswell commented Apr 13, 2022

Copy link
Copy Markdown
Member

This seems mostly, reasonable to me, but I think that the state needs to be more localized.

The test failures look real and related, but trying to reproduce them locally I get failures reliably if I do

pytest -k test_mathtext_rendering

but then if I re-run just the failures as

pytest -k test_mathtext_rendering --lf

then everything passes.

$ pytest -k 'test_mathtext_rendering and (-29 or -30)' -v
=================================================================================================================================================================================== test session starts ====================================================================================================================================================================================
platform linux -- Python 3.11.0a7+, pytest-7.2.0.dev47+g752a059cc, pluggy-1.0.0 -- /home/tcaswell/.virtualenvs/bleeding/bin/python3
cachedir: .pytest_cache
benchmark: 3.4.1 (defaults: timer=time.perf_counter disable_gc=False min_rounds=5 min_time=0.000005 max_time=1.0 calibration_precision=10 warmup=False warmup_iterations=100000)
rootdir: /home/tcaswell/source/p/matplotlib/matplotlib, configfile: pytest.ini, testpaths: lib
plugins: timeout-2.1.0, rerunfailures-10.2, forked-1.4.0, cov-3.0.0, xdist-2.5.0, anyio-3.5.0, benchmark-3.4.1, instafail-0.4.2, tornasync-0.6.0.post2
collected 8776 items / 8746 deselected / 2 skipped / 30 selected                                                                                                                                                                                                                                                                                                                           

lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[png-mathtext-cm-29] PASSED                                                                                                                                                                                                                                                                                            [  3%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[png-mathtext-cm-30] PASSED                                                                                                                                                                                                                                                                                            [  6%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[png-mathtext-stix-29] PASSED                                                                                                                                                                                                                                                                                          [ 10%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[png-mathtext-stix-30] PASSED                                                                                                                                                                                                                                                                                          [ 13%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[png-mathtext-stixsans-29] PASSED                                                                                                                                                                                                                                                                                      [ 16%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[png-mathtext-stixsans-30] PASSED                                                                                                                                                                                                                                                                                      [ 20%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[png-mathtext-dejavusans-29] PASSED                                                                                                                                                                                                                                                                                    [ 23%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[png-mathtext-dejavusans-30] PASSED                                                                                                                                                                                                                                                                                    [ 26%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[png-mathtext-dejavuserif-29] PASSED                                                                                                                                                                                                                                                                                   [ 30%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[png-mathtext-dejavuserif-30] PASSED                                                                                                                                                                                                                                                                                   [ 33%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[pdf-mathtext-cm-29] PASSED                                                                                                                                                                                                                                                                                            [ 36%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[pdf-mathtext-cm-30] FAILED                                                                                                                                                                                                                                                                                            [ 40%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[pdf-mathtext-stix-29] PASSED                                                                                                                                                                                                                                                                                          [ 43%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[pdf-mathtext-stix-30] FAILED                                                                                                                                                                                                                                                                                          [ 46%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[pdf-mathtext-stixsans-29] PASSED                                                                                                                                                                                                                                                                                      [ 50%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[pdf-mathtext-stixsans-30] FAILED                                                                                                                                                                                                                                                                                      [ 53%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[pdf-mathtext-dejavusans-29] PASSED                                                                                                                                                                                                                                                                                    [ 56%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[pdf-mathtext-dejavusans-30] FAILED                                                                                                                                                                                                                                                                                    [ 60%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[pdf-mathtext-dejavuserif-29] PASSED                                                                                                                                                                                                                                                                                   [ 63%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[pdf-mathtext-dejavuserif-30] FAILED                                                                                                                                                                                                                                                                                   [ 66%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[svg-mathtext-cm-29] PASSED                                                                                                                                                                                                                                                                                            [ 70%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[svg-mathtext-cm-30] FAILED                                                                                                                                                                                                                                                                                            [ 73%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[svg-mathtext-stix-29] PASSED                                                                                                                                                                                                                                                                                          [ 76%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[svg-mathtext-stix-30] FAILED                                                                                                                                                                                                                                                                                          [ 80%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[svg-mathtext-stixsans-29] PASSED                                                                                                                                                                                                                                                                                      [ 83%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[svg-mathtext-stixsans-30] FAILED                                                                                                                                                                                                                                                                                      [ 86%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[svg-mathtext-dejavusans-29] PASSED                                                                                                                                                                                                                                                                                    [ 90%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[svg-mathtext-dejavusans-30] FAILED                                                                                                                                                                                                                                                                                    [ 93%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[svg-mathtext-dejavuserif-29] PASSED                                                                                                                                                                                                                                                                                   [ 96%]
lib/matplotlib/tests/test_mathtext.py::test_mathtext_rendering[svg-mathtext-dejavuserif-30] FAILED

seems to be the minimal amount of tests that will reproduce the failures which strongly suggests the problem is that the new flag is class (and hence process global!) state.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@oscargus oscargus added topic: text/mathtext status: needs workflow approval For PRs from new contributors, from which GitHub blocks workflows by default. labels Apr 13, 2022
@henrybeUM

henrybeUM commented Apr 14, 2022

Copy link
Copy Markdown
Contributor Author

How would you recommend tying it to the instance instead of the class? The ParserState stack gets pushed and popped unpredictably, so keeping track of it there doesn't seem possible.
Any advice is appreciated :)
All reactions

Sorry, something went wrong.

Uh oh!

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

tacaswell commented Apr 14, 2022

Copy link
Copy Markdown
Member

Hmm, sorry I jumped to conclusion too quickly 🐑

Unfortunately I'm not super familiar with this part of the code base so can not say much more than there is state leaking....
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@henrybeUM
updated test images for mathtext_rendering, modified comments and fla…
c99596e 
…g state in mathtext.py
henrybeUM
henrybeUM commented Apr 15, 2022
View reviewed changes

@henrybeUM henrybeUM left a comment

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I dug into the test issue: Fixing the spacing issue made the old baseline images invalid(they aren't spaced) and they were slightly off. Replaced with newly generated baseline images.
I also updated the state for consistency and revised pull request comments. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@oscargus

oscargus commented Apr 15, 2022

Copy link
Copy Markdown
Member

I believe @anntzer is really the person with most in-depth knowledge here (of those currently active).
All reactions

Sorry, something went wrong.

Uh oh!

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

anntzer commented Apr 15, 2022

Copy link
Copy Markdown
Contributor

If I understand correctly the image changes because the new code includes a thin space after the integral (and exponents)? This seems wrong? (i.e. the space should be suppressed because nothing comes after?)
Perhaps we need some kind of "collapsible space" notion (IIRC TeX has this too), though that is probably out of scope for this PR.

Other than that, the PR seems reasonable modulo minor style points; I'll take advantage of this to also push for #21448 :-).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@henrybeUM
Updated style of mathtext contributions
cc3e7aa
@henrybeUM

henrybeUM commented Apr 15, 2022

Copy link
Copy Markdown
Contributor Author

@anntzer That's correct. I believe the current parser has the same issue for operators without sub/super scripts: \sin and \sin^{2} both have unneeded spaces when nothing follows. A collapsible space would definitely fix the problem, but is definitely above my ability. Also, I just updated the style, let me know if it's to your liking!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anntzer
anntzer reviewed Apr 16, 2022
View reviewed changes
Comment thread lib/matplotlib/_mathtext.py
delimiters = self._left_delim | self._ambi_delim | self._right_delim
delimiters |= {'^', '_'}

if (next_char not in delimiters and

@anntzer anntzer Apr 16, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Actually, it looks like avoiding insertion of the spurious space is just adding a check that if (next_char != '' and ...) (i.e. we are not at the end of the math string). Perhaps you can make that change as part of this PR too? This way we would avoid changing the baseline images.

Sorry, something went wrong.

Uh oh!

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

@henrybeUM henrybeUM Apr 17, 2022 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@anntzer, I don't think this actually fixes the issue. I made the change, reverted to the original images locally, and the same failures occur. I believe changing this statement could alter spacing after non sub/super scripted operators. However, not for sub/superscripted ones, since they will be followed by a '_' or '^' by definition.

In essence, we would need to check the next char after the sub/superscripted content, but it's very hard to tell where the sub/super scripted content stops.

Sorry, something went wrong.

Uh oh!

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

@henrybeUM henrybeUM Apr 21, 2022

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@anntzer any thoughts on how you want to move forward?

Sorry, something went wrong.

Uh oh!

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

@anntzer anntzer Apr 21, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Sorry, indeed the above solution is not enough. Perhaps we can just drop that test for now (replace the entry by None, and delete the baseline images): indeed, there's already tests 37, 56, and 75 which test placement of integrals limits, which seems enough.

Sorry, something went wrong.

Uh oh!

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

@henrybeUM henrybeUM Apr 22, 2022

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@anntzer I dropped the test and made a correction to the initial code to reset the flag between queries, It should be good to go. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
henrybeUM added 2 commits April 21, 2022 23:23
@henrybeUM
Deleted test 30, also fixed a bug where subsuper state could leak int…
9b36a78 
…o other expressions
@henrybeUM
Removed two empty lines to pass linting
6b39a68
@henrybeUM

henrybeUM commented Apr 22, 2022

Copy link
Copy Markdown
Contributor Author

@anntzer My apologies for the linting mistake, It passes locally now and is ready to merge.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
anntzer
anntzer reviewed Apr 23, 2022
View reviewed changes
Comment thread lib/matplotlib/_mathtext.py
next_char = next((c for c in s[next_char_loc:] if c != ' '), '')
delimiters = self._left_delim | self._ambi_delim | self._right_delim
delimiters |= {'^', '_'}

@anntzer anntzer Apr 23, 2022

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

undo this blank line?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
anntzer
anntzer approved these changes Apr 23, 2022
View reviewed changes

@anntzer anntzer left a comment

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Minor style point, but otherwise lgtm (could probably also squash the commits, and add a descriptive commit message briefly describing the bug this fixes).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
timhoffm
timhoffm approved these changes Apr 25, 2022
View reviewed changes
Comment thread lib/matplotlib/_mathtext.py
self._math_expression = p.math

# To add space to nucleus operators after sub/superscripts
self._subsuper_flag = False

@timhoffm timhoffm Apr 25, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Slightly clearer naming:

Suggested change
self._subsuper_flag = False
self._in_subscript_or_superscript = False

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@timhoffm timhoffm changed the title Bugfix for issue 17852 Fix spacing after mathtext operators with sub/superscripts Apr 25, 2022
@jklymak

jklymak commented Jun 2, 2022

Copy link
Copy Markdown
Member

Can the two minor points be addressed, and then please move from draft and we can merge?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@jklymak
jklymak marked this pull request as draft June 2, 2022 14:30
@jklymak jklymak added the status: needs revision label Jun 2, 2022
timhoffm added a commit to timhoffm/matplotlib that referenced this pull request Jun 10, 2022
@timhoffm @henrybeUM
Fix spacing after mathtext operators with sub/superscripts
0380a24 
Picks up matplotlib#22839.

Closes matplotlib#22839, matplotlib#17852.

Co-authored-by: henrybeUM <[email protected]>
@timhoffm timhoffm mentioned this pull request Jun 10, 2022
Fix spacing after mathtext operators with sub/superscripts #23243
Merged
timhoffm added a commit to timhoffm/matplotlib that referenced this pull request Jun 12, 2022
@timhoffm @henrybeUM
Fix spacing after mathtext operators with sub/superscripts
ed4a462 
Picks up matplotlib#22839.

Closes matplotlib#22839, matplotlib#17852.

Co-authored-by: henrybeUM <[email protected]>
@oscargus oscargus closed this in #23243 Jun 12, 2022
andrew-fennell pushed a commit to andrew-fennell/matplotlib that referenced this pull request Jun 14, 2022
@timhoffm @henrybeUM @andrew-fennell
Fix spacing after mathtext operators with sub/superscripts
9b1632f 
Picks up matplotlib#22839.

Closes matplotlib#22839, matplotlib#17852.

Co-authored-by: henrybeUM <[email protected]>
jklymak pushed a commit to jklymak/matplotlib that referenced this pull request Jun 24, 2022
@timhoffm @henrybeUM @jklymak
Fix spacing after mathtext operators with sub/superscripts
8daf300 
Picks up matplotlib#22839.

Closes matplotlib#22839, matplotlib#17852.

Co-authored-by: henrybeUM <[email protected]>
imgbot Bot pushed a commit to kkipngenokoech/matplotlib that referenced this pull request Apr 19, 2026
@timhoffm @henrybeUM @ImgBotApp
Fix spacing after mathtext operators with sub/superscripts
5cd0cdb 
Picks up matplotlib#22839.

Closes matplotlib#22839, matplotlib#17852.

Co-authored-by: henrybeUM <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tacaswell tacaswell tacaswell left review comments
@github-actions github-actions[bot] github-actions[bot] left review comments
@anntzer anntzer anntzer approved these changes
@timhoffm timhoffm timhoffm approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

status: needs revision status: needs workflow approval For PRs from new contributors, from which GitHub blocks workflows by default. 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.

6 participants

@henrybeUM @tacaswell @oscargus @anntzer @jklymak @timhoffm
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.