Opened 6 years ago
Closed 6 years ago
#13735 closed defect (fixed)
Fixing a bug in sage.misc.misc.coeff_repr
Reported by: | stumpc5 | Owned by: | sage-combinat |
---|---|---|---|
Priority: | major | Milestone: | sage-5.10 |
Component: | combinatorics | Keywords: | coefficient repr |
Cc: | chapoton | Merged in: | sage-5.10.beta5 |
Authors: | Vít Tuček | Reviewers: | Andrey Novoseltsev |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | sage-5.10.beta4 | 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
Attachments (4)
Change History (48)
comment:1 follow-up: ↓ 2 Changed 6 years ago by
- Status changed from new to needs_review
comment:2 in reply to: ↑ 1 Changed 6 years ago by
Changed 6 years ago by
comment:3 Changed 6 years ago by
apply trac_13735-fix_bug_in_coef_repr-cs.patch
comment:4 follow-up: ↓ 5 Changed 6 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 6 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 6 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 6 years ago by
- Status changed from needs_review to needs_work
comment:8 Changed 6 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 6 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 6 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 6 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 6 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 6 years ago by
Sure. That's a trivial change.
comment:14 Changed 6 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 6 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 6 years ago by
comment:16 Changed 6 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 6 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 6 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 6 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 6 years ago by
- Description modified (diff)
comment:21 Changed 6 years ago by
- Reviewers set to Andrey Novoseltsev
- Status changed from needs_review to positive_review
comment:22 Changed 6 years ago by
- 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
- 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
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
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
- 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
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
Replying to vittucek:
Can I somehow get notified when sage reaches beta4?
I will personally notify you.
comment:29 follow-up: ↓ 30 Changed 6 years ago by
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
Replying to 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?
Yes, exactly.
comment:31 in reply to: ↑ 23 Changed 6 years ago by
comment:32 Changed 6 years ago by
sage-5.10.beta4 has been released, you should rebase this patch.
comment:33 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:34 Changed 6 years ago by
comment:35 Changed 6 years ago by
Andrey, can you review this again please?
comment:36 Changed 6 years ago by
- 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
- Status changed from needs_work to needs_info
I've just tried it on a fresh beta4 install and it applied cleanly.
comment:38 Changed 6 years ago by
Did you download it from trac? Maybe something got wrong while you were uploading it here?
comment:39 Changed 6 years ago by
Yes, I downloaded it from trac.
comment:40 Changed 6 years ago by
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
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
- 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
- Status changed from needs_review to positive_review
comment:44 Changed 6 years ago by
- Merged in set to sage-5.10.beta5
- Resolution set to fixed
- Status changed from positive_review to closed
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.