#12908 closed defect (fixed)
unnecessary blank in latex expression of multivariate polynomial
Reported by:  dkrenn  Owned by:  was 

Priority:  trivial  Milestone:  sage8.7 
Component:  user interface  Keywords:  beginner, latex, blank, multivariate polynomial 
Cc:  charpent  Merged in:  
Authors:  André Apitzsch, Durgesh Agrawal  Reviewers:  Debbie Matthews, Erik Bray, Emmanuel Charpentier 
Report Upstream:  N/A  Work issues:  add additional regression test, formatting nitpicks 
Branch:  86d9920 (Commits)  Commit:  
Dependencies:  Stopgaps: 
Description
In the LaTeX representation of a multivariate polynomial is an unnecessary blank when variable is concatenated with a 
, see
sage: R.<X,Y> = ZZ[] sage: latex(XY) X  Y sage: latex(X^2X) X^{2}  X
It does not happen with +
sage: latex(X+Y) X + Y
or with a constant
sage: latex(X1) X  1
In the univarate case the problem does not appear.
Note that this "wrong" output is no problem (i.e. a correct input for LaTeX)
Attachments (2)
Change History (45)
comment:1 Changed 7 years ago by
Changed 7 years ago by
comment:2 Changed 7 years ago by
 Status changed from new to needs_review
