Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Fix spacing after mathtext operators with sub/superscripts#22839
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
…/matplotlib into bugfix-for-issue-17852
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 uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode 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.
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?
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.
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. :)
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.
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 thatif (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.
henrybeUMApr 17, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
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?
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.
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. :)
@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 |= {'^', '_'} | |||
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?
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).
| @@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 upmatplotlib#22839.Closesmatplotlib#22839,matplotlib#17852.Co-authored-by: henrybeUM <98666765+henrybeUM@users.noreply.github.com>
Picks upmatplotlib#22839.Closesmatplotlib#22839,matplotlib#17852.Co-authored-by: henrybeUM <98666765+henrybeUM@users.noreply.github.com>
Picks upmatplotlib#22839.Closesmatplotlib#22839,matplotlib#17852.Co-authored-by: henrybeUM <98666765+henrybeUM@users.noreply.github.com>
Picks upmatplotlib#22839.Closesmatplotlib#22839,matplotlib#17852.Co-authored-by: henrybeUM <98666765+henrybeUM@users.noreply.github.com>
Uh oh!
There was an error while loading.Please reload this page.
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).