Opened 6 years ago

Closed 6 years ago

Fixing a bug in sage.misc.misc.coeff_repr

Reported by: Owned by: stumpc5 sage-combinat major sage-5.10 combinatorics coefficient repr chapoton sage-5.10.beta5 Vít Tuček Andrey Novoseltsev N/A sage-5.10.beta4

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


comment:1 follow-up: ↓ 2 Changed 6 years ago by stumpc5

• Status changed from new to needs_review

comment:2 in reply to: ↑ 1 Changed 6 years ago by 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.

comment:3 Changed 6 years ago by stumpc5

apply trac_13735-fix_bug_in_coef_repr-cs.patch

comment:4 follow-up: ↓ 5 Changed 6 years ago by novoselt

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

comment:5 in reply to: ↑ 4 Changed 6 years ago by stumpc5

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...

comment:6 Changed 6 years ago by stumpc5

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

comment:7 Changed 6 years ago by stumpc5

• Status changed from needs_review to needs_work

comment:8 Changed 6 years ago by novoselt

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...

Changed 6 years ago by vittucek

fix the representation of linear combinations and include more testcases

comment:9 follow-up: ↓ 10 Changed 6 years ago by vittucek

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.

comment:10 in reply to: ↑ 9 Changed 6 years ago by stumpc5

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

comment:11 Changed 6 years ago by vittucek

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

comment:12 Changed 6 years ago by novoselt

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

comment:13 Changed 6 years ago by vittucek

Sure. That's a trivial change.

comment:14 Changed 6 years ago by vittucek

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?

Last edited 6 years ago by vittucek (previous) (diff)

comment:15 Changed 6 years ago by vittucek

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


comment:16 Changed 6 years ago by vittucek

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.

comment:17 Changed 6 years ago by vittucek

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
^
SyntaxError: invalid syntax



Thus my patch is still valid.

comment:18 Changed 6 years ago by novoselt

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"!

comment:19 Changed 6 years ago by vittucek

• Authors changed from Christian Stump to Christian Stump, Vít Tuček
• Status changed from needs_work to needs_review

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

comment:20 Changed 6 years ago by novoselt

• Description modified (diff)

comment:21 Changed 6 years ago by novoselt

• Reviewers set to Andrey Novoseltsev
• Status changed from needs_review to positive_review

comment:22 Changed 6 years ago by jdemeyer

• Status changed from positive_review to needs_work

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]
**********************************************************************


comment:23 follow-up: ↓ 31 Changed 6 years ago by jdemeyer

• Dependencies set to #4327

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)]
**********************************************************************


comment:24 Changed 6 years ago by vittucek

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?

comment:25 Changed 6 years ago by vittucek

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.

comment:26 Changed 6 years ago by jdemeyer

• Dependencies changed from #4327 to sage-5.10.beta4

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.

comment:27 follow-up: ↓ 28 Changed 6 years ago by vittucek

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

comment:28 in reply to: ↑ 27 Changed 6 years ago by jdemeyer

Can I somehow get notified when sage reaches beta4?

I will personally notify you.

comment:29 follow-up: ↓ 30 Changed 6 years ago by vittucek

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?

comment:30 in reply to: ↑ 29 Changed 6 years ago by jdemeyer

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.

comment:31 in reply to: ↑ 23 Changed 6 years ago by nthiery

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

comment:32 Changed 6 years ago by jdemeyer

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

Changed 6 years ago by vittucek

final version tested on top of 5.10-beta4

comment:33 Changed 6 years ago by vittucek

• Status changed from needs_work to needs_review

comment:34 Changed 6 years ago by vittucek

• Authors changed from Christian Stump, Vít Tuček to Vít Tuček

comment:35 Changed 6 years ago by jdemeyer

Andrey, can you review this again please?

comment:36 Changed 6 years ago by novoselt

• Status changed from needs_review to needs_work

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)


comment:37 Changed 6 years ago by vittucek

• Status changed from needs_work to needs_info

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

comment:40 Changed 6 years ago by novoselt

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?

comment:41 Changed 6 years ago by vittucek

I have no idea what might be the issue here. I've checked http://trac.sagemath.org/sage_trac/ticket/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.

comment:42 Changed 6 years ago by novoselt

• Status changed from needs_info to needs_review

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

comment:43 Changed 6 years ago by novoselt

• Status changed from needs_review to positive_review

comment:44 Changed 6 years ago by jdemeyer

• Merged in set to sage-5.10.beta5
• Resolution set to fixed
• Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.