Opened 8 years ago
Last modified 6 years ago
#13433 needs_work defect
Lazy power series: fix bad handling of base ring and categorification
Reported by: | hivert | Owned by: | hivert |
---|---|---|---|
Priority: | major | Milestone: | sage-6.4 |
Component: | combinatorics | Keywords: | LazyPowerSeries, categories, Species, days49 |
Cc: | sage-combinat, SimonKing, chapoton, darij | 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 (29)
comment:1 Changed 8 years ago by
- Cc SimonKing added
- Keywords categories added
- Summary changed from Fix bad handling of base ring in lazy power series to Lazy power series: fix bad handling of base ring and categorification
comment:2 Changed 8 years ago by
- Description modified (diff)
comment:3 Changed 8 years ago by
- Status changed from new to needs_review
comment:4 Changed 8 years ago by
- Cc chapoton added
- Keywords Species added
comment:5 Changed 8 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:6 Changed 7 years ago by
Fixed !
Florent
comment:7 Changed 7 years ago by
- Cc darij added
comment:8 Changed 7 years ago by
- Keywords days49 added
Fixed a multiline doctest to silence the patchbot. Ready for review.
Florent
comment:9 Changed 7 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 7 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 7 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 7 years ago by
comment:12 Changed 7 years ago by
- Reviewers set to 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 7 years ago by
- Dependencies set to #14685
Changed 7 years ago by
comment:14 Changed 7 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 7 years ago by
Apply trac_13433-lazy_power_serie_gen_fix-fh.patch trac_13433-review-mh.patch
Changed 7 years ago by
Alternative version of the BASE patch, which applies on my system. The review patch needs no changes, hence is not included.
comment:16 Changed 7 years ago by
For patchbot:
apply
Apply trac_13433-base_patch-darijs_mod.patch trac_13433-review-mh.patch
Changed 7 years ago by
comment:17 Changed 7 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 7 years ago by
This patch breaks subtraction of lazy power series. I'm going to investigaate.
comment:19 Changed 7 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 7 years ago by
- Status changed from needs_review to needs_work
comment:21 Changed 7 years ago by
- Milestone changed from sage-5.11 to sage-5.12
comment:22 Changed 7 years ago by
A fix is in #15673
comment:23 Changed 7 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:24 Changed 7 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:25 Changed 6 years ago by
- Milestone changed from sage-6.3 to sage-6.4
The patch is not yet ready for review !