Skip to content

BUG: Dot should not be spaced when used as a decimal separator #5301

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 1 commit into from
Oct 29, 2015

Conversation

zblz
Copy link
Member

@zblz zblz commented Oct 22, 2015

I removed the spacing after the dot when the previous and next characters are digits, i.e., when it is being used as a decimal separator.

I also sneaked in a change a few lines above so that binary operators will not be spaced after a parens (so that $(-2)$ is not spaced as if it was an operation), see #5020.

I added a test for this dot spacing issue, but given the high threshold for these tests I am not sure whether it would fail even if the spacing is as before this PR. At least it runs through the new code.

attn @mdboom

@@ -2504,7 +2504,8 @@ def symbol(self, s, loc, toks):
break
# Binary operators at start of string should not be spaced
if (c in self._binary_operators and
(len(s[:loc].split()) == 0 or prev_char == '{')):
(len(s[:loc].split()) == 0 or prev_char == '{' or
Copy link
Member

Choose a reason for hiding this comment

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

This should support everything in _left_delim. Could probably just be prev_char in self._left_delim.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, _left_delim is definitely more general than (. However, { is not in _left_delim (only \{), so I'll do that comparison separately.

@mdboom mdboom added this to the next major release (2.0) milestone Oct 22, 2015
@mdboom
Copy link
Member

mdboom commented Oct 22, 2015

Can you try turning down the tolerance and seeing how many other tests fail and in what way? It would be good to know if there are any regressions.

        @image_comparison(baseline_images=[filename], extensions=extensions,
                          tol=32)  # <-- Change here

@zblz
Copy link
Member Author

zblz commented Oct 22, 2015

@tacaswell did an overview of the mathtext tests with lower tolerance in #4887, but ended not using it because of lack of time in checking the images. Quite a lot of them fail with tol=15, so we should probably take a look at them at some point.

@mdboom
Copy link
Member

mdboom commented Oct 22, 2015

I didn't mean to lower the tolerance permanently, necessarily, just lower it enough to see if this change causes any regressions. I can probably find some time to sort through them soon.

@zblz
Copy link
Member Author

zblz commented Oct 23, 2015

I did a run with tol=0 following the method of @tacaswell in #4887 using v1.5.0rc3 and I get the following mathtext tests failing:

  • 00: baseline image is wrong, symbols are not properly spaced.
  • 03: dot decimal spacing is right in baseline and not in master, should be fixed by this PR
  • 07: baseline image is wrong, symbols are not properly spaced.
  • 09: spacing is wildly different, I imagine the baseline is wrong since you recently did a PR on mathtext spacing to match TeX?
  • 10: files are almost identical (RMS 0.008 in stix svg).
  • 18: baseline is wrong (symbol spacing)
  • 19: idem
  • 22: idem
  • 25: idem
  • 26: accents are shifted, the baseline looks worse to me
  • 28: baseline is wrong (symbol spacing)
  • 33: idem
  • 35: idem
  • 38: idem
  • 39: accent location, same as 26
  • 41: stixsans svg fails with RMS 0.006
  • 44: baseline is wrong (symbol spacing)
  • 45: idem
  • 47: idem
  • 48: idem
  • 52: idem
  • 60: idem
  • 63: idem
  • 64: another spacing issue with \, as in 09. baseline looks wrong to me: 2 \, a should render to a very small space, and the baseline spacing is pretty big.
  • 65: spacing difference after ! (baseline has no space, master has it as per the _punctuation_symbols spacing)
  • 67: baseline is wrong: symbol spacing and spacing after comma
  • 69: accent position, as in 26, 39
  • 71: different spacing between symbols
  • 72: idem

There are a lot of them related to recent PRs in which we did not replace the baseline images because the tests where not failing (#4887 for symbol spacing, #4403 for spacing symbols such as \,). If we replace these and the accent ones we could drop the tolerance significantly to be able to catch spacing issues in future changes.

@zblz
Copy link
Member Author

zblz commented Oct 23, 2015

Regarding this PR, I don't think there are any relevant regressions. I believe it only affects test 03 where the baseline image is closer to this PR than the master render. Most of the other uses of decimal separators in the tests suite (tick labels mostly) are not in a mathtext enviroment, so this PR does not affect those.

@zblz
Copy link
Member Author

zblz commented Oct 27, 2015

I have removed the new test because test 3 should have caught this problem if the tolerance was lower. When #5307 is merged, that test will be enough.

@mdboom
Copy link
Member

mdboom commented Oct 27, 2015

Just a note to myself and others: To avoid recreation of too many baseline images, this should be merged before #5307, the test images regenerated in #5307, and then merge #5307.

@mdboom mdboom mentioned this pull request Oct 27, 2015
mdboom added a commit that referenced this pull request Oct 29, 2015
BUG: Dot should not be spaced when used as a decimal separator
@mdboom mdboom merged commit a9ee326 into matplotlib:master Oct 29, 2015
@zblz zblz deleted the fix-dot-space branch October 29, 2015 17:11
@mdboom
Copy link
Member

mdboom commented Oct 30, 2015

Backported to v2.0.x as 60b60fb

mdboom added a commit that referenced this pull request Oct 30, 2015
BUG: Dot should not be spaced when used as a decimal separator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants