Opened 10 years ago
Last modified 3 days ago
#13433 needs_review defect
Lazy power series: fix bad handling of base ring and categorification
Reported by: | Florent Hivert | Owned by: | Florent Hivert |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | combinatorics | Keywords: | LazyPowerSeries, categories, Species, days49 |
Cc: | Sage Combinat CC user, Simon King, Frédéric Chapoton, Darij Grinberg | Merged in: | |
Authors: | Florent Hivert | Reviewers: | Mike Hansen, Nicolas Thiery |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #14685 | Stopgaps: |
Description (last modified by )
Currently
sage: R = LazyPowerSeriesRing(QQ) sage: type(R.gen().coefficient(0)) <type 'sage.rings.rational.Rational'> sage: type(R.gen().coefficient(1)) <type 'int'> sage: type(R.gen().coefficient(2)) <type 'int'>
it should be always Rational
I also fixed the following bug:
sage: from sage.combinat.species.series import LazyPowerSeriesRing sage: L = LazyPowerSeriesRing(QQ) sage: g = L.gen(); z = L.zero() sage: s = z+g; s Uninitialized lazy power series sage: s.coefficients(10) [0, 0, 0, 0, 0, 0, 0, 0, 0, 0] sage: s Uninitialized lazy power series
It should be:
sage: s.coefficients(10) [0, 1, 0, 0, 0, 0, 0, 0, 0, 0] sage: s x + O(x^10)
Attachments (4)
Change History (30)
comment:1 Changed 10 years ago by
Cc: | Simon King added |
---|---|
Keywords: | categories added |
Summary: | Fix bad handling of base ring in lazy power series → Lazy power series: fix bad handling of base ring and categorification |
comment:2 Changed 9 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 9 years ago by
Status: | new → needs_review |
---|
comment:4 Changed 9 years ago by
Cc: | Frédéric Chapoton added |
---|---|
Keywords: | Species added |
comment:5 Changed 9 years ago by
Salut Florent,
There are some failing tests:
sage -t sage/combinat/tutorial.py # 5 doctests failed sage -t sage/tests/french_book/polynomes.py # 7 doctests failed
Could you fix those or move the patch down the queue?
Thanks,
Nicolas
comment:7 Changed 9 years ago by
Cc: | Darij Grinberg added |
---|
comment:8 Changed 9 years ago by
Keywords: | days49 added |
---|
Fixed a multiline doctest to silence the patchbot. Ready for review.
Florent
comment:9 Changed 9 years ago by
Any chance to get docstrings on _sum_generator_gen and/or sum_generator? Andrew and me are wondering what precisely these functions do in http://trac.sagemath.org/sage_trac/ticket/14542 . Thank you!
comment:10 Changed 9 years ago by
I have just been through the path, and it looks good to go.
Just one thing: using the new syntax for example continuations. And since the whole file is touched anyway, we might as well do this change everywhere.
comment:11 Changed 9 years ago by
There is a doctest failing, see http://patchbot.sagemath.org/log/13433/Fedora/18/x86_64/3.9.5-201.fc18.x86_64/desktop/2013-07-13%2021:47:59%20+0100?short . One way to fix it is to replace the _div_
method of the CycleIndexSeries
class by a __div__
method, since I don't think that class belongs to any category which has a division method. On the other hand, this might not be what you want.
Ceterum censeo, _sum_generator_gen
and sum_generator
still need to be documented...
Changed 9 years ago by
Attachment: | trac_13433-review-mh.patch added |
---|
comment:12 Changed 9 years ago by
Reviewers: | → Mike Hansen, Nicolas Thiery |
---|
I've attached a review patch which takes care of these issues if someone would take a look at it.
comment:13 Changed 9 years ago by
Dependencies: | → #14685 |
---|
Changed 9 years ago by
Attachment: | trac_13433-lazy_power_serie_gen_fix-fh.patch added |
---|
comment:14 Changed 9 years ago by
I've attached a new patch which will apply on top of #14685. I've pushed these to the queue.
comment:15 Changed 9 years ago by
Apply trac_13433-lazy_power_serie_gen_fix-fh.patch trac_13433-review-mh.patch
Changed 9 years ago by
Attachment: | trac_13433-base_patch-darijs_mod.patch added |
---|
Alternative version of the BASE patch, which applies on my system. The review patch needs no changes, hence is not included.
comment:16 Changed 9 years ago by
For patchbot:
apply
Apply trac_13433-base_patch-darijs_mod.patch trac_13433-review-mh.patch
Changed 9 years ago by
Attachment: | trac_13433-review-mh-darijs_mod.patch added |
---|
comment:17 Changed 9 years ago by
Mike, thank you for the docstring! I've understood it now. I've just attached my version of your review patch, which adds another example hopefully clarifying how to use sum_generator *in practice*. The example is a bit artificial because right now lazy power series can neither be divided nor subtracted(!!). I've also fixed another typo.
I must say I'm a bit surprised to see division (__div__
) defined in generating_series.py
but not in series.py
. I am also surprised by the lack of _neg_
(or __neg__
?) in both files, although one can use (-1) * u of course. Finally, subtraction of lazy power series does not seem to work (it ends in RuntimeError: maximum recursion depth exceeded in __instancecheck__
).
For patchbot:
apply
Apply trac_13433-base_patch-darijs_mod.patch trac_13433-review-mh-darijs_mod.patch
comment:18 Changed 9 years ago by
This patch breaks subtraction of lazy power series. I'm going to investigaate.
comment:19 Changed 9 years ago by
OK, I have no idea what is broken and how to fix it, but it's clear that something is going wrong. All I know is that the bug is in the base file (trac_13433-base_patch-darijs_mod.patch, or previously trac_13433-lazy_power_serie_gen_fix-fh.patch).
comment:20 Changed 9 years ago by
Status: | needs_review → needs_work |
---|
comment:21 Changed 9 years ago by
Milestone: | sage-5.11 → sage-5.12 |
---|
comment:23 Changed 9 years ago by
Milestone: | sage-6.1 → sage-6.2 |
---|
comment:24 Changed 8 years ago by
Milestone: | sage-6.2 → sage-6.3 |
---|
comment:25 Changed 8 years ago by
Milestone: | sage-6.3 → sage-6.4 |
---|
comment:26 Changed 3 days ago by
Milestone: | sage-6.4 → sage-duplicate/invalid/wontfix |
---|---|
Status: | needs_work → needs_review |
This is no longer relevant, since #32367.
The patch is not yet ready for review !