The attached patch removes the unnecessary blank.
But now we also have
sage: latex(X^2X) X^{2}  X
instead of
sage: latex(X^2X)  X^{2}  X
Is this reasonable or should we find a solution to keep the blank between the leading variable and its sign ("
")?
comment:3 Changed 7 years ago by
 Status changed from needs_review to needs_work
No, it is not reasonable. As it was pointed out, the current output is correct and the only reason to "fix" is aesthetics. From this point of view, getting rid of a double space and swallowing a single one is not good  better to have more spaces then less.
comment:4 followup: ↓ 5 Changed 7 years ago by
Currently we have
sage: latex(x^2x) x^{2}  x sage: R.<y> = ZZ[] sage: latex(y^2y) y^{2}  y
Notice, there is no blank in the leading term, if it's no multivariate polynomial. So why do we need one if it's a multivariate polynomial?
IMO it should be consistent.
comment:5 in reply to: ↑ 4 Changed 7 years ago by
Replying to aapitzsch:
IMO it should be consistent.
Yes, and this means that there is always exactly one space, if it is missing, it is the same "bug" as with double spaces or even worse since it makes reading more difficult and interferes with word wrapping.
A more useful LaTeX alteration than decreasing spaces would be getting rid of {}
around singletons, i.e. instead of x_{1}^{2}
it would be better to have x_1^2
, again for reasons of readability and subsequent "manual manipulations". Macaulay2 is making such a change, as can be seen in one of the failing tests here:
http://trac.sagemath.org/sage_trac/ticket/11710#comment:23
Changed 7 years ago by
comment:6 Changed 7 years ago by
 Status changed from needs_work to needs_review
Apply only trac_12908_latex_blank.patch.
This patch doesn't fix all "space bugs" in LaTeX expressions, only those which are related to the bug mentioned at the top.
comment:7 Changed 6 years ago by
I attempted to apply trac_12908_latex_blank.patch to Sage 5.10, but it appears rotted.
~/sage/devel/sage$ hg qimport trac_12908_latex_blank.patch adding trac_12908_latex_blank.patch to series file ~/sage/devel/sage$ hg qpush applying trac_12908_latex_blank.patch patching file doc/de/tutorial/latex.rst Hunk #1 succeeded at 73 with fuzz 1 (offset 5 lines). patching file doc/en/tutorial/latex.rst Hunk #1 succeeded at 70 with fuzz 1 (offset 10 lines). patching file sage/algebras/free_algebra_quotient_element.py Hunk #1 FAILED at 136 1 out of 1 hunks FAILED  saving rejects to file sage/algebras/free_algebra_quotient_element.py.rej patching file sage/combinat/free_module.py Hunk #1 FAILED at 1032 1 out of 1 hunks FAILED  saving rejects to file sage/combinat/free_module.py.rej patching file sage/rings/polynomial/polynomial_quotient_ring_element.py Hunk #1 FAILED at 185 1 out of 1 hunks FAILED  saving rejects to file sage/rings/polynomial/polynomial_quotient_ring_element.py.rej patch failed, unable to continue (try v) patch failed, rejects left in working dir errors during apply, please fix and refresh trac_12908_latex_blank.patch
On a side note, I would like to vote that the space between the sign and leading term should not be there. I am new to sage and trac, but I prefer
X  Y
as opposed to
 X  Y
The leading negative indicates an inverse where as the subtraction between objects is an operation (adding an inverse). That is:
X + Y = X  Y
Since the original patch has the same problems applying, I will change the status to needs work.
comment:8 Changed 6 years ago by
 Reviewers set to Debbie Matthews
 Status changed from needs_review to needs_work
comment:9 Changed 6 years ago by
Are there any particular style guidelines that recommend smaller space for unitary minus? I personally prefer the same space as for binary for consistency.
comment:10 Changed 6 years ago by
 Milestone changed from sage5.11 to sage5.12
comment:11 Changed 5 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:12 Changed 5 years ago by
FWIW, Don Knuth doesn't use spaces in his TeXbook
. A better style guide you won't get.
comment:13 Changed 5 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:14 Changed 5 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:15 Changed 7 weeks ago by
 Milestone sage6.4 deleted
comment:16 Changed 7 weeks ago by
 Branch set to u/ghdurgeshra/unnecessary_blank_in_latex_expression_of_multivariate_polynomial
comment:17 Changed 7 weeks ago by
 Commit set to 7573eb9aecb377d47d199654b6ad591db13204cd
Testing:
sage: R.<X,Y> = ZZ[] sage: latex(XY) X  Y
(single space present between '' and Y)
sage: latex(X^2X) X^{2}  X
(single space present between '' and X)
New commits:
7573eb9  Remove unnecessary blank in latex expression of multivariate polynomial

comment:18 followup: ↓ 20 Changed 7 weeks ago by
 Reviewers changed from Debbie Matthews to Debbie Matthews, Erik Bray
 Work issues set to add regression tests as doctests to singular_polynomial_latex
Thanks! Please add the tests to the source in a followup commit. You can add the test cases in the docstring for singular_polynomial_latex
. You can follow the format of the one existing regression test that's already there. Just put this one after the test for #11186. I would also add a test for the leading 
case as in:
sage: latex(x^2x) x^{2}  x sage: R.<y> = ZZ[] sage: latex(y^2y) y^{2}  y
comment:19 Changed 7 weeks ago by
 Commit changed from 7573eb9aecb377d47d199654b6ad591db13204cd to ea2d10f5c0083d54d2fd7b95da6d159199fd9df5
Branch pushed to git repo; I updated commit sha1. New commits:
ea2d10f  Add test case

comment:20 in reply to: ↑ 18 ; followup: ↓ 21 Changed 7 weeks ago by
Replying to embray:
Thanks! Please add the tests to the source in a followup commit. You can add the test cases in the docstring for
singular_polynomial_latex
. You can follow the format of the one existing regression test that's already there. Just put this one after the test for #11186. I would also add a test for the leading
case as in:sage: latex(x^2x) x^{2}  x sage: R.<y> = ZZ[] sage: latex(y^2y) y^{2}  y
Done.
comment:21 in reply to: ↑ 20 Changed 7 weeks ago by
Dear Durgesh,
1) Welcome aboard !
2) About your proposal :
Replying to ghdurgeshra:
[ Snip... ]
Done.
A better way to pass that information would have been to change the state of the ticket to needs_work
: this way, all people working (or having worked) on that ticket would have received the (useful) info that a proposed fix needed review.
I'll try to have a look later this weekend (possibly today).
comment:22 Changed 7 weeks ago by
 Status changed from needs_work to needs_review
comment:23 followup: ↓ 26 Changed 7 weeks ago by
 Status changed from needs_review to needs_work
 Work issues changed from add regression tests as doctests to singular_polynomial_latex to add additional regression test, formatting nitpicks

