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 — Properly use thin space after math text operator by mpetroff · Pull Request #17890 · matplotlib/matplotlib

Conversation

@mpetroff

@mpetroff mpetroff commented Jul 11, 2020

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. This pull request makes said fix and should fix issue #17852.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • [Not applicable] New features are documented, with examples if plot related
  • [Not applicable] Documentation is sphinx and numpydoc compliant
  • [Not applicable] Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • [Not applicable] Documented in doc/api/next_api_changes/* if API changed in a backward-incompatible way

Sorry, something went wrong.

Uh oh!

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

All reactions
@mpetroff
Properly use thin space after math text operator.
42e9b97
A thin space should follow a math operator except when it is followed by
something like a parenthesis or a bracket. This should fix issue #17852.
@tacaswell tacaswell added this to the v3.4.0 milestone Jul 11, 2020
@QuLogic
QuLogic requested a review from anntzer July 11, 2020 22:04
QuLogic
QuLogic approved these changes Jul 11, 2020
View reviewed changes

@QuLogic QuLogic left a comment

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.

Seems reasonable.

Sorry, something went wrong.

Uh oh!

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

All reactions
tacaswell
tacaswell approved these changes Jul 11, 2020
View reviewed changes

@tacaswell tacaswell left a comment

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.

Thanks, this looks good to me!

Sorry, something went wrong.

Uh oh!

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

All reactions
@tacaswell

tacaswell commented Jul 11, 2020

Copy link
Copy Markdown
Member

Lets give @anntzer a few days and then merge this.

All reactions

Sorry, something went wrong.

Uh oh!

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

@anntzer

anntzer commented Jul 11, 2020

Copy link
Copy Markdown
Contributor

I didn't look super carefully (may be nice to figure out what exactly is TeX's behavior) but I guess this inserts a space in \sin^n x or in \sin\sqrt x where we may not want one?

All reactions

Sorry, something went wrong.

Uh oh!

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

@anntzer anntzer added the topic: text/mathtext label Jul 11, 2020
@mpetroff
Don't add space before subscript / superscript.
bcc2748
@mpetroff

mpetroff commented Jul 12, 2020

Copy link
Copy Markdown
Contributor Author

@anntzer You're correct; I forgot about those cases. The space before the subscript / superscript can easily be handled by also excluding _ and ^, which is a change I just made.

According to p. 170 of the TeXbook, space is always inserted after an operator except when followed by opening / closing atoms (parentheses, etc.) and punctuation. This means that space should be inserted before \sqrt. It also means that the thin space should actually be included when there's a subscript / superscript, but it should be included after the subscript / superscript. I verified both of these with TeX.

Screenshot from 2020-07-11 21-35-22

However, I'm not sure how to put the thin space after the subscript / superscript, since I haven't quite figured out how that code works.

All reactions

Sorry, something went wrong.

Uh oh!

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

@anntzer

anntzer commented Jul 12, 2020

Copy link
Copy Markdown
Contributor
  1. I'm very much just armchair-quarterbacking here, but perhaps if the next token is sub/superscript then set a flag saying "after this sub/super (possibly both sub and super, if both are present), check whether a thin space should be added"?
  2. I guess the same logic (with or without sub/super handling) is also needed for operatorname (just below)? (Should function just be implemented in terms of operatorname, i.e. replace the whole body of function() by hlist = self.operatorname(...); hlist.function_name = toks[0]; return hlist, and just put the thin space logic only in operatorname?)
  3. Regardless, this is clearly an improvement, and does not preclude implementing sub/super handling later, but my guess is that at least we could try handling operatorname in this PR (i.e. I'm not insisting on 1., but perhaps on 2.).
All reactions

Sorry, something went wrong.

Uh oh!

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

@mpetroff
Extend operator thin space insertion to \operatorname{}.
71adb18
@mpetroff

mpetroff commented Jul 12, 2020

Copy link
Copy Markdown
Contributor Author

I extended the improvement to include operatorname. However, the tests are going to fail now because the old rendering of $\operatorname{cos} x$ was incorrect.

All reactions

Sorry, something went wrong.

Uh oh!

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

@mpetroff
Don't change behavior for Kerns.
f5907f3
@anntzer

anntzer commented Jul 12, 2020 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Contributor

I guess it would be reasonable to just get rid of the single failing test as it's behavior is effectively being tested by the check_figures_equal test.

All reactions

Sorry, something went wrong.

Uh oh!

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

@mpetroff

mpetroff commented Jul 12, 2020

Copy link
Copy Markdown
Contributor Author

With regard to subscript / superscript, I can easily determine in the subsuper parsing function that the nucleus is an operator. However, I can't figure out how to determine how many characters make up the full subscript / superscript expression, so I can't check what follows to determine whether or not a thin space should be inserted.

All reactions

Sorry, something went wrong.

Uh oh!

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

@QuLogic
QuLogic self-requested a review July 14, 2020 21:47
anntzer
anntzer approved these changes Jul 15, 2020
View reviewed changes
tacaswell
tacaswell approved these changes Jul 15, 2020
View reviewed changes

@tacaswell tacaswell left a comment

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 still approve, but we need to delete the no longer used files.

Sorry, something went wrong.

Uh oh!

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

All reactions
@mpetroff
Drop failing test, since it is tested elsewhere.
8fd1c62
The reference rendering for `$\operatorname{cos} x$` was incorrect. Since this
is also tested with `test_operator_space`, it can be safely dropped.
@mpetroff

mpetroff commented Jul 15, 2020

Copy link
Copy Markdown
Contributor Author

I updated the commit that dropped the test to also drop the corresponding baseline images.

All reactions

Sorry, something went wrong.

Uh oh!

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

QuLogic
QuLogic reviewed Jul 15, 2020
View reviewed changes
Comment thread lib/matplotlib/mathtext.py Resolved

Uh oh!

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

Comment thread lib/matplotlib/mathtext.py Resolved

Uh oh!

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

Comment thread lib/matplotlib/tests/test_mathtext.py Resolved

Uh oh!

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

@mpetroff
Fix bug with space between operator and sub / superscripts.
da01cf8
Also add tests for it. A thin space should not be inserted in math text
between the operator and subscript / superscript.
@QuLogic
QuLogic merged commit 81a560c into matplotlib:master Jul 16, 2020
@mpetroff
mpetroff deleted the fix-mathtext-operator-spacing branch July 16, 2020 14:08
@jkseppan jkseppan mentioned this pull request Jul 27, 2020
Thin space missing after mathtext operators #17852
Closed
@henrybeUM henrybeUM mentioned this pull request Apr 13, 2022
Fix spacing after mathtext operators with sub/superscripts #22839
Closed
2 tasks done
@anntzer anntzer mentioned this pull request Nov 1, 2025
Fix spacing in r"$\max f$". #30715
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@QuLogic QuLogic QuLogic approved these changes
@tacaswell tacaswell tacaswell approved these changes
@anntzer anntzer anntzer approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

topic: text/mathtext

Projects

None yet

Milestone

v3.4.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

@mpetroff @tacaswell @anntzer @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 01402503 — 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.