Fix spacing after mathtext operators with sub/superscripts #22839
Conversation
…/matplotlib into bugfix-for-issue-17852
There was a problem hiding this comment.
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.
lib/matplotlib/_mathtext.py
Outdated
| @@ -2104,7 +2104,8 @@ def unknown_symbol(self, s, loc, toks): | |||
| r'overleftarrow': r'\leftarrow', | |||
| r'mathring': r'\circ', | |||
| } | |||
|
|
|||
| # To add space to nucleus operators after sub/superscripts | |||
| _subsuper_flag = False | |||
There was a problem hiding this comment.
I think this should be instance not class state?
|
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 but then if I re-run just the failures as then everything passes. 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. |
|
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. |
|
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.... |
…g state in mathtext.py
There was a problem hiding this comment.
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. :)
|
I believe @anntzer is really the person with most in-depth knowledge here (of those currently active). |
|
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?) Other than that, the PR seems reasonable modulo minor style points; I'll take advantage of this to also push for #21448 :-). |
|
@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! |
| @@ -2159,10 +2162,17 @@ def operatorname(self, s, loc, toks): | |||
| next_char = next((c for c in s[next_char_loc:] if c != ' '), '') | |||
| delimiters = self._left_delim | self._ambi_delim | self._right_delim | |||
| delimiters |= {'^', '_'} | |||
|
|
|||
| if (next_char not in delimiters and | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@anntzer any thoughts on how you want to move forward?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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. :)
|
@anntzer My apologies for the linting mistake, It passes locally now and is ready to merge. |
| @@ -2159,11 +2164,19 @@ def operatorname(self, s, loc, toks): | |||
| next_char = next((c for c in s[next_char_loc:] if c != ' '), '') | |||
| delimiters = self._left_delim | self._ambi_delim | self._right_delim | |||
| delimiters |= {'^', '_'} | |||
|
|
|||
| @@ -1947,6 +1947,9 @@ def __init__(self): | |||
| self._expression = p.main | |||
| self._math_expression = p.math | |||
|
|
|||
| # To add space to nucleus operators after sub/superscripts | |||
| self._subsuper_flag = False | |||
There was a problem hiding this comment.
Slightly clearer naming:
| self._subsuper_flag = False | |
| self._in_subscript_or_superscript = False |
|
Can the two minor points be addressed, and then please move from draft and we can merge? |
Picks up matplotlib#22839. Closes matplotlib#22839, matplotlib#17852. Co-authored-by: henrybeUM <98666765+henrybeUM@users.noreply.github.com>
Picks up matplotlib#22839. Closes matplotlib#22839, matplotlib#17852. Co-authored-by: henrybeUM <98666765+henrybeUM@users.noreply.github.com>
Picks up matplotlib#22839. Closes matplotlib#22839, matplotlib#17852. Co-authored-by: henrybeUM <98666765+henrybeUM@users.noreply.github.com>
Picks up matplotlib#22839. Closes matplotlib#22839, matplotlib#17852. Co-authored-by: henrybeUM <98666765+henrybeUM@users.noreply.github.com>
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
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).