Fixing a bug in sage.misc.misc.coeff_repr

[stumpc5]

Currently, we have

sage: alpha,beta,gamma=FreeAlgebra(ZZ,3,'alpha,beta,gamma').gens()
sage: latex(alpha-beta)
\alpha + \left(-1\right)\beta

This patch turns this into

sage: alpha,beta,gamma=FreeAlgebra(ZZ,3,'alpha,beta,gamma').gens()
sage: latex(alpha-beta)
\alpha - \beta

Apply attachment: trac_13735_fix_repr_lincomb.patch

Dependencies: sage-5.10.beta4

CC: @fchapoton

Component: combinatorics

Keywords: coefficient repr

Author: Vít Tuček

Reviewer: Andrey Novoseltsev

Merged: sage-5.10.beta5

Issue created by migration from https://trac.sagemath.org/ticket/13735

[stumpc5]

comment:2

Replying to @stumpc5:

This one seems to not apply on 5.5.rc0 (while it does on 5.5.beta1). I am currently building rc0 and will fix this as soon as rc0 is ready on my machine.

[stumpc5]

comment:3

Attachment: trac_13735-fix_bug_in_coef_repr-cs.patch.gz

apply trac_13735-fix_bug_in_coef_repr-cs.patch

[novoselt]

comment:4

And what about alpha - 2 * beta? It does not look like the patch would improve this.

[stumpc5]

comment:5

Replying to @novoselt:

And what about alpha - 2 * beta? It does not look like the patch would improve this.

This is very right. And I think you're right that the patch should not only handle one particular case (which I was catching in a completely different context).

I will look into that...

[stumpc5]

comment:6

I started playing, but whenever I solved some problem, I introduced another. Since this is not really urgent (and not really necessary for the ticket I am actually working on), I leave it open for now, and we can decide later what we want to do with it.

Best, Christian

[novoselt]

comment:8

My personal feeling is that latexing of expressions is disorganized in general and it is not clear how and where bugs have to be fixed, e.g. #13356 is much more serious than the issue here, but extra parentheses annoy me every once in while too, especially when with one ring all is OK and with another one not...

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

Attachment: trac_13735_fix_latex_repr_lincomb.patch.gz

fix the representation of linear combinations and include more testcases

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

comment:9

Hi!

Scratching my own itch, I've changed the code for repr_lincomb to properly handle negative coefficients as well as zero. It required one change to how coeff_repr works which may cause problems. I've included testcases for repr_lincomb.

Is there a way how can I test the rest of sage only for this specific change? I don't want to run all the numerical test which take quite a while on my machine.

[stumpc5]

comment:10

Replying to @vit-tucek:

Hi!

Scratching my own itch, I've changed the code for repr_lincomb to properly handle negative coefficients as well as zero. It required one change to how coeff_repr works which may cause problems. I've included testcases for repr_lincomb.

feel free to take over this patch if you like - I stopped working on it since solving one issue somewhere caused others in other places.

To see if your patch causes problems somewhere, you cannot do much that running the complete doctests...

Cheers, Christian

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

comment:11

For future reference, this is the list of modules that fail after my patch. Some of these failures are clearly bugs (e.g. /tmp/sage-5.8/devel/sage/sage/combinat/sf/hall_littlewood.py)

devel/sage/sage/structure/formal_sum.py devel/sage/sage/rings/polynomial/plural.pyx devel/sage/sage/modules/finite_submodule_iter.pyx devel/sage/sage/combinat/free_module.py devel/sage/sage/combinat/root_system/weyl_characters.py devel/sage/sage/combinat/sf/llt.py devel/sage/sage/combinat/sf/hall_littlewood.py devel/sage/sage/combinat/sf/sfa.py devel/sage/sage/combinat/sf/k_dual.py devel/sage/sage/algebras/free_algebra_element.py devel/sage/sage/algebras/group_algebra_new.py devel/sage/sage/algebras/free_algebra_quotient_element.py devel/sage/sage/algebras/iwahori_hecke_algebra.py devel/sage/sage/algebras/steenrod/steenrod_algebra_mult.py devel/sage/sage/algebras/steenrod/steenrod_algebra.py devel/sage/sage/schemes/elliptic_curves/monsky_washnitzer.py devel/sage/sage/schemes/generic/divisor.py devel/sage/sage/modular/modsym/boundary.py

[novoselt]

comment:12

Can we please have a space after the leading minus? Not -x - x^2 but rather - x - x^2 etc.

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

