Opened 10 years ago

Closed 4 years ago

#6551 closed enhancement (fixed)

fix ugliness in printing of multivariate polynomials

Reported by: AlexGhitza Owned by: AlexGhitza
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: algebra Keywords: print latex multivariate polynomial
Cc: malb Merged in:
Authors: Alex Ghitza Reviewers: David Loeffler
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

The printing (and latex-ing) of multivariate polynomials is sometimes quite ugly, and inconsistent with the much prettier printing of univariate polynomials. One gets things like the following (taken from doctests in the Sage library):

(-6/5)*x^2*y^2 + (-3)*x*y^3 + 6/5*x^2*y + 11/12*x*y^2 + (-18)*y^2 + (-3/4)*y

or even

sage: xgcd((b+g)*y^2, (a+g)*y+b)
((b^3 + (g)*b^2)/(a^2 + (2*g)*a + 3), 1, ((-b + (-g))/(a + (g)))*y + (b^2 + (g)*b)/(a^2 + (2*g)*a + 3))

The attached patch fixes this, factors out common code for printing and latex-ing, and makes printing consistent across various representations of multivariate polynomials.

Attachments (3)

trac_6551.patch (92.8 KB) - added by AlexGhitza 10 years ago.
trac_6551-rebased_for_6500.patch (91.2 KB) - added by davidloeffler 10 years ago.
apply after the three patches at #6500
trac_6551_fix.patch (1.4 KB) - added by AlexGhitza 10 years ago.

Download all attachments as: .zip

Change History (21)

Changed 10 years ago by AlexGhitza

comment:1 Changed 10 years ago by AlexGhitza

  • Owner changed from tbd to AlexGhitza
  • Status changed from new to assigned
  • Summary changed from fix ugliness in printing of multivariate polynomials to [with patch, needs review] fix ugliness in printing of multivariate polynomials

comment:2 Changed 10 years ago by davidloeffler

Bad news: this conflicts quite severely with my patches at #6500. I feel a bit guilty about this, because it was my referee comments on #6183 that pushed you into working on this, so I will handle preparing a rebased version.

Changed 10 years ago by davidloeffler

apply after the three patches at #6500

comment:3 Changed 10 years ago by davidloeffler

Here's a patch that applies cleanly on top of the #6500 patches. For what it's worth, the patch looks fine to me, but I'm not an expert on multivariate commutative algebra so probably it'd be better to get someone else to review it.

comment:4 Changed 10 years ago by davidloeffler

  • Summary changed from [with patch, needs review] fix ugliness in printing of multivariate polynomials to [with patch, needs work] fix ugliness in printing of multivariate polynomials

Hi Alex,

I decided to run all doctests just to check that I had rebased the patch correctly, but there seems to be some funny business in quaternion algebras. This is with the #6500 patches and the rebased patch here, applied to 4.1:

sage -t  "devel/sage/sage/algebras/quaternion_algebra_element.py"                                      
**********************************************************************                                 
File "/home/david/sage-4.1/devel/sage/sage/algebras/quaternion_algebra_element.py", line 17:           
    sage: sage.algebras.quaternion_algebra_element.unpickle_QuaternionAlgebraElement_generic_v0(*t)    
Expected:                                                                                              
    2/3 + X*i - X^2*j + X^3*k                                                                          
Got:                                                                                                   
    2/3 + X*i + (-X^2)*j + X^3*k                                                                       
**********************************************************************                                 

and

**********************************************************************                                 
File "/home/david/sage-4.1/devel/sage/sage/algebras/quatalg/quaternion_algebra.py", line 455:          
    sage: QuaternionAlgebra(GF(17)(2),3).random_element()                                              
Expected:                                                                                              
    11 + 16*i + 4*j + 13*k                                                                             
Got:                                                                                                   
    11 - i + 4*j + 13*k                                                                                
**********************************************************************                                 

This has nothing to do with the rebasing, because I ran these two tests again using your original patch and without the #6500 patches and they failed in exactly the same way. Any idea what is going on here?