src/sage/libs/singular/polynomial.pyx
diff git a/src/sage/libs/singular/polynomial.pyx b/src/sage/libs/singular/polynomial.pyx index 2e4efb1..a494142 100644
a b cdef object singular_polynomial_latex(poly *p, ring *r, object base, object late 460 460 sage: P.<v,w> = K[] 461 461 sage: latex((z+1)*v*w  z*w^2 + z*v + z^2*w + z + 1) 462 462 \left(z + 1\right) v w  z w^{2} + z v + \left(z  1\right) w + z + 1 463 464 Demonstrate that there is no blank in latex expression of multivariate 465 polynomial (:trac:`12908`):: 466 sage: R.<X,Y> = ZZ[] 467 sage: latex(XY) 468 X  Y 469 sage: latex(X^2X) 470 X^{2}  X 463 471 """ 464 472 poly = "" 465 473 cdef unsigned long e
Please include a blank line after the ::
as in other examples. I think it isn't strictly needed but it's stylistically more consistent. I would also reword the text to "Demonstrate that there are no extra blanks in latex expressions of multivariate polynomial". The wording you have is confusing: There are blanks, and this demonstrates that there are. What we want is not extra, superfluous blanks. Please also add an example with a leading unary 
as mentioned in comment:18.
Sorry for the nitpicks but that's why it's nice for new contributors to start with an easy ticket :) Looks like you're almost there.
comment:24 Changed 7 weeks ago by
 Commit changed from ea2d10f5c0083d54d2fd7b95da6d159199fd9df5 to 75d995617ab858f83ee7e80e28c7de524770390a
Branch pushed to git repo; I updated commit sha1. New commits:
75d9956  Nit fixes

comment:25 Changed 7 weeks ago by
 Status changed from needs_work to needs_review
comment:26 in reply to: ↑ 23 Changed 7 weeks ago by
Replying to embray:
[ Snip... ]
Sorry for the nitpicks but that's why it's nice for new contributors to start with an easy ticket :) Looks like you're almost there.
Ahem...
Apart from (justifiable) nitpickings, this (correct) fix breaks three other doctests that rely on a specific latex representation :
 sage t long warnlong 144.0 src/sage/modular/modform_hecketriangle/graded_ring_element.py # 1 doctest failed sage t long warnlong 144.0 src/sage/modular/modform_hecketriangle/element.py # 1 doctest failed sage t long warnlong 144.0 src/sage/algebras/weyl_algebra.py # 1 doctest failed 
Here are the errors :
charpent@zenbookflip:/usr/local/sage8$ sage t long warnlong 144.0 src/sage/modular/modform_hecketriangle/graded_ring_element.py Running doctests with ID 201901041339474a9651a6. Git branch: t/12908/unnecessary_blank_in_latex_expression_of_multivariate_polynomial Using optional=dochtml,fricas,gap_packages,giacpy_sage,gmpy2,memlimit,mpir,python2,sage Doctesting 1 file. sage t long warnlong 144.0 src/sage/modular/modform_hecketriangle/graded_ring_element.py ********************************************************************** File "src/sage/modular/modform_hecketriangle/graded_ring_element.py", line 252, in sage.modular.modform_hecketriangle.graded_ring_element.?._latex_ Failed example: latex(QuasiModularFormsRing(n=infinity)(x*(xy^2)*z)) Expected:  E_{4} f_{i}^{2} E_{2} + E_{4}^{2} E_{2} Got: E_{4} f_{i}^{2} E_{2} + E_{4}^{2} E_{2} ********************************************************************** 1 item had failures: 1 of 7 in sage.modular.modform_hecketriangle.graded_ring_element.?._latex_ [672 tests, 1 failure, 17.26 s]  sage t long warnlong 144.0 src/sage/modular/modform_hecketriangle/graded_ring_element.py # 1 doctest failed  Total time for all tests: 17.4 seconds cpu time: 17.0 seconds cumulative wall time: 17.3 seconds charpent@zenbookflip:/usr/local/sage8$ sage t long warnlong 144.0 src/sage/modular/modform_hecketriangle/element.py Running doctests with ID 20190104134022f8799c62. Git branch: t/12908/unnecessary_blank_in_latex_expression_of_multivariate_polynomial Using optional=dochtml,fricas,gap_packages,giacpy_sage,gmpy2,memlimit,mpir,python2,sage Doctesting 1 file. sage t long warnlong 144.0 src/sage/modular/modform_hecketriangle/element.py ********************************************************************** File "src/sage/modular/modform_hecketriangle/element.py", line 111, in sage.modular.modform_hecketriangle.element.FormsElement._latex_ Failed example: latex(QuasiModularForms(n=infinity, k=8, ep=1)(x*(xy^2))) Expected:  E_{4} f_{i}^{2} + E_{4}^{2} Got: E_{4} f_{i}^{2} + E_{4}^{2} ********************************************************************** 1 item had failures: 1 of 5 in sage.modular.modform_hecketriangle.element.FormsElement._latex_ [83 tests, 1 failure, 2.43 s]  sage t long warnlong 144.0 src/sage/modular/modform_hecketriangle/element.py # 1 doctest failed  Total time for all tests: 7.5 seconds cpu time: 2.2 seconds cumulative wall time: 2.4 seconds charpent@zenbookflip:/usr/local/sage8$ sage t long warnlong 144.0 src/sage/algebras/weyl_algebra.py # 1 doctest failed Running doctests with ID 20190104134051fc1e6dd9. Git branch: t/12908/unnecessary_blank_in_latex_expression_of_multivariate_polynomial Using optional=dochtml,fricas,gap_packages,giacpy_sage,gmpy2,memlimit,mpir,python2,sage Doctesting 1 file. sage t long warnlong 144.0 src/sage/algebras/weyl_algebra.py ********************************************************************** File "src/sage/algebras/weyl_algebra.py", line 112, in sage.algebras.weyl_algebra.repr_from_monomials Failed example: latex(c*(a*a + 2)*b) Expected: \left(  x  2 \right) e_{1} e_{2}  4 x  8 Got: \left( x  2 \right) e_{1} e_{2}  4 x  8 ********************************************************************** 1 item had failures: 1 of 28 in sage.algebras.weyl_algebra.repr_from_monomials [159 tests, 1 failure, 0.18 s]  sage t long warnlong 144.0 src/sage/algebras/weyl_algebra.py # 1 doctest failed  Total time for all tests: 0.2 seconds cpu time: 0.2 seconds cumulative wall time: 0.2 seconds
In all cases, the change introduced by the ticket gives a correct LaTeX representation, differing just by the amount of space. These doctests should be fixed.
New commits:
75d9956  Nit fixes

comment:27 Changed 7 weeks ago by
 Status changed from needs_review to needs_work
comment:28 Changed 7 weeks ago by
 Cc charpent added
comment:29 Changed 7 weeks ago by
 Commit changed from 75d995617ab858f83ee7e80e28c7de524770390a to 3ab45d5fe2e106436b12e0c7cfed99d0416525fa
Branch pushed to git repo; I updated commit sha1. New commits:
3ab45d5  Fix doctests

comment:30 Changed 7 weeks ago by
 Status changed from needs_work to needs_review
comment:31 Changed 7 weeks ago by
 Reviewers changed from Debbie Matthews, Erik Bray to Debbie Matthews, Erik Bray, Emmanuel Charpentier
 Status changed from needs_review to positive_review
Merged on 8.6.rc0, passes ptestlong ==> positive_review
.
comment:32 Changed 7 weeks ago by
I figured there would be some new test failures as a result of this, and was going to check that next after all other details were dealt with? Also, I'd prefer people squash their commits when making lots of little minor changes so I will do that.
comment:33 Changed 7 weeks ago by
 Branch changed from u/ghdurgeshra/unnecessary_blank_in_latex_expression_of_multivariate_polynomial to public/ticket12908
 Commit changed from 3ab45d5fe2e106436b12e0c7cfed99d0416525fa to 86d9920a0dea3d92b062f34636465991df0591a3
Squashed. Thanks Durgesh.
New commits:
86d9920  Remove unnecessary blank in latex expression of multivariate polynomial

comment:34 Changed 7 weeks ago by
 Milestone set to sage8.7
comment:35 Changed 4 weeks ago by
 Branch changed from public/ticket12908 to 86d9920a0dea3d92b062f34636465991df0591a3
 Resolution set to fixed
 Status changed from positive_review to closed
comment:36 followup: ↓ 37 Changed 4 weeks ago by
 Commit 86d9920a0dea3d92b062f34636465991df0591a3 deleted
Have all authors got their names in the "Authors" field?
comment:37 in reply to: ↑ 36 Changed 4 weeks ago by
I haven't. Will do right now. Edit: Done Replying to slelievre:
Have all authors got their names in the "Authors" field?
comment:38 Changed 4 weeks ago by
I mean on the Trac ticket. Click "Modify Ticket", then add your full name
to the Authors
field.
comment:39 Changed 4 weeks ago by
comment:40 Changed 4 weeks ago by
The ticket's already been merged, so I don't know if the change in authors will have any effect.
comment:41 Changed 4 weeks ago by
It will matter when producing the changelog for SageMath 8.7.
comment:42 followup: ↓ 43 Changed 4 weeks ago by
Oh, the changelog isn't just based on the git log?
comment:43 in reply to: ↑ 42 Changed 4 weeks ago by
Replying to jhpalmieri:
Oh, the changelog isn't just based on the git log?
No, it's based on the information on Trac. In fact, the changeloggenerating script predates the switch to git!
Does this really need a fix? The latex output can be used in two ways:
In both the cases, the output will be fine.