comment:13

Sure. That's a trivial change.

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

comment:14

I am in the middle of bugfixing right now.

sage: R.<q> = ZZ[]
sage:  A2 = WeylCharacterRing(['A',2], base_ring = R, style="coroots")
sage: [A2(x) for x in [-1]]

[-1*A2(0,0)]

This is because the type of the coefficient -1 is sage.rings.polynomial.polynomial_integer_dense_flint.Polynomial_integer_dense_flint and the test c < 0 evaluates to False

Is this a bug or intended behaviour?

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

comment:15

Similar issue arises in combinat/sf/llt.py, combinat/sf/k_dual.py and combinat/sf/hall_littlewood.py, where -t > 0 and type(-t) == sage.rings.fraction_field_element.FractionFieldElement_1poly_field

I think this should be a bug since c=-t leads to

sage: c > 0 and -c > 0

True

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

Attachment: failures.txt

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

comment:16

I've hacked around the issues I mentioned and uploaded a bugfixed version of the patch. Right now I think that the only failures are due to the desired changes in output. Please see attached failures.txt and confirm this.

As for the space after leading minus as suggested by novoselt -- it can be done easily, but a lot of doctests expects it to be not there; i.e. it would require more "repairs" to doctests than current version. I think we need broader agreement to put that space there.

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

comment:17

make ptestlong in Sage 5.9 produced only the following error:

