Opened 10 years ago
Closed 3 months ago
#13433 closed defect (invalid)
Lazy power series: fix bad handling of base ring and categorification
Reported by: | hivert | Owned by: | hivert |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | combinatorics | Keywords: | LazyPowerSeries, categories, Species, days49 |
Cc: | sage-combinat, SimonKing, chapoton, darij | Merged in: | |
Authors: | Reviewers: | Travis Scrimshaw | |
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 (32)
comment:1 Changed 10 years ago by
Cc: | SimonKing 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 10 years ago by
Description: | modified (diff) |
---|
comment:3 Changed 10 years ago by
Status: | new → needs_review |
---|
comment:4 Changed 10 years ago by
Cc: | chapoton added |
---|---|
Keywords: | Species added |
comment:5 Changed 10 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 10 years ago by
Cc: | darij added |
---|
comment:8 Changed 10 years ago by
Keywords: | days49 added |
---|
Fixed a multiline doctest to silence the patchbot. Ready for review.
Florent
comment:9 Changed 10 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 10 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 10 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 10 years ago by
Attachment: | trac_13433-review-mh.patch added |
---|
comment:12 Changed 10 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 10 years ago by
Dependencies: | → #14685 |
---|
Changed 10 years ago by
Attachment: | trac_13433-lazy_power_serie_gen_fix-fh.patch added |
---|
comment:14 Changed 10 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 10 years ago by
Apply trac_13433-lazy_power_serie_gen_fix-fh.patch trac_13433-review-mh.patch
Changed 10 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 10 years ago by
For patchbot:
apply
Apply trac_13433-base_patch-darijs_mod.patch trac_13433-review-mh.patch
Changed 10 years ago by
Attachment: | trac_13433-review-mh-darijs_mod.patch added |
---|
comment:17 Changed 10 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 10 years ago by
This patch breaks subtraction of lazy power series. I'm going to investigaate.
comment:19 Changed 10 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 10 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 9 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 5 months ago by
Milestone: | sage-6.4 → sage-duplicate/invalid/wontfix |
---|---|
Status: | needs_work → needs_review |
This is no longer relevant, since #32367.
comment:27 Changed 4 months ago by
Authors: | Florent Hivert |
---|---|
Reviewers: | Mike Hansen, Nicolas Thiery → Travis Scrimshaw |
Status: | needs_review → positive_review |
comment:28 Changed 3 months ago by
Resolution: | → invalid |
---|---|
Status: | positive_review → closed |
The patch is not yet ready for review !