Opened 7 years ago

Closed 9 months ago

Last modified 9 months ago

#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) 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)

trac_12908.patch (7.8 KB) - added by aapitzsch 7 years ago.
trac_12908_latex_blank.patch (15.9 KB) - added by aapitzsch 7 years ago.

Download all attachments as: .zip

Change History (45)

comment:1 Changed 7 years ago by ppurka

Does this really need a fix? The latex output can be used in two ways:

  1. Input to a latex document.
  2. Display on the notebook with typeset.

In both the cases, the output will be fine.

Changed 7 years ago by aapitzsch

comment:2 Changed 7 years ago by aapitzsch

  • Authors set to André Apitzsch
  • 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 7 years ago by novoselt

  • 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: Changed 7 years ago by aapitzsch

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 7 years ago by novoselt

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 aapitzsch

comment:6 Changed 7 years ago by aapitzsch

  • 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 dsmatth

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 dsmatth

  • Reviewers set to Debbie Matthews
  • Status changed from needs_review to needs_work

comment:9 Changed 6 years ago by novoselt

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 jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:11 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:12 Changed 6 years ago by rws

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 vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:14 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:15 Changed 10 months ago by embray

  • Milestone sage-6.4 deleted

comment:16 Changed 10 months ago by gh-durgeshra

  • Branch set to u/gh-durgeshra/unnecessary_blank_in_latex_expression_of_multivariate_polynomial

comment:17 Changed 10 months ago by gh-durgeshra

  • 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:

7573eb9Remove unnecessary blank in latex expression of multivariate polynomial

comment:18 follow-up: Changed 10 months ago by embray

  • 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 10 months ago by git

  • Commit changed from 7573eb9aecb377d47d199654b6ad591db13204cd to ea2d10f5c0083d54d2fd7b95da6d159199fd9df5

Branch pushed to git repo; I updated commit sha1. New commits:

ea2d10fAdd test case

comment:20 in reply to: ↑ 18 ; follow-up: Changed 10 months ago by gh-durgeshra

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 10 months ago by charpent

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 10 months ago by gh-durgeshra

  • Status changed from needs_work to needs_review

comment:23 follow-up: Changed 10 months ago by embray

  • 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 
    460460        sage: P.<v,w> = K[]
    461461        sage: latex((z+1)*v*w - z*w^2 + z*v + z^2*w + z + 1)
    462462        \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
    463471    """
    464472    poly = ""
    465473    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 10 months ago by git

  • Commit changed from ea2d10f5c0083d54d2fd7b95da6d159199fd9df5 to 75d995617ab858f83ee7e80e28c7de524770390a

Branch pushed to git repo; I updated commit sha1. New commits:

75d9956Nit fixes

comment:25 Changed 10 months ago by gh-durgeshra

  • Status changed from needs_work to needs_review

comment:26 in reply to: ↑ 23 Changed 10 months ago by charpent

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:

75d9956Nit fixes

comment:27 Changed 10 months ago by charpent

  • Status changed from needs_review to needs_work

comment:28 Changed 10 months ago by charpent

  • Cc charpent added

comment:29 Changed 10 months ago by git

  • Commit changed from 75d995617ab858f83ee7e80e28c7de524770390a to 3ab45d5fe2e106436b12e0c7cfed99d0416525fa

Branch pushed to git repo; I updated commit sha1. New commits:

3ab45d5Fix doctests

comment:30 Changed 10 months ago by gh-durgeshra

  • Status changed from needs_work to needs_review

comment:31 Changed 10 months ago by charpent

  • 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 9 months ago by embray

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 9 months ago by embray

  • 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:

86d9920Remove unnecessary blank in latex expression of multivariate polynomial

comment:34 Changed 9 months ago by chapoton

  • Milestone set to sage-8.7

comment:35 Changed 9 months ago by vbraun

  • Branch changed from public/ticket-12908 to 86d9920a0dea3d92b062f34636465991df0591a3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:36 follow-up: Changed 9 months ago by slelievre

  • Commit 86d9920a0dea3d92b062f34636465991df0591a3 deleted

Have all authors got their names in the "Authors" field?

comment:37 in reply to: ↑ 36 Changed 9 months ago by gh-durgeshra

I haven't. Will do right now. Edit: Done Replying to slelievre:

Have all authors got their names in the "Authors" field?

Last edited 9 months ago by gh-durgeshra (previous) (diff)

comment:38 Changed 9 months ago by slelievre

I mean on the Trac ticket. Click "Modify Ticket", then add your full name to the Authors field.

comment:39 Changed 9 months ago by gh-durgeshra

  • Authors changed from André Apitzsch to André Apitzsch, Durgesh Agrawal

comment:40 Changed 9 months ago by jhpalmieri

The ticket's already been merged, so I don't know if the change in authors will have any effect.

comment:41 Changed 9 months ago by slelievre

It will matter when producing the changelog for SageMath 8.7.

comment:42 follow-up: Changed 9 months ago by jhpalmieri

Oh, the changelog isn't just based on the git log?

comment:43 in reply to: ↑ 42 Changed 9 months ago by jdemeyer

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!

Note: See TracTickets for help on using tickets.