./sage sage -t --long devel/sage/sage/structure/sage_object.pyx
  File "sage", line 31
    resolvelinks() {
                   ^
SyntaxError: invalid syntax

Thus my patch is still valid.

[novoselt]

comment:18

Is the last patch the only one that has to be applied? It should also list your full name in the header, and if this is ready for review - please add your name to the authors and switch to "needs review"!

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

Changed author from Christian Stump to Christian Stump, Vít Tuček

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

comment:19

Yes. The last patch is the only one to be applied.

[novoselt]

Description changed:

--- 
+++ 
@@ -14,3 +14,4 @@
 \alpha - \beta

+Apply attachment: trac_13735_fix_repr_lincomb.patch

[novoselt]

Reviewer: Andrey Novoseltsev

[jdemeyer]

comment:22

Could this have caused

sage -t --long devel/sage/sage/modular/modsym/boundary.py
**********************************************************************
File "devel/sage/sage/modular/modsym/boundary.py", line 1299, in sage.modular.modsym.boundary.BoundarySpace_wtk_eps._coerce_cusp
Failed example:
    [ B(Cusp(i,13)) for i in range(13) ]
Expected:
    [[0],
    [1/13],
    (-zeta4)*[1/13],
    [1/13],
    (-1)*[1/13],
    (-zeta4)*[1/13],
    (-zeta4)*[1/13],
    zeta4*[1/13],
    zeta4*[1/13],
    [1/13],
    (-1)*[1/13],
    zeta4*[1/13],
    (-1)*[1/13]]
Got:
    [[0], [1/13], -zeta4*[1/13], [1/13], -[1/13], -zeta4*[1/13], -zeta4*[1/13], zeta4*[1/13], zeta4*[1/13], [1/13], -[1/13], zeta4*[1/13], -[1/13]]
**********************************************************************
File "devel/sage/sage/modular/modsym/boundary.py", line 1323, in sage.modular.modsym.boundary.BoundarySpace_wtk_eps._coerce_cusp
Failed example:
    [ B(Cusp(i,13)) for i in range(13) ]
Expected:
    [0,
    [1/13],
    (-zeta4)*[1/13],
    [1/13],
    (-1)*[1/13],
    (-zeta4)*[1/13],
    (-zeta4)*[1/13],
    zeta4*[1/13],
    zeta4*[1/13],
    [1/13],
    (-1)*[1/13],
    zeta4*[1/13],
    (-1)*[1/13]]
Got:
    [0, [1/13], -zeta4*[1/13], [1/13], -[1/13], -zeta4*[1/13], -zeta4*[1/13], zeta4*[1/13], zeta4*[1/13], [1/13], -[1/13], zeta4*[1/13], -[1/13]]
**********************************************************************

sage -t --long devel/sage/sage/modular/modsym/ambient.py
**********************************************************************
File "devel/sage/sage/modular/modsym/ambient.py", line 2100, in sage.modular.modsym.ambient.ModularSymbolsAmbient.twisted_winding_element
Failed example:
    M.twisted_winding_element(0,eps)
Expected:
    2*(1,23) + (-2)*(1,32) + 2*(1,34)
Got:
    2*(1,23) - 2*(1,32) + 2*(1,34)
**********************************************************************

**********************************************************************
File "devel/sage/sage/tests/book_schilling_zabrocki_kschur_primer.py", line 571, in sage.tests.book_schilling_zabrocki_kschur_primer
Failed example:
    p(ks3z[2, 2, 2, 2, 2, 2, 2, 2])     # long time (17s on sage.math, 2013)
Expected:
    1/12*p[4, 4, 4, 4] + 1/4*p[8, 8] + (-1/3)*p[12, 4]
Got:
    1/12*p[4, 4, 4, 4] + 1/4*p[8, 8] - 1/3*p[12, 4]
**********************************************************************
File "devel/sage/sage/tests/book_schilling_zabrocki_kschur_primer.py", line 573, in sage.tests.book_schilling_zabrocki_kschur_primer
Failed example:
    p(ks3[2,2])
Expected:
    1/12*p[1, 1, 1, 1] + 1/4*p[2, 2] + (-1/3)*p[3, 1]
Got:
    1/12*p[1, 1, 1, 1] + 1/4*p[2, 2] - 1/3*p[3, 1]
**********************************************************************
File "devel/sage/sage/tests/book_schilling_zabrocki_kschur_primer.py", line 575, in sage.tests.book_schilling_zabrocki_kschur_primer
Failed example:
    p(ks3[2,2]).plethysm(p[4])
Expected:
    1/12*p[4, 4, 4, 4] + 1/4*p[8, 8] + (-1/3)*p[12, 4]
Got:
    1/12*p[4, 4, 4, 4] + 1/4*p[8, 8] - 1/3*p[12, 4]
**********************************************************************

[jdemeyer]

Dependencies: #4327

[jdemeyer]

comment:23

With #4327:

sage -t devel/sage/sage/combinat/root_system/root_lattice_realizations.py
**********************************************************************
File "devel/sage/sage/combinat/root_system/root_lattice_realizations.py", line 1840, in sage.combinat.root_system.root_lattice_realizations.RootLatticeRealizations.Pare
ntMethods.plot_roots
Failed example:
    list(RootSystem(["A",2]).root_lattice().plot_roots("all"))
Expected:
    [Arrow from (0.0,0.0) to (1.0,0.0),
     Text '$\alpha_{1}$' at the point (1.05,0.0),
     Arrow from (0.0,0.0) to (0.0,1.0),
     Text '$\alpha_{2}$' at the point (0.0,1.05),
     Arrow from (0.0,0.0) to (1.0,1.0),
     Text '$\alpha_{1} + \alpha_{2}$' at the point (1.05,1.05),
     Arrow from (0.0,0.0) to (-1.0,0.0),
     Text '$\left(-1\right)\alpha_{1}$' at the point (-1.05,0.0),
     Arrow from (0.0,0.0) to (0.0,-1.0),
     Text '$\left(-1\right)\alpha_{2}$' at the point (0.0,-1.05),
     Arrow from (0.0,0.0) to (-1.0,-1.0),
     Text '$\left(-1\right)\alpha_{1} + \left(-1\right)\alpha_{2}$' at the point (-1.05,-1.05)]
Got:
    [Arrow from (0.0,0.0) to (1.0,0.0), Text '$\alpha_{1}$' at the point (1.05,0.0), Arrow from (0.0,0.0) to (0.0,1.0), Text '$\alpha_{2}$' at the point (0.0,1.05), Arr
ow from (0.0,0.0) to (1.0,1.0), Text '$\alpha_{1} + \alpha_{2}$' at the point (1.05,1.05), Arrow from (0.0,0.0) to (-1.0,0.0), Text '$-\alpha_{1}$' at the point (-1.05,
0.0), Arrow from (0.0,0.0) to (0.0,-1.0), Text '$-\alpha_{2}$' at the point (0.0,-1.05), Arrow from (0.0,0.0) to (-1.0,-1.0), Text '$-\alpha_{1} - \alpha_{2}$' at the p
oint (-1.05,-1.05)]
**********************************************************************

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

comment:24

The only bug I see is the change from [0] to 0, otherwise it is the correct behaviour. If you disagree, then we need to figure out what exactly _is_ the expected behaviour of rep().

Regarding the first issue - my version of sage (5.9 + my patch) does not raise any errors. Where can I get the code to debug this?

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

comment:25

Sorry, I mangled two testcases together. So it's again just the question of replacing (-1) by unary minus and replacing (-x) by -x* which I think is correct.

[jdemeyer]

Changed dependencies from #4327 to sage-5.10.beta4

[jdemeyer]

comment:26

You're right about tests passing on sage-5.9 (and even sage-5.10.beta3). I recommend you to wait for sage-5.10.beta4 and then rebase to that.

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

comment:27

Can I somehow get notified when sage reaches beta4? This patch lies here for 5 weeks and could have made it into 5.9 if I knew that one has to change the status to needs_review. (Developer's guide is somehow lacking in this regard.)

[jdemeyer]

comment:28

Replying to @vit-tucek:

Can I somehow get notified when sage reaches beta4?

I will personally notify you.

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

comment:29

Thanks. So just to be clear, the plan is to wait for 5.10 beta4 and then fix all the docstrings or possible bugs and turn the status of this patch to needs_review?

[jdemeyer]

comment:30

Replying to @vit-tucek:

Thanks. So just to be clear, the plan is to wait for 5.10 beta4 and then fix all the docstrings or possible bugs and turn the status of this patch to needs_review?

Yes, exactly.

[nthiery]

comment:31

Replying to @jdemeyer:

With #4327:

     ...
     Text '$\left(-1\right)\alpha_{1} + \left(-1\right)\alpha_{2}$' at the point (-1.05,-1.05)]
Got:
     Text '$-\alpha_{1} - \alpha_{2}$' at the point (-1.05,-1.05)]

