Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lazy power series: fix bad handling of base ring and categorification #13433

Closed
hivert opened this issue Sep 6, 2012 · 26 comments
Closed

Lazy power series: fix bad handling of base ring and categorification #13433

hivert opened this issue Sep 6, 2012 · 26 comments

Comments

@hivert
Copy link
Contributor

hivert commented Sep 6, 2012

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)

Depends on #14685

CC: @sagetrac-sage-combinat @simon-king-jena @fchapoton @darijgr

Component: combinatorics

Keywords: LazyPowerSeries, Species, days49

Reviewer: Travis Scrimshaw

Issue created by migration from https://trac.sagemath.org/ticket/13433

@hivert hivert added this to the sage-5.11 milestone Sep 6, 2012
@hivert hivert self-assigned this Sep 6, 2012
@hivert
Copy link
Contributor Author

hivert commented Sep 6, 2012

comment:1

The patch is not yet ready for review !

@hivert hivert changed the title Fix bad handling of base ring in lazy power series Lazy power series: fix bad handling of base ring and categorification Sep 6, 2012
@hivert

This comment has been minimized.

@hivert
Copy link
Contributor Author

hivert commented Apr 25, 2013

Changed keywords from LazyPowerSeries to LazyPowerSeries, Species

@nthiery
Copy link
Contributor

nthiery commented May 8, 2013

comment:5

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

@hivert
Copy link
Contributor Author

hivert commented Jun 17, 2013

comment:6

Fixed !

Florent

@hivert
Copy link
Contributor Author

hivert commented Jun 17, 2013

comment:8

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

Florent

@hivert
Copy link
Contributor Author

hivert commented Jun 17, 2013

Changed keywords from LazyPowerSeries, Species to LazyPowerSeries, Species, days49

@darijgr
Copy link
Contributor

darijgr commented Jun 17, 2013

comment:9

Any chance to get docstrings on _sum_generator_gen and/or sum_generator? Andrew and me are wondering what precisely these functions do in #14542 . Thank you!

@nthiery
Copy link
Contributor

nthiery commented Jun 18, 2013

comment:10

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.

@darijgr
Copy link
Contributor

darijgr commented Jul 16, 2013

comment:11

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

@mwhansen
Copy link
Contributor

Attachment: trac_13433-review-mh.patch.gz

@mwhansen
Copy link
Contributor

comment:12

I've attached a review patch which takes care of these issues if someone would take a look at it.

@mwhansen
Copy link
Contributor

Reviewer: Mike Hansen, Nicolas Thiery

@mwhansen
Copy link
Contributor

Dependencies: #14685

@mwhansen
Copy link
Contributor

comment:14

Attachment: trac_13433-lazy_power_serie_gen_fix-fh.patch.gz

I've attached a new patch which will apply on top of #14685. I've pushed these to the queue.

@mwhansen
Copy link
Contributor

comment:15

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

@darijgr
Copy link
Contributor

darijgr commented Jul 29, 2013

Attachment: trac_13433-base_patch-darijs_mod.patch.gz

Alternative version of the BASE patch, which applies on my system. The review patch needs no changes, hence is not included.

@darijgr
Copy link
Contributor

darijgr commented Jul 29, 2013

comment:16

For patchbot:

apply

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

@darijgr
Copy link
Contributor

darijgr commented Jul 29, 2013

Attachment: trac_13433-review-mh-darijs_mod.patch.gz

@darijgr
Copy link
Contributor

darijgr commented Jul 29, 2013

comment:17

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

@darijgr
Copy link
Contributor

darijgr commented Jul 29, 2013

comment:18

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

@darijgr
Copy link
Contributor

darijgr commented Jul 29, 2013

comment:19

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

@mwhansen
Copy link
Contributor

comment:22

A fix is in #15673

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.1, sage-6.2 Jan 30, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@mantepse
Copy link
Contributor

comment:26

This is no longer relevant, since #32367.

@mantepse mantepse removed this from the sage-6.4 milestone Sep 21, 2022
@tscrim
Copy link
Collaborator

tscrim commented Oct 22, 2022

Changed reviewer from Mike Hansen, Nicolas Thiery to Travis Scrimshaw

@tscrim
Copy link
Collaborator

tscrim commented Oct 22, 2022

Changed author from Florent Hivert to none

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants