Opened 9 years ago

Last modified 9 years ago

#13735 closed defect

Fixing a bug in sage.misc.misc.coeff_repr — at Version 20

Reported by: stumpc5 Owned by: sage-combinat
Priority: major Milestone: sage-5.10
Component: combinatorics Keywords: coefficient repr
Cc: chapoton Merged in:
Authors: Christian Stump, Vít Tuček Reviewers:
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by novoselt)

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

Change History (23)

comment:1 follow-up: Changed 9 years ago by stumpc5

  • Status changed from new to needs_review

comment:2 in reply to: ↑ 1 Changed 9 years ago by stumpc5

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.

Changed 9 years ago by stumpc5

comment:3 Changed 9 years ago by stumpc5

apply trac_13735-fix_bug_in_coef_repr-cs.patch

comment:4 follow-up: Changed 9 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 9 years ago by stumpc5

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

comment:6 Changed 9 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 9 years ago by stumpc5

  • Status changed from needs_review to needs_work

comment:8 Changed 9 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 9 years ago by vittucek

fix the representation of linear combinations and include more testcases

comment:9 follow-up: Changed 9 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 9 years ago by stumpc5

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

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 9 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 9 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 9 years ago by vittucek

Sure. That's a trivial change.

comment:14 Changed 9 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 9 years ago by vittucek (previous) (diff)

comment:15 Changed 9 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

Changed 9 years ago by vittucek

comment:16 Changed 9 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 9 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
    resolvelinks() {
                   ^
SyntaxError: invalid syntax

Thus my patch is still valid.

comment:18 Changed 9 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 9 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 9 years ago by novoselt

  • Description modified (diff)
Note: See TracTickets for help on using tickets.