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:

Status badges

Description (last modified by hivert)

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)

trac_13433-review-mh.patch (1.2 KB) - added by mhansen 10 years ago.
trac_13433-lazy_power_serie_gen_fix-fh.patch (36.8 KB) - added by mhansen 10 years ago.
trac_13433-base_patch-darijs_mod.patch (39.6 KB) - added by darij 10 years ago.
Alternative version of the BASE patch, which applies on my system. The review patch needs no changes, hence is not included.
trac_13433-review-mh-darijs_mod.patch (2.9 KB) - added by darij 10 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 10 years ago by hivert

Cc: SimonKing added
Keywords: categories added
Summary: Fix bad handling of base ring in lazy power seriesLazy power series: fix bad handling of base ring and categorification

The patch is not yet ready for review !

comment:2 Changed 10 years ago by hivert

Description: modified (diff)

comment:3 Changed 10 years ago by hivert

Status: newneeds_review

comment:4 Changed 10 years ago by hivert

Cc: chapoton added
Keywords: Species added

comment:5 Changed 10 years ago by nthiery

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 10 years ago by hivert

Fixed !

Florent

comment:7 Changed 10 years ago by darij

Cc: darij added

comment:8 Changed 10 years ago by hivert

Keywords: days49 added

Fixed a multiline doctest to silence the patchbot. Ready for review.

Florent

comment:9 Changed 10 years ago by darij

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 nthiery

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 darij

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...

Last edited 10 years ago by darij (previous) (diff)

Changed 10 years ago by mhansen

Attachment: trac_13433-review-mh.patch added

comment:12 Changed 10 years ago by mhansen

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 mhansen

Dependencies: #14685

Changed 10 years ago by mhansen

comment:14 Changed 10 years ago by mhansen

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 mhansen

Apply trac_13433-lazy_power_serie_gen_fix-fh.patch trac_13433-review-mh.patch

Changed 10 years ago by darij

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 darij

For patchbot:

apply

Apply trac_13433-base_patch-darijs_mod.patch​ trac_13433-review-mh.patch

Changed 10 years ago by darij

comment:17 Changed 10 years ago by darij

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

Last edited 10 years ago by darij (previous) (diff)

comment:18 Changed 10 years ago by darij

This patch breaks subtraction of lazy power series. I'm going to investigaate.

comment:19 Changed 10 years ago by darij

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 darij

Status: needs_reviewneeds_work

comment:21 Changed 9 years ago by jdemeyer

Milestone: sage-5.11sage-5.12

comment:22 Changed 9 years ago by mhansen

A fix is in #15673

comment:23 Changed 9 years ago by vbraun_spam

Milestone: sage-6.1sage-6.2

comment:24 Changed 9 years ago by vbraun_spam

Milestone: sage-6.2sage-6.3

comment:25 Changed 8 years ago by vbraun_spam

Milestone: sage-6.3sage-6.4

comment:26 Changed 5 months ago by mantepse

Milestone: sage-6.4sage-duplicate/invalid/wontfix
Status: needs_workneeds_review

This is no longer relevant, since #32367.

comment:27 Changed 4 months ago by tscrim

Authors: Florent Hivert
Reviewers: Mike Hansen, Nicolas ThieryTravis Scrimshaw
Status: needs_reviewpositive_review

comment:28 Changed 3 months ago by mkoeppe

Resolution: invalid
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.