Sorry for the little conflict, and thank you for improving the output of root system plots :-)

[jdemeyer]

comment:32

sage-5.10.beta4 has been released, you should rebase this patch.

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

final version tested on top of 5.10-beta4

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

comment:33

Attachment: trac_13735_fix_repr_lincomb.patch.gz

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

Changed author from Christian Stump, Vít Tuček to Vít Tuček

[jdemeyer]

comment:35

Andrey, can you review this again please?

[novoselt]

comment:36

The patch does not apply for me on 5.10-beta4 because of this hunk:

--- root_lattice_realizations.py
+++ root_lattice_realizations.py
@@ -1845,11 +1845,11 @@
                  Arrow from (0.0,0.0) to (1.0,1.0),
                  Text '$\alpha_{1} + \alpha_{2}$' at the point (1.05,1.05),
                  Arrow from (0.0,0.0) to (-1.0,0.0),
-                 Text '$\left(-1\right)\alpha_{1}$' at the point (-1.05,0.0),
+                 Text '$-\alpha_{1}$' at the point (-1.05,0.0),
                  Arrow from (0.0,0.0) to (0.0,-1.0),
-                 Text '$\left(-1\right)\alpha_{2}$' at the point (0.0,-1.05),
+                 Text '$-\alpha_{2}$' at the point (0.0,-1.05),
                  Arrow from (0.0,0.0) to (-1.0,-1.0),
-                 Text '$\left(-1\right)\alpha_{1} + \left(-1\right)\alpha_{2}$' at the point (-1.05,-1.05)]
+                 Text '$-\alpha_{1} - \alpha_{2}$' at the point (-1.05,-1.05)]
             """
             plot_options = self.plot_parse_options(**options)
             root_lattice = self.root_system.root_lattice()
sage/combinat/root_system/root_lattice_realizations.py.rej (END)

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

comment:37

I've just tried it on a fresh beta4 install and it applied cleanly.

[novoselt]

comment:38

Did you download it from trac? Maybe something got wrong while you were uploading it here?

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

comment:39

Yes, I downloaded it from trac.

[novoselt]

comment:40

I am still getting an error applying on beta4 without anything else. Can it be some caching issue that I am not getting the newest version of the patch? Does the hunk above seem to be correct at least?

[fe60a72e-b3d0-4317-856b-4536ec0e4b8d]

comment:41

I have no idea what might be the issue here. I've checked https://github.com/sagemath/sage/issues/4327 and the patch there seems to agree with mine. I've downloaded & built another beta4 and applied it directly (i.e. patch -p1 < trac_13735_fix_repr_lincomb.patch) and it went without any problems.

[novoselt]

comment:42

OK, my bad, sorry - I've messed up with symbolic links while switching to beta4, everything applies fine now, running tests!

[jdemeyer]

Merged: sage-5.10.beta5