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: |
Description (last modified by )
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
Change History (23)
comment:1 follow-up: ↓ 2 Changed 9 years ago by
- Status changed from new to needs_review
comment:2 in reply to: ↑ 1 Changed 9 years ago by
Changed 9 years ago by
comment:3 Changed 9 years ago by
apply trac_13735-fix_bug_in_coef_repr-cs.patch
comment:4 follow-up: ↓ 5 Changed 9 years ago by
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
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
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
- Status changed from needs_review to needs_work
comment:8 Changed 9 years ago by
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...
comment:9 follow-up: ↓ 10 Changed 9 years ago by
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
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
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
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
Sure. That's a trivial change.
comment:14 Changed 9 years ago by
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?
comment:15 Changed 9 years ago by
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
comment:16 Changed 9 years ago by
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
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
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
- 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
- Description modified (diff)
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.