David

comment:5 Changed 10 years ago by davidloeffler

  • Authors set to Alex Ghitza
  • Reviewers set to David Loeffler

comment:6 Changed 10 years ago by AlexGhitza

Hi David,

I just checked and I'm getting the same failures as you both on my laptop and sage.math (which is weird because I could swear I tested this about a million times).

I have an inkling of what might be going on but it's going to have to wait until tomorrow because I'm falling asleep in my chair.

Thanks for reviewing, and for the work you put in rebasing the patch.

Changed 10 years ago by AlexGhitza

comment:7 Changed 10 years ago by AlexGhitza

  • Summary changed from [with patch, needs work] fix ugliness in printing of multivariate polynomials to [with patch, needs review] fix ugliness in printing of multivariate polynomials

Yes, the problem was indeed that I was mixing up some stuff with #6183.

I've added a small patch that takes care of this, and all tests (should) now pass.

comment:8 Changed 10 years ago by malb

  • Summary changed from [with patch, needs review] fix ugliness in printing of multivariate polynomials to [with patch, needs work] fix ugliness in printing of multivariate polynomials

Unfortunately, the patch has bit-rotted:

applying trac_6551-rebased_for_6500.patch
patching file sage/matrix/matrix_mpolynomial_dense.pyx
Hunk #1 FAILED at 0
1 out of 6 hunks FAILED -- saving rejects to file sage/matrix/matrix_mpolynomial_dense.pyx.rej
patching file sage/rings/polynomial/polydict.pyx
Hunk #5 succeeded at 887 with fuzz 1 (offset 0 lines).
patching file sage/rings/polynomial/toy_buchberger.py
Hunk #1 FAILED at 53
1 out of 1 hunks FAILED -- saving rejects to file sage/rings/polynomial/toy_buchberger.py.rej
patching file sage/schemes/elliptic_curves/ell_curve_isogeny.py
Hunk #7 FAILED at 1864
1 out of 21 hunks FAILED -- saving rejects to file sage/schemes/elliptic_curves/ell_curve_isogeny.py.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
Errors during apply, please fix and refresh trac_6551-rebased_for_6500.patch

I read the patch and it looks fine so far. It is mainly a question of taste anyway IMHO. However, it might be worth checking for performance loses due to this patch (conversion to strings is used to communicate with Singular for instance)

comment:9 Changed 10 years ago by malb

After thinking about this some more time: I think the proposed implementation for libSingular polynomials is way too slow. I suggest someone (e.g. me) re-implements the PolyDict? printing for those to make it reasonably efficient.

comment:10 Changed 10 years ago by AlexGhitza

Martin,

Thanks for looking at this carefully, and sorry for having left things somewhat half-baked. In my defense, I had already spent a long time getting the formatting to be consistent, and I'm not sure I'd be very good at speed issues. But I completely agree with you that this is ubiquitous code that should be fast. Count on me to referee your implementation when you're done with it.

comment:11 Changed 10 years ago by AlexGhitza

  • Summary changed from [with patch, needs work] fix ugliness in printing of multivariate polynomials to fix ugliness in printing of multivariate polynomials

comment:12 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:13 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:14 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:15 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:16 Changed 4 years ago by bruno

  • Milestone changed from sage-6.4 to sage-duplicate/invalid/wontfix
  • Report Upstream set to N/A

This ticket seems invalid. For the first example:

sage: R.<x,y> = QQ[]
sage: p = (-6/5)*x^2*y^2 + (-3)*x*y^3 + 6/5*x^2*y + 11/12*x*y^2 + (-18)*y^2 + (-3/4)*y
sage: p
-6/5*x^2*y^2 - 3*x*y^3 + 6/5*x^2*y + 11/12*x*y^2 - 18*y^2 - 3/4*y

I cannot make the second example work (and this does not appear in the doctests).

comment:17 Changed 4 years ago by bruno

  • Status changed from needs_work to positive_review

comment:18 Changed 4 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.