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 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 7 years ago.
trac_13433-lazy_power_serie_gen_fix-fh.patch (36.8 KB) - added by mhansen 7 years ago.
trac_13433-base_patch-darijs_mod.patch (39.6 KB) - added by darij 7 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 7 years ago.

Download all attachments as: .zip

Change History (29)

comment:1 Changed 8 years ago by hivert

  • 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

The patch is not yet ready for review !

comment:2 Changed 8 years ago by hivert

  • Description modified (diff)

comment:3 Changed 8 years ago by hivert

  • Status changed from new to needs_review

comment:4 Changed 8 years ago by hivert

  • Cc chapoton added
  • Keywords Species added

comment:5 Changed 8 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 7 years ago by hivert

Fixed !

Florent

comment:7 Changed 7 years ago by darij

  • Cc darij added

comment:8 Changed 7 years ago by hivert

  • Keywords days49 added

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

Florent

comment:9 Changed 7 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 7 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 7 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 7 years ago by darij (previous) (diff)

Changed 7 years ago by mhansen

comment:12 Changed 7 years ago by mhansen

  • 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 mhansen

  • Dependencies set to #14685

Changed 7 years ago by mhansen

comment:14 Changed 7 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 7 years ago by mhansen

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

Changed 7 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 7 years ago by darij

For patchbot:

apply

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

Changed 7 years ago by darij

comment:17 Changed 7 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 7 years ago by darij (previous) (diff)

comment:18 Changed 7 years ago by darij

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

comment:19 Changed 7 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 7 years ago by darij

  • Status changed from needs_review to needs_work

comment:21 Changed 7 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:22 Changed 7 years ago by mhansen

A fix is in #15673

comment:23 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:24 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:25 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4
Note: See TracTickets for help on using tickets.