Skip to content

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.

Sign up for GitHub

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

Closed
henrybeUM wants to merge 8 commits into from
Closed

Fix spacing after mathtext operators with sub/superscripts #22839

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c545354
Initial Investigation into the issue and a potential fix
henrybeUM Apr 13, 2022
99d0801
Merge branch 'matplotlib:main' into bugfix-for-issue-17852
henrybeUM Apr 13, 2022
a96e530
Cleaned up _mathtext.py comments, added relavant testing
henrybeUM Apr 13, 2022
c9c5e62
Merge branch 'bugfix-for-issue-17852' of https://github.com/henrybeUM…
henrybeUM Apr 13, 2022
c99596e
updated test images for mathtext_rendering, modified comments and fla…
henrybeUM Apr 15, 2022
cc3e7aa
Updated style of mathtext contributions
henrybeUM Apr 15, 2022
9b36a78
Deleted test 30, also fixed a bug where subsuper state could leak int…
henrybeUM Apr 22, 2022
6b39a68
Removed two empty lines to pass linting
henrybeUM Apr 22, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 17 additions & 2 deletions lib/matplotlib/_mathtext.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@tacaswell tacaswell Apr 13, 2022

Copy link
Copy Markdown
Member

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?

_wide_accents = set(r"widehat widetilde widebar".split())

# make a lambda and call it to get the namespace right
Expand Down Expand Up @@ -2159,10 +2160,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 |= {'^', '_'}

@anntzer anntzer Apr 23, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

undo this blank line?

if (next_char not in delimiters and

@anntzer anntzer Apr 16, 2022

Copy link
Copy Markdown
Contributor

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.

@henrybeUM henrybeUM Apr 17, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.

@henrybeUM henrybeUM Apr 21, 2022

Copy link
Copy Markdown
Contributor Author

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?

@anntzer anntzer Apr 21, 2022

Copy link
Copy Markdown
Contributor

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.

@henrybeUM henrybeUM Apr 22, 2022

Copy link
Copy Markdown
Contributor Author

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

toks[0] not in self._overunder_functions):
# Add thin space except when followed by parenthesis, bracket, etc.
hlist_list += [self._make_space(self._space_widths[r'\,'])]

# if followed by a super/subscript, set flag to true
# This flag tells subsuper to add space after this operator
if(next_char in {'^', '_'}):
self._subsuper_flag = True

self.pop_state()
return Hlist(hlist_list)

Expand Down Expand Up @@ -2394,8 +2402,15 @@ def subsuper(self, s, loc, toks):

if not self.is_dropsub(last_char):
x.width += constants.script_space * xHeight
result = Hlist([nucleus, x])

# Do we need to add a space after the nucleus
# Checks the flag set by operatorname
result = [nucleus, x]
if(self._subsuper_flag):
result += [self._make_space(self._space_widths[r'\,'])]
self._subsuper_flag = False

result = Hlist(result)
return [result]

def _genfrac(self, ldelim, rdelim, rule, style, num, den):
Expand Down
4 changes: 4 additions & 0 deletions lib/matplotlib/tests/test_mathtext.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,9 @@ def test_operator_space(fig_test, fig_ref):
fig_test.text(0.1, 0.6, r"$\operatorname{op}[6]$")
fig_test.text(0.1, 0.7, r"$\cos^2$")
fig_test.text(0.1, 0.8, r"$\log_2$")
# Check spacing after Sub/Super scripts
# Github issue 17852
fig_test.text(0.1, 0.7, r"$\sin^2 \cos$")

fig_ref.text(0.1, 0.1, r"$\mathrm{log\,}6$")
fig_ref.text(0.1, 0.2, r"$\mathrm{log}(6)$")
Expand All @@ -342,6 +345,7 @@ def test_operator_space(fig_test, fig_ref):
fig_ref.text(0.1, 0.6, r"$\mathrm{op}[6]$")
fig_ref.text(0.1, 0.7, r"$\mathrm{cos}^2$")
fig_ref.text(0.1, 0.8, r"$\mathrm{log}_2$")
fig_ref.text(0.1, 0.7, r"$\mathrm{sin}^2 \mathrm{\,cos}$")


def test_mathtext_fallback_valid():
Expand Down