Opened 10 years ago

Closed 3 weeks ago

#13433 closed defect (invalid)

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: Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #14685 Stopgaps:

Status badges

Description (last modified by Florent 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 Mike Hansen 9 years ago.
trac_13433-lazy_power_serie_gen_fix-fh.patch (36.8 KB) - added by Mike Hansen 9 years ago.
trac_13433-base_patch-darijs_mod.patch (39.6 KB) - added by Darij Grinberg 9 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 Grinberg 9 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 10 years ago by Florent Hivert

Cc: Simon King 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 Florent Hivert

Description: modified (diff)

comment:3 Changed 10 years ago by Florent Hivert

Status: newneeds_review

comment:4 Changed 10 years ago by Florent Hivert

Cc: Frédéric Chapoton added
Keywords: Species added

comment:5 Changed 10 years ago by Nicolas M. Thiéry

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 9 years ago by Florent Hivert

Fixed !

Florent

comment:7 Changed 9 years ago by Darij Grinberg

Cc: Darij Grinberg added

comment:8 Changed 9 years ago by Florent Hivert

Keywords: days49 added

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

Florent

comment:9 Changed 9 years ago by Darij Grinberg

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 Nicolas M. Thiéry

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 Darij Grinberg

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 9 years ago by Darij Grinberg (previous) (diff)

Changed 9 years ago by Mike Hansen

Attachment: trac_13433-review-mh.patch added

comment:12 Changed 9 years ago by Mike Hansen

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 Mike Hansen

Dependencies: #14685

Changed 9 years ago by Mike Hansen

comment:14 Changed 9 years ago by Mike Hansen

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 Mike Hansen

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

Changed 9 years ago by Darij Grinberg

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 Darij Grinberg

For patchbot:

apply

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

Changed 9 years ago by Darij Grinberg

comment:17 Changed 9 years ago by Darij Grinberg

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.

For patchbot:

apply

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

Version 1, edited 9 years ago by Darij Grinberg (previous) (next) (diff)

comment:18 Changed 9 years ago by Darij Grinberg

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

comment:19 Changed 9 years ago by Darij Grinberg

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 Darij Grinberg

Status: needs_reviewneeds_work

comment:21 Changed 9 years ago by Jeroen Demeyer

Milestone: sage-5.11sage-5.12

comment:22 Changed 9 years ago by Mike Hansen

A fix is in #15673

comment:23 Changed 9 years ago by For batch modifications

Milestone: sage-6.1sage-6.2

comment:24 Changed 9 years ago by For batch modifications

Milestone: sage-6.2sage-6.3

comment:25 Changed 8 years ago by For batch modifications

Milestone: sage-6.3sage-6.4

comment:26 Changed 2 months ago by Martin Rubey

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

This is no longer relevant, since #32367.

comment:27 Changed 6 weeks ago by Travis Scrimshaw

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

comment:28 Changed 3 weeks ago by Matthias Köppe

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