-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Fix over/under mathtext symbols #19314
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
Conversation
I am confused as to why the mathfont tests (which do not involve subsuper) have baseline changes? (perhaps I'm missing something obvious...) |
It looks like maybe you updated |
1f5fb57
to
78d37a9
Compare
Yes, I can see that 18 needs updates in some places but not all of them. Try using |
That's right, and it was deliberate - for 18 only these fail (6 out of 15 variants): 18-pdf-mathtext-cm
18-pdf-mathtext-dejavuserif
18-pdf-mathtext-stix
18-svg-mathtext-stix
18-svg-mathtext-stixsans For IDs like 22 or 67, (15 out of 15 fail): 67-pdf-mathtext-cm
67-pdf-mathtext-dejavusans
67-pdf-mathtext-dejavuserif
67-pdf-mathtext-stix
67-pdf-mathtext-stixsans
67-png-mathtext-cm
67-png-mathtext-dejavusans
67-png-mathtext-dejavuserif
67-png-mathtext-stix
67-png-mathtext-stixsans
67-svg-mathtext-cm
67-svg-mathtext-dejavusans
67-svg-mathtext-dejavuserif
67-svg-mathtext-stix
67-svg-mathtext-stixsans I believe we're comparing images with a tolerance? We could change just the failing images and forget about the IDs, it should not raise an error. Should I update the PR? |
No, the default tolerance is 0, and I don't see the mathtext tests setting anything different.
Yes, better to reduce test image churn when there is no failure necessitating an update. |
78d37a9
to
d8466c1
Compare
With the latest push only 67 failing The failing mathtext images:to_rewrite = [
"mathtext_cm_18.pdf",
"mathtext_dejavuserif_18.pdf",
"mathtext_stix_18.pdf",
"mathtext_stix_18.svg",
"mathtext_stixsans_18.svg",
"mathtext_cm_22.pdf",
"mathtext_dejavusans_22.pdf",
"mathtext_dejavuserif_22.pdf",
"mathtext_stix_22.pdf",
"mathtext_stixsans_22.pdf",
"mathtext_cm_22.png",
"mathtext_dejavusans_22.png",
"mathtext_dejavuserif_22.png",
"mathtext_stix_22.png",
"mathtext_stixsans_22.png",
"mathtext_cm_22.svg",
"mathtext_dejavusans_22.svg",
"mathtext_dejavuserif_22.svg",
"mathtext_stix_22.svg",
"mathtext_stixsans_22.svg",
"mathtext_dejavuserif_29.pdf",
"mathtext_dejavusans_29.svg",
"mathtext_cm_34.pdf",
"mathtext_dejavusans_34.pdf",
"mathtext_dejavuserif_34.pdf",
"mathtext_stix_34.pdf",
"mathtext_stixsans_34.pdf",
"mathtext_cm_34.png",
"mathtext_dejavusans_34.png",
"mathtext_dejavuserif_34.png",
"mathtext_stix_34.png",
"mathtext_stixsans_34.png",
"mathtext_cm_34.svg",
"mathtext_dejavusans_34.svg",
"mathtext_dejavuserif_34.svg",
"mathtext_stix_34.svg",
"mathtext_stixsans_34.svg",
"mathtext_cm_52.pdf",
"mathtext_dejavusans_52.pdf",
"mathtext_dejavuserif_52.pdf",
"mathtext_stix_52.pdf",
"mathtext_stixsans_52.pdf",
"mathtext_cm_52.png",
"mathtext_dejavusans_52.png",
"mathtext_dejavuserif_52.png",
"mathtext_stix_52.png",
"mathtext_stixsans_52.png",
"mathtext_cm_52.svg",
"mathtext_dejavusans_52.svg",
"mathtext_dejavuserif_52.svg",
"mathtext_stix_52.svg",
"mathtext_stixsans_52.svg",
"mathtext_cm_67.pdf",
"mathtext_dejavusans_67.pdf",
"mathtext_dejavuserif_67.pdf",
"mathtext_stix_67.pdf",
"mathtext_stixsans_67.pdf",
"mathtext_cm_67.png",
"mathtext_dejavusans_67.png",
"mathtext_dejavuserif_67.png",
"mathtext_stix_67.png",
"mathtext_stixsans_67.png",
"mathtext_cm_67.svg",
"mathtext_dejavusans_67.svg",
"mathtext_dejavuserif_67.svg",
"mathtext_stix_67.svg",
"mathtext_stixsans_67.svg"
] |
It would need a second review. |
Just a few points regarding the baseline images changes:
diff --git i/lib/matplotlib/tests/test_mathtext.py w/lib/matplotlib/tests/test_mathtext.py
index 16ac7c7ba..b5fd906d2 100644
--- i/lib/matplotlib/tests/test_mathtext.py
+++ w/lib/matplotlib/tests/test_mathtext.py
@@ -31,11 +31,13 @@ math_tests = [
r'$x^2$',
r'$x^2_y$',
r'$x_y^2$',
- r'$\prod_{i=\alpha_{i+1}}^\infty$',
+ (r'$\sum _{\genfrac{}{}{0}{}{0\leq i\leq m}{0<j<n}}f\left(i,j\right)'
+ r'\mathcal{R}\prod_{i=\alpha_{i+1}}^\infty a_i \sin(2 \pi f x_i)'
+ r"\sqrt[2]{\prod^\frac{x}{2\pi^2}_\infty}$"),
r'$x = \frac{x+\frac{5}{2}}{\frac{y+3}{8}}$',
r'$dz/dt = \gamma x^2 + {\rm sin}(2\pi y+\phi)$',
r'Foo: $\alpha_{i+1}^j = {\rm sin}(2\pi f_j t_i) e^{-5 t_i/\tau}$',
- r'$\mathcal{R}\prod_{i=\alpha_{i+1}}^\infty a_i \sin(2 \pi f x_i)$',
+ None,
r'Variable $i$ is good',
r'$\Delta_i^j$',
r'$\Delta^j_{i+1}$',
@@ -47,7 +49,7 @@ math_tests = [
r"$f'\quad f'''(x)\quad ''/\mathrm{yr}$",
r'$\frac{x_2888}{y}$',
r"$\sqrt[3]{\frac{X_2}{Y}}=5$",
- r"$\sqrt[5]{\prod^\frac{x}{2\pi^2}_\infty}$",
+ None,
r"$\sqrt[3]{x}=5$",
r'$\frac{X}{\frac{X}{Y}}$',
r"$W^{3\beta}_{\delta_1 \rho_1 \sigma_2} = U^{3\beta}_{\delta_1 \rho_1} + \frac{1}{8 \pi 2} \int^{\alpha_2}_{\alpha_2} d \alpha^\prime_2 \left[\frac{ U^{2\beta}_{\delta_1 \rho_1} - \alpha^\prime_2U^{1\beta}_{\rho_1 \sigma_2} }{U^{0\beta}_{\rho_1 \sigma_2}}\right]$",
@@ -89,11 +91,13 @@ math_tests = [
r'${x}_{92}^{31415}+\pi $',
r'${x}_{{y}_{b}^{a}}^{{z}_{c}^{d}}$',
r'${y}_{3}^{\prime \prime \prime }$',
+ # End of the MathML torture tests.
+
r"$\left( \xi \left( 1 - \xi \right) \right)$", # Bug 2969451
r"$\left(2 \, a=b\right)$", # Sage bug #8125
r"$? ! &$", # github issue #466
None,
- r'$\sum _{\genfrac{}{}{0}{}{0\leq i\leq m}{0<j<n}}P\left(i,j\right)$',
+ None,
r"$\left\Vert a \right\Vert \left\vert b \right\vert \left| a \right| \left\| b\right\| \Vert a \Vert \vert b \vert$",
r'$\mathring{A} \AA$',
r'$M \, M \thinspace M \/ M \> M \: M \; M \ M \enspace M \quad M \qquad M \! M$', (together with updating image 18 for the new test) shrinks the overall size of the new baselines from 508K to 256K, a not insignificant saving. (I explicitly left test 52 alone as we can reasonably leave the "mathml torture test" as is; I also slightly tweaked the mathtext expression to reduce the number of independent glyphs used.) |
I notice something interesting: (side by side) Maybe it's hard to notice here, but there is definitely something fishy w.r.t. png vs svg backend? |
d8466c1
to
b74db71
Compare
Without looking carefully at this, it may be related to #19261 (comment) / #5414? (If so, this may change depending on |
You will need to rebase if you want to fix the docs CI build. @anntzer is there anything else left to do here? |
Nope, lgtm. |
No, it doesn't look like this has any relevant doc changes. |
b0dbead via matplotlib#19314 several tests were merged but the images for them were not removed.
b0dbead via matplotlib#19314 several tests were merged but the images for them were not removed.
PR Summary
This PR fixes the alignment issues with the current
overunder
symbols inmathtext
.Brief summary of the proposal:
(This rewrites the following IDs of the tests):
Closes #19178
PR Checklist
pytest
passes).flake8
on changed files to check).flake8-docstrings
and runflake8 --docstring-convention=all
).doc/users/next_whats_new/
(follow instructions in README.rst there).doc/api/next_api_changes/
(follow instructions in README.rst there).