Skip to content

Properly use thin space after math text operator #17890

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 6 commits into from
Jul 16, 2020
Merged

Properly use thin space after math text operator #17890

merged 6 commits into from
Jul 16, 2020

Conversation

mpetroff
Copy link
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

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
Copy link
Member

@QuLogic QuLogic left a comment

Choose a reason for hiding this comment

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

Seems reasonable.

Copy link
Member

@tacaswell tacaswell left a 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!

@tacaswell
Copy link
Member

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

@anntzer
Copy link
Contributor

anntzer commented Jul 11, 2020

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?

@mpetroff
Copy link
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.

@anntzer
Copy link
Contributor

anntzer commented Jul 12, 2020

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

@mpetroff
Copy link
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.

@anntzer
Copy link
Contributor

anntzer commented Jul 12, 2020

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.

@mpetroff
Copy link
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.

@QuLogic QuLogic self-requested a review July 14, 2020 21:47
Copy link
Member

@tacaswell tacaswell left a 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.

The reference rendering for `$\operatorname{cos} x$` was incorrect. Since this
is also tested with `test_operator_space`, it can be safely dropped.
@mpetroff
Copy link
Contributor Author

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

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