#12908 closed defect (fixed)
unnecessary blank in latex expression of multivariate polynomial
Reported by: | dkrenn | Owned by: | was |
---|---|---|---|
Priority: | trivial | Milestone: | sage-8.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, GitHub, GitLab) | 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(X-Y) X - Y sage: latex(X^2-X) X^{2} - X
It does not happen with +
sage: latex(X+Y) X + Y
or with a constant
sage: latex(X-1) 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 10 years ago by
Changed 10 years ago by
comment:2 Changed 10 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^2-X) -X^{2} - X
instead of
sage: latex(-X^2-X) - 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 10 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 follow-up: ↓ 5 Changed 10 years ago by
Currently we have
sage: latex(-x^2-x) -x^{2} - x sage: R.<y> = ZZ[] sage: latex(-y^2-y) -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 10 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 10 years ago by
comment:6 Changed 10 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 9 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 9 years ago by
- Reviewers set to Debbie Matthews
- Status changed from needs_review to needs_work
comment:9 Changed 9 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 9 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:11 Changed 8 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:12 Changed 8 years ago by
FWIW, Don Knuth doesn't use spaces in his TeXbook
. A better style guide you won't get.
comment:13 Changed 8 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:14 Changed 8 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:15 Changed 3 years ago by
- Milestone sage-6.4 deleted
comment:16 Changed 3 years ago by
- Branch set to u/gh-durgeshra/unnecessary_blank_in_latex_expression_of_multivariate_polynomial
comment:17 Changed 3 years ago by
- Commit set to 7573eb9aecb377d47d199654b6ad591db13204cd
Testing:
sage: R.<X,Y> = ZZ[] sage: latex(X-Y) X - Y
(single space present between '-' and Y)
sage: latex(X^2-X) X^{2} - X
(single space present between '-' and X)
New commits:
7573eb9 | Remove unnecessary blank in latex expression of multivariate polynomial
|
comment:18 follow-up: ↓ 20 Changed 3 years 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 follow-up 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^2-x) -x^{2} - x sage: R.<y> = ZZ[] sage: latex(-y^2-y) -y^{2} - y
comment:19 Changed 3 years 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 ; follow-up: ↓ 21 Changed 3 years ago by
Replying to embray:
Thanks! Please add the tests to the source in a follow-up 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^2-x) -x^{2} - x sage: R.<y> = ZZ[] sage: latex(-y^2-y) -y^{2} - y
Done.
comment:21 in reply to: ↑ 20 Changed 3 years ago by
Dear Durgesh,
1) Welcome aboard !
2) About your proposal :
Replying to gh-durgeshra:
[ 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 week-end (possibly today).
comment:22 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:23 follow-up: ↓ 26 Changed 3 years 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(X-Y) 468 X - Y 469 sage: latex(X^2-X) 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 3 years 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 3 years ago by
- Status changed from needs_work to needs_review
comment:26 in reply to: ↑ 23 Changed 3 years 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 --warn-long 144.0 src/sage/modular/modform_hecketriangle/graded_ring_element.py # 1 doctest failed sage -t --long --warn-long 144.0 src/sage/modular/modform_hecketriangle/element.py # 1 doctest failed sage -t --long --warn-long 144.0 src/sage/algebras/weyl_algebra.py # 1 doctest failed ----------------------------------------------------------------------
Here are the errors :
charpent@zen-book-flip:/usr/local/sage-8$ sage -t --long --warn-long 144.0 src/sage/modular/modform_hecketriangle/graded_ring_element.py Running doctests with ID 2019-01-04-13-39-47-4a9651a6. 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 --warn-long 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*(x-y^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 --warn-long 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@zen-book-flip:/usr/local/sage-8$ sage -t --long --warn-long 144.0 src/sage/modular/modform_hecketriangle/element.py Running doctests with ID 2019-01-04-13-40-22-f8799c62. 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 --warn-long 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*(x-y^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 --warn-long 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@zen-book-flip:/usr/local/sage-8$ sage -t --long --warn-long 144.0 src/sage/algebras/weyl_algebra.py # 1 doctest failed Running doctests with ID 2019-01-04-13-40-51-fc1e6dd9. 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 --warn-long 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 --warn-long 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 3 years ago by
- Status changed from needs_review to needs_work
comment:28 Changed 3 years ago by
- Cc charpent added
comment:29 Changed 3 years 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 3 years ago by
- Status changed from needs_work to needs_review
comment:31 Changed 3 years 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 3 years 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 3 years ago by
- Branch changed from u/gh-durgeshra/unnecessary_blank_in_latex_expression_of_multivariate_polynomial to public/ticket-12908
- Commit changed from 3ab45d5fe2e106436b12e0c7cfed99d0416525fa to 86d9920a0dea3d92b062f34636465991df0591a3
Squashed. Thanks Durgesh.
New commits:
86d9920 | Remove unnecessary blank in latex expression of multivariate polynomial
|
comment:34 Changed 3 years ago by
- Milestone set to sage-8.7
comment:35 Changed 3 years ago by
- Branch changed from public/ticket-12908 to 86d9920a0dea3d92b062f34636465991df0591a3
- Resolution set to fixed
- Status changed from positive_review to closed
comment:36 follow-up: ↓ 37 Changed 3 years ago by
- Commit 86d9920a0dea3d92b062f34636465991df0591a3 deleted
Have all authors got their names in the "Authors" field?
comment:37 in reply to: ↑ 36 Changed 3 years 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 3 years ago by
I mean on the Trac ticket. Click "Modify Ticket", then add your full name
to the Authors
field.
comment:39 Changed 3 years ago by
comment:40 Changed 3 years 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 3 years ago by
It will matter when producing the changelog for SageMath 8.7.
comment:42 follow-up: ↓ 43 Changed 3 years ago by
Oh, the changelog isn't just based on the git log?
comment:43 in reply to: ↑ 42 Changed 3 years 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 changelog-generating 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.