Opened 16 months ago
Closed 2 months ago
#32367 closed enhancement (fixed)
Replace Lazy Power Series in species directory
Reported by:  Tejasvi Chebrolu  Owned by:  

Priority:  major  Milestone:  sage9.8 
Component:  combinatorics  Keywords:  LazyPowerSeries, FormalSeries, gsoc2021 
Cc:  Martin Rubey, Travis Scrimshaw, Paul, Zimmermann, Thierry Monteil  Merged in:  
Authors:  Tejasvi Chebrolu, Martin Rubey, Travis Scrimshaw  Reviewers:  Martin Rubey, Travis Scrimshaw 
Report Upstream:  N/A  Work issues:  
Branch:  4fc981b (Commits, GitHub, GitLab)  Commit:  4fc981b458f8d628684decf221618911ace06333 
Dependencies:  #34423  Stopgaps: 
Description (last modified by )
In this ticket we finalize the implementation of lazy series, and replace the old lazy series framework.
Change History (132)
comment:1 Changed 16 months ago by
Branch:  → u/ghtejasvicsr1/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series 

comment:2 Changed 16 months ago by
Commit:  → 0d56b0227713d7a6b26612593b311071a05782b8 

comment:3 Changed 16 months ago by
Keywords:  gsoc2021 added; GSoC21 removed 

comment:4 Changed 16 months ago by
Dependencies:  → #32324 

comment:5 Changed 16 months ago by
Commit:  0d56b0227713d7a6b26612593b311071a05782b8 → 39993e3ac56e0e26b69dd3a889c2aa8c030558fb 

Branch pushed to git repo; I updated commit sha1. New commits:
39993e3  Fixed the initialising issue.

comment:6 Changed 16 months ago by
Commit:  39993e3ac56e0e26b69dd3a889c2aa8c030558fb → db206803e6c2847bb01600f1ba63ba9539894408 

comment:7 Changed 16 months ago by
Commit:  db206803e6c2847bb01600f1ba63ba9539894408 → 7d491f6f7deee6cf4755c94442212366a0d96322 

Branch pushed to git repo; I updated commit sha1. New commits:
974fa4f  Changed _list and _term methods.

6cfdf8d  Created restrict method in the series helper function.

eef15d8  Changed the name of iterator to callable.

7d491f6  Raised a not implemented error for all not corrected methods in the generating_series file.

comment:8 Changed 16 months ago by
Branch:  u/ghtejasvicsr1/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series → u/mantepse/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series 

comment:9 Changed 16 months ago by
Commit:  7d491f6f7deee6cf4755c94442212366a0d96322 → a9084c6d28f3ecbfe544133741415731279df1b6 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
661c3d4  backport two fixes from 32324

8a7dcd3  fix oversight in shift

78b8fff  Last trivial fix for clean pyflakes.

96771f0  Merge branch 'u/tscrim/dense_lls31897' of git://trac.sagemath.org/sage into t/32324/lazy_taylor_series

86925af  initial version of lazy symmetric functions

aa8a250  initial and insanely stupid plethysm implementation

1a36f59  add (currently failing) doctest for plethysm

03198a0  fix bug in plethysm

eb3e88f  more doctests for composition and plethysm

a9084c6  Merge branch 'u/mantepse/lazy_taylor_series' of git://trac.sagemath.org/sage into t/32367/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series

comment:10 Changed 16 months ago by
Commit:  a9084c6d28f3ecbfe544133741415731279df1b6 → 4b76e93596f91a5068a5d2dbba31a65f748dea73 

Branch pushed to git repo; I updated commit sha1. New commits:
4b76e93  switch to power sum by default

comment:11 Changed 16 months ago by
Branch:  u/mantepse/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series → u/ghtejasvicsr1/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series 

comment:12 Changed 16 months ago by
Commit:  4b76e93596f91a5068a5d2dbba31a65f748dea73 → 48e8e2686f68a911c5a705b91971e0f3d5924113 

Branch pushed to git repo; I updated commit sha1. New commits:
48e8e26  Changed more methods.

comment:13 Changed 16 months ago by
Commit:  48e8e2686f68a911c5a705b91971e0f3d5924113 → 78a85aeeaeabf44e6a6ebd9c58a486cebf373eca 

Branch pushed to git repo; I updated commit sha1. New commits:
78a85ae  Some more work in generating series.

comment:14 Changed 12 months ago by
Milestone:  sage9.5 → sage9.6 

comment:15 Changed 8 months ago by
Milestone:  sage9.6 → sage9.7 

comment:16 Changed 4 months ago by
Branch:  u/ghtejasvicsr1/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series → u/mantepse/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series 

comment:17 Changed 4 months ago by
Commit:  78a85aeeaeabf44e6a6ebd9c58a486cebf373eca → d4f2e5cc8b49b70138ab1f56fb0062d22c83491c 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
e4479ed  fix missing space

3da6b3d  better docstring

8798139  Adding a slightly more complicated doctest and other small tweaks.

75e3f3b  Merge branch 'public/rings/laurent_quo_rem_bug34330' into public/rings/lazy_talyor_series32324

c74fd71  improve documentation, move options to abstract base class

9d6579b  improve documentation, move zero, one, characteristic, etc. to ABC

feba6b8  Working more on __call__ for LazySymFunc.

3f3e0f2  Merge branch 'public/rings/lazy_talyor_series32324' of https://github.com/sagemath/sagetracmirror into public/rings/lazy_talyor_series32324

6727228  Merge branch 'public/rings/lazy_talyor_series32324' of trac.sagemath.org:sage into t/32324/public/rings/lazy_talyor_series32324

d4f2e5c  Merge branch 'public/rings/lazy_talyor_series32324' of trac.sagemath.org:sage into t/32367/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series

comment:18 Changed 4 months ago by
Commit:  d4f2e5cc8b49b70138ab1f56fb0062d22c83491c → 1b34e62a503050fd50c831d87746f5740a0df92d 

Branch pushed to git repo; I updated commit sha1. New commits:
028796d  Fixing numerous issues with __call__ and expanding its functionality. Moving plethysm to a Stream_plethysm.

9fb155f  Removing unused code from previous version.

1b34e62  Merge branch 'public/rings/lazy_talyor_series32324' of trac.sagemath.org:sage into t/32367/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series

comment:19 Changed 4 months ago by
Commit:  1b34e62a503050fd50c831d87746f5740a0df92d → 5ba18e80f4f99f03393056a33cfeb42b413e9066 

Branch pushed to git repo; I updated commit sha1. New commits:
7f9dbb1  Some last doc fixes and tweaks.

4e03fee  remove unused local variable

e780472  Addressing the linter complaint.

5ba18e8  Merge branch 'public/rings/lazy_talyor_series32324' of trac.sagemath.org:sage into t/32367/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series

comment:21 Changed 4 months ago by
No, not at all, I am working on it.
I am having slight difficulties with plethysm, but I think that I have mostly resolved these by now.
comment:22 Changed 4 months ago by
Commit:  5ba18e80f4f99f03393056a33cfeb42b413e9066 → 1e8f69dfeab7f92a5579fbcb488f6759cedc569f 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
50ce450  derivative for LazyTaylorSeries

10cfc11  derivative for LazySymmetricFunctions

7be0b86  functorial composition for LazySymmetricFunctions

c755d71  implement arithmetic product for LazySymmetricFunction

c36796e  Merge branch 'u/mantepse/implement_arithmetic_product_of_lazy_symmetric_functions' of trac.sagemath.org:sage into t/32367/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series

18429ef  do not use change_ring, because it is brittle, and take care of zero coefficients which are not symmetric functions

481d78d  Merge branch 'u/mantepse/implement_functorial_composition_of_lazy_symmetric_functiosn' of trac.sagemath.org:sage into t/34423/implement_arithmetic_product_of_lazy_symmetric_functions

b469431  do not use change_ring, because it is brittle

56ee69e  Merge branch 'u/mantepse/implement_arithmetic_product_of_lazy_symmetric_functions' of trac.sagemath.org:sage into t/32367/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series

1e8f69d  remove LazyPowerSeries and Stream

comment:23 Changed 3 months ago by
Commit:  1e8f69dfeab7f92a5579fbcb488f6759cedc569f → d75ac136b99030723109a279f29534d9cbedb726 

Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
e1e7f0d  Merge branch 'u/mantepse/implement_derivatives_of_lazy_series' of trac.sagemath.org:sage into t/34473/remove_rings_from_streams

6010361  remove ring argument from Stream_map_coefficient and Stream_function

aa15394  Merge branch 'u/mantepse/remove_rings_from_streams' of trac.sagemath.org:sage into t/34422/implement_functorial_composition_of_lazy_symmetric_functiosn

760372a  address reviewer's comments

8ca3624  polish functorial composition

b2ef1e4  allow SymmetricFunctions as arguments of functorial composition

83e7dd3  Merge branch 'u/mantepse/implement_functorial_composition_of_lazy_symmetric_functiosn' of trac.sagemath.org:sage into t/34423/implement_arithmetic_product_of_lazy_symmetric_functions

d22f87a  adapt code after merge

f2a3caf  reimplement arithmetic_product by using the symmetric function version (which is more robust

d75ac13  Merge branch 'u/mantepse/implement_arithmetic_product_of_lazy_symmetric_functions' of trac.sagemath.org:sage into t/32367/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series

comment:24 Changed 3 months ago by
Commit:  d75ac136b99030723109a279f29534d9cbedb726 → 293b4392f318a6a6a487dc87fa8c8d64864fbf23 

Branch pushed to git repo; I updated commit sha1. New commits:
293b439  adapt doctests

comment:26 Changed 3 months ago by
Dependencies:  #32324 → #34423 

comment:27 Changed 3 months ago by
Authors:  Tejasvi Chebrolu → Tejasvi Chebrolu, Martin Rubey, Travis Scrimshaw 

comment:28 Changed 3 months ago by
From a first quick look, it seems like there are just a few small things to address:
These need at least one doctest each:
def __init__(self, base_ring): super().__init__(base_ring, names="z")
This can be simplified:
 return CIS(lambda n: _cl_term(n)) + return CIS(_cl_term)
We can also use (the cached)
base_ring(1) +base_ring.one()
We no longer have a coefficients()
method (a difference between the implementations), right? We should introduce a deprecation message for this method or add it to the lazy series. I am leaning towards adding it to the lazy series as a redirect of self[:n]
.
comment:29 Changed 3 months ago by
Commit:  293b4392f318a6a6a487dc87fa8c8d64864fbf23 → 89270ebb1e49759628c1db69de93744a0d3b29df 

Branch pushed to git repo; I updated commit sha1. New commits:
8b6a752  no period at end of input description, break long doctest

f88a5c8  fix arithmetic product with 0 and finite support

89270eb  Merge branch 'u/mantepse/implement_arithmetic_product_of_lazy_symmetric_functions' of trac.sagemath.org:sage into t/32367/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series

comment:30 Changed 3 months ago by
There are several failing doctests and a few hidden bugs elsewhere (found using grep LazyPowerSeries?). Trying to fix.
comment:31 Changed 3 months ago by
There is at least one place, where we feed the LazyPowerSeriesRing? with an infinite iterator, see below.
While this is surely easy to change, shouldn't we support this, too? Actually, I thought we did, but I cannot find it right now.
# If we're here, we may not be a finite crystal. # In fact, we're probably infinite. from sage.rings.lazy_series_ring import LazyTaylorSeriesRing if q is None: P = LazyTaylorSeriesRing(ZZ, names='q') else: P = q.parent() if not isinstance(P, LazyTaylorSeriesRing): raise TypeError("the parent of q must be a lazy power series ring") ret = P(iter_by_deg(mg)) return ret
comment:32 Changed 3 months ago by
There is one doctest from a book which we cannot sensibly support anymore, see below.
In short:
sage: L.<z> = LazyPowerSeriesRing(QQ) sage: C = L() sage: C._name = 'C' sage: C.define( z + C * C )
must become
sage: L.<z> = LazyPowerSeriesRing(QQ) sage: C = L.undefined(valuation=1) sage: C.define( z + C * C )
Can I simply change this? (Note that we do not support this recursive definition with valuation=0 anymore.)
Sage example in ./combinat.tex, line 654:: sage: L.<z> = LazyPowerSeriesRing(QQ) Sage example in ./combinat.tex, line 661:: sage: C = L() sage: C._name = 'C' sage: C.define( z + C * C ) Sage example in ./combinat.tex, line 666:: sage: [C.coefficient(i) for i in range(11)] [0, 1, 1, 2, 5, 14, 42, 132, 429, 1430, 4862]
comment:33 Changed 3 months ago by
Similarly, also in src/sage/tests/books/computationalmathematicswithsagemath/
, but polynomes.tex
.
I think we can make exponential
an alias, but compute_coefficients
does not really make sense anymore.
sage: L.<x> = LazyPowerSeriesRing(QQ) sage: lazy_exp = x.exponential(); lazy_exp O(1) Sage example in ./polynomes.tex, line 2039:: sage: lazy_exp[5] 1/120 sage: lazy_exp 1 + x + 1/2*x^2 + 1/6*x^3 + 1/24*x^4 + 1/120*x^5 + O(x^6) Sage example in ./polynomes.tex, line 2062:: sage: f = L(1) # the constant lazy series 1 sage: for i in range(5): ....: f = (x*f).exponential() ....: f.compute_coefficients(5) # forces the computation ....: print(f) # of the first coefficients 1 + x + 1/2*x^2 + 1/6*x^3 + 1/24*x^4 + 1/120*x^5 + O(x^6) 1 + x + 3/2*x^2 + 5/3*x^3 + 41/24*x^4 + 49/30*x^5 + O(x^6) 1 + x + 3/2*x^2 + 8/3*x^3 + 101/24*x^4 + 63/10*x^5 + O(x^6) 1 + x + 3/2*x^2 + 8/3*x^3 + 125/24*x^4 + 49/5*x^5 + O(x^6) 1 + x + 3/2*x^2 + 8/3*x^3 + 125/24*x^4 + 54/5*x^5 + O(x^6)
Finally:
Sage example in ./polynomes.tex, line 2105:: sage: from sage.combinat.species.series import LazyPowerSeries sage: f = LazyPowerSeries(L, name='f') sage: f.define((x*f).exponential()) sage: f.coefficients(8) [1, 1, 3/2, 8/3, 125/24, 54/5, 16807/720, 16384/315]
(L
is the lazy power series ring here)
comment:34 Changed 3 months ago by
Cc:  Paul Zimmermann added 

Status:  needs_review → needs_info 
comment:35 Changed 3 months ago by
I thought we also supported iterators too, but it is possible that we don’t. I don’t quite have any explicit memory of what we did other than have a discussion about how to handle them. It might have fallen through the cracks.
We can make these changes. The standard thing to do is to cc the book author(s) to let them know.
I guess we don’t have a good mechanism for finding out what is the smallest valuation for a define()
series should make sense. I remember us talking about this for Laurent series and not knowing there is a unique solution, much less knowing what lower bound might work. The issue for the power series was we had to try and catch an infinite recursion error (which would be very bad practice because it would hide bugs). I don’t think we talked too much about other solutions. When we have one unknown function, we could take its _approximate_order
to be a variable, e.g., n
and take the ceiling of the smallest positive real root of f(n)  n
, where f
is the function that we get from the define()
input _approximate_order
.
Well, if we don’t want to try anything like this beforehand (or it becomes too much work), then we can simply change the test (and I would say it is better for the user to supply anyways). (That is also a very evil test. :p
)
I think exponential()
is a good alias to have. I would add a deprecation for compute_coefficients()
and remove the line in the above doctest.
comment:36 Changed 3 months ago by
Important and somewhat urgent question:
Support for passing generators turns out to be easy, but there is a question of semantics. If the valuation is specified, should it refer to the first term yielded by the generator, or the first nonzero term yielded by the generator.
Both is easy to achieve.
By way of example: do we want
sage: L.<x> = LazyLaurentSeriesRing(QQ); L(iter(NN), valuation=3) x^3 + 2*x^2 + 3*x^1 + 4 + 5*x + 6*x^2 + O(x^3)
or
sage: L.<x> = LazyLaurentSeriesRing(QQ); L(iter(NN), valuation=3) x^2 + 2*x^1 + 3 + 4*x + 5*x^2 + 6*x^3 + O(x^4)
Compare with our current decisions:
sage: L.<x> = LazyLaurentSeriesRing(QQ); L(range(5), valuation=3) x^3 + 2*x^2 + 3*x^1 + 4 sage: L.<x> = LazyLaurentSeriesRing(QQ); L(lambda n: n+3, valuation=3) x^2 + 2*x^1 + 3 + 4*x + 5*x^2 + 6*x^3 + O(x^4)
comment:37 Changed 3 months ago by
The first nonzero term following our statement that passing the valuation
means the series exactly has that valuation.
We might want to do something as a safety guard of running forever (say 1000 trials) if someone does something crazy like pass
def zero(): while True: yield 0
(This might arise more naturally if someone passes something that is not known to be exactly 0
but ends up being 0
and doing some iteration over that.)
comment:38 Changed 3 months ago by
Commit:  89270ebb1e49759628c1db69de93744a0d3b29df → b76d8f40ac50839297cb25fe23da16c0c0a8c49c 

Branch pushed to git repo; I updated commit sha1. New commits:
b76d8f4  allow to define lazy series using iterators, adapt doctest

comment:39 followup: 44 Changed 3 months ago by
There are a few further minor issues.
I think that for LazyTaylorSeries
, LazyDirichletSeries
, LazySymmetricFunction
, etc. we really don't want
sage: L.<x> = LazyTaylorSeriesRing(QQ) sage: (x^2/(1x)^3)[:5] [1, 3, 6] sage: L(lambda n: n*(n1)/2)[:5] [0, 0, 1, 3, 6]
Instead, I think that for these classes we always want to start from degree 0. The current behaviour also breaks some doctests in the species directory.
I am also hesitant to encode a coefficients
method with behaviour quite different from the other methods with the same name. In particular, we have
def coefficients(self): """ Return the nonzero coefficients of this polynomial in a list. The returned list is decreasingly ordered by the term ordering of ``self.parent()``, i.e. the list of coefficients matches the list of monomials returned by :meth:`sage.rings.polynomial.multi_polynomial_libsingular.MPolynomial_libsingular.monomials`. EXAMPLES:: sage: R.<x,y,z> = PolynomialRing(QQ,3,order='degrevlex') sage: f=23*x^6*y^7 + x^3*y+6*x^7*z sage: f.coefficients() [23, 6, 1] sage: R.<x,y,z> = PolynomialRing(QQ,3,order='lex') sage: f=23*x^6*y^7 + x^3*y+6*x^7*z sage: f.coefficients() [6, 23, 1] Test the same stuff with base ring `\ZZ`  different implementation:: sage: R.<x,y,z> = PolynomialRing(ZZ,3,order='degrevlex') sage: f=23*x^6*y^7 + x^3*y+6*x^7*z sage: f.coefficients() [23, 6, 1] sage: R.<x,y,z> = PolynomialRing(ZZ,3,order='lex') sage: f=23*x^6*y^7 + x^3*y+6*x^7*z sage: f.coefficients() [6, 23, 1]
As far as I can see, there is a single exception to the behaviour indicated by the doctests above, which is the definition in multi_power_series_ring_elemnt.py
.
I could imagine returning a lazy_list
and a list, if the support is known to be finite. Unfortunately, for the code in species, the default behaviour is sparse=False
, whereas in all other cases, it is sparse=True
.
Another minor issue: for L.<x> = LazyPowerSeriesRing(QQ)
we had L([1,2,3])
what would now be L([1,2], constant = 3)
. However, I really do not want to replicate this behaviour.
comment:40 Changed 3 months ago by
Cc:  Thierry Monteil added 

comment:41 Changed 3 months ago by
Commit:  b76d8f40ac50839297cb25fe23da16c0c0a8c49c → 4eb4dbd483cbac03078e0d709a8487c4f18ad219 

Branch pushed to git repo; I updated commit sha1. New commits:
4eb4dbd  fix behaviour of __getitem__ for slices, provide missing doctests, minor simplifications in species directory

comment:42 Changed 3 months ago by
Commit:  4eb4dbd483cbac03078e0d709a8487c4f18ad219 → 6f76b00e9f2aa3e39118b8278349f48de0753c05 

Branch pushed to git repo; I updated commit sha1. New commits:
6f76b00  replace LazyTaylorSeriesRing and LazyTaylorSeries with LazyPowerSeriesRing and LazyPowerSeries and add appropriate deprecations

comment:43 Changed 3 months ago by
This last push could be moved into a separate ticket if absolutely necessary.
Apart from that, the only thing missing is a decision about the semantics of the coefficients
function.
comment:44 followup: 45 Changed 3 months ago by
Replying to Martin Rubey:
I think that for
LazyTaylorSeries
,LazyDirichletSeries
,LazySymmetricFunction
, etc. we really don't wantsage: L.<x> = LazyTaylorSeriesRing(QQ) sage: (x^2/(1x)^3)[:5] [1, 3, 6] sage: L(lambda n: n*(n1)/2)[:5] [0, 0, 1, 3, 6]Instead, I think that for these classes we always want to start from degree 0. The current behaviour also breaks some doctests in the species directory.
I would say that is a bug and should be fixed (which I think is what you’re saying as well).
I am also hesitant to encode a
coefficients
method with behaviour quite different from the other methods with the same name. In particular, we havedef coefficients(self): """ Return the nonzero coefficients of this polynomial in a list. The returned list is decreasingly ordered by the term ordering of ``self.parent()``, i.e. the list of coefficients matches the list of monomials returned by :meth:`sage.rings.polynomial.multi_polynomial_libsingular.MPolynomial_libsingular.monomials`.As far as I can see, there is a single exception to the behaviour indicated by the doctests above, which is the definition in
multi_power_series_ring_elemnt.py
.
I agree with you. It’s not a bug, so we are not justified in simply changing the behavior. Although I feel a general deprecation message every time this is call would be obtrusive (and bad). I am leaning towards just making the backwards incompatible change, documenting how to get the previous behavior [f[i] for i in range(n)]
, and dealing with the consequences, but it was used all over the examples. I can’t think of a good way out of this other than something convoluted with an extra argument. Thus, we probably just have to make the change and take our medicine.
I could imagine returning a
lazy_list
and a list, if the support is known to be finite. Unfortunately, for the code in species, the default behaviour issparse=False
, whereas in all other cases, it issparse=True
.
I don’t think it is so important whether the backend is sparse or dense. I think you’re intuition is correct in returning a lazy_list
unless the series is known to be finite.
Another minor issue: for
L.<x> = LazyPowerSeriesRing(QQ)
we hadL([1,2,3])
what would now beL([1,2], constant = 3)
. However, I really do not want to replicate this behaviour.
Eek, that is highly unexpected. Did we think that should be the behavior at some point? I (now?) think we should have list input be the corresponding finite to match what polynomials do.
comment:45 followup: 49 Changed 3 months ago by
Replying to Travis Scrimshaw:
Replying to Martin Rubey:
I think that for
LazyTaylorSeries
,LazyDirichletSeries
,LazySymmetricFunction
, etc. we really don't wantsage: L.<x> = LazyTaylorSeriesRing(QQ) sage: (x^2/(1x)^3)[:5] [1, 3, 6] sage: L(lambda n: n*(n1)/2)[:5] [0, 0, 1, 3, 6]Instead, I think that for these classes we always want to start from degree 0.
I would say that is a bug and should be fixed (which I think is what you’re saying as well).
Yes, that's done already in the commit in comment:41.
I am also hesitant to encode a
coefficients
method with behaviour quite different from the other methods with the same name. In particular, we have
[in multi_polynomia.pyx
]
def coefficients(self): """ Return the nonzero coefficients of this polynomial in a list.
As far as I can see, there is a single exception to the behaviour indicated by the doctests above, which is the definition in
multi_power_series_ring_elemnt.py
.I agree with you. It’s not a bug, so we are not justified in simply changing the behavior. Although I feel a general deprecation message every time this is call would be obtrusive (and bad). I am leaning towards just making the backwards incompatible change, documenting how to get the previous behavior
[f[i] for i in range(n)]
, and dealing with the consequences, but it was used all over the examples. I can’t think of a good way out of this other than something convoluted with an extra argument. Thus, we probably just have to make the change and take our medicine.
Just to be clear: making the definition as
def coefficients(self, n=None, sparse=False):
i.e., with default behaviour as it is now, but with an option that makes it in line with the other coefficient methods in sage is *not* what we should do?
I must say, if we make the default sparse=True
, i.e., introduce a backwards incompatible change, then I think we really should provide a deprecation warning. One might not notice immediately that zeros are missing all of a sudden.
Possibly, to make this decision slightly less important, I could put the definition into the generating functions module only, or, alternatively, only into LazyPowerSeries
, and raise a NotImplementedError
for multivariate series.
Another minor issue: for
L.<x> = LazyPowerSeriesRing(QQ)
we hadL([1,2,3])
what would now beL([1,2], constant = 3)
. However, I really do not want to replicate this behaviour.Eek, that is highly unexpected.
As far as I remember we discussed this issue and decided that we cannot do anything sensible about it. I think I'll put a
.. WARNING:: The behaviour of ``LazyPowerSeries(l)`` for a list ``l`` with nonzero last element `e` changed with :trac:`32367`. To obtain the old behaviour, use ``LazyPowerSeries(l, constant=e)``.
into the docstring of the (new) LazyPowerSeries?.
comment:46 Changed 3 months ago by
Commit:  6f76b00e9f2aa3e39118b8278349f48de0753c05 → f5e3f2ef6f0c1757881e16c1563833cf119f4a83 

Branch pushed to git repo; I updated commit sha1. New commits:
f5e3f2e  fix documentation

comment:47 Changed 3 months ago by
I added a warning for the new behaviour concerning the specification of the constant.
comment:48 Changed 3 months ago by
Commit:  f5e3f2ef6f0c1757881e16c1563833cf119f4a83 → c5bfba5f8e8d41bc6341b7a490ae56469e2adcae 

Branch pushed to git repo; I updated commit sha1. New commits:
c5bfba5  adapt a doctest

comment:49 followup: 50 Changed 3 months ago by
Replying to Martin Rubey:
Replying to Travis Scrimshaw:
Replying to Martin Rubey:
I am also hesitant to encode a
coefficients
method with behaviour quite different from the other methods with the same name.[snip]
As far as I can see, there is a single exception to the behaviour indicated by the doctests above, which is the definition in
multi_power_series_ring_elemnt.py
.I agree with you. It’s not a bug, so we are not justified in simply changing the behavior. Although I feel a general deprecation message every time this is call would be obtrusive (and bad). I am leaning towards just making the backwards incompatible change, documenting how to get the previous behavior
[f[i] for i in range(n)]
, and dealing with the consequences, but it was used all over the examples. I can’t think of a good way out of this other than something convoluted with an extra argument. Thus, we probably just have to make the change and take our medicine.Just to be clear: making the definition as
def coefficients(self, n=None, sparse=False):i.e., with default behaviour as it is now, but with an option that makes it in line with the other coefficient methods in sage is *not* what we should do?
No, I agree with you that it is what we should do. The problem is how to do this with an effective and unobtrusive deprecation.
I must say, if we make the default
sparse=True
, i.e., introduce a backwards incompatible change, then I think we really should provide a deprecation warning. One might not notice immediately that zeros are missing all of a sudden.
Then we are introducing a new argument that we then have to add to code, which we will also want to deprecate later. This becomes a hassle on both the developer and user side.
Actually, maybe we can just issue a deprecation on the n
parameter, where we also say this now will return only nonzero coefficients? If people are passing n
, then they are expecting the old behavior.
Given the other backwardsincompatible change we are making (the one below), it isn’t much more painful to do a second one at the same time either.
Possibly, to make this decision slightly less important, I could put the definition into the generating functions module only, or, alternatively, only into
LazyPowerSeries
, and raise aNotImplementedError
for multivariate series.
I think it is better to be consistent about this.
Another minor issue: for
L.<x> = LazyPowerSeriesRing(QQ)
we hadL([1,2,3])
what would now beL([1,2], constant = 3)
. However, I really do not want to replicate this behaviour.Eek, that is highly unexpected.
As far as I remember we discussed this issue and decided that we cannot do anything sensible about it. I think I'll put a
.. WARNING:: The behaviour of ``LazyPowerSeries(l)`` for a list ``l`` with nonzero last element `e` changed with :trac:`32367`. To obtain the old behaviour, use ``LazyPowerSeries(l, constant=e)``.into the docstring of the (new)
LazyPowerSeries
.
Ah, I misread it initially. This was the behavior of the old LazyPowerSeries
. Just two little things with this:
 Use
L
instead ofl
since it has a chance to be read asI
depending on the font (like the one I am using to type this comment in fact).  Make sure there is an easy to see example of the new constant behavior is clearly visible (in the classlevel ring doc, its
_element_constructor_
, the element class’s classlevel doc; the modulelevel doc if there are examples there).
comment:50 followup: 55 Changed 3 months ago by
Replying to Travis Scrimshaw:
 Use
L
instead ofl
since it has a chance to be read asI
depending on the font (like the one I am using to type this comment in fact).
OK, I am using c
now, for 'coefficients', since L
is used for the parent usually.
 Make sure there is an easy to see example of the new constant behavior is clearly visible (in the classlevel ring doc, its
_element_constructor_
, the element class’s classlevel doc; the modulelevel doc if there are examples there).
There are several such examples, eg. line 1344 and beyond. Note that this change is not so bad as it may look: almost always, the old input was a list ending with 0, because this is the constant you usually want.
comment:51 Changed 3 months ago by
Commit:  c5bfba5f8e8d41bc6341b7a490ae56469e2adcae → ceecc89392820fbbf07f2ad1b9915ac02ac4d6d4 

Branch pushed to git repo; I updated commit sha1. New commits:
ceecc89  don't use l

comment:52 Changed 3 months ago by
Hello, we are supposed to be in maintenance mode today, as announced on sagedevel and trac main page. I think everything is now in order, but please try clicking around..
comment:54 Changed 3 months ago by
Commit:  ceecc89392820fbbf07f2ad1b9915ac02ac4d6d4 → 9629b2878b228fb2c5623f4d2b1df60e2c4bb287 

Branch pushed to git repo; I updated commit sha1. New commits:
9629b28  require the start value for __getitem__ of LazyLaurentSeries

comment:55 Changed 3 months ago by
Replying to Martin Rubey:
Replying to Travis Scrimshaw:
 Use
L
instead ofl
since it has a chance to be read asI
depending on the font (like the one I am using to type this comment in fact).OK, I am using
c
now, for 'coefficients', sinceL
is used for the parent usually.
Thank you.
 Make sure there is an easy to see example of the new constant behavior is clearly visible (in the classlevel ring doc, its
_element_constructor_
, the element class’s classlevel doc; the modulelevel doc if there are examples there).There are several such examples, eg. line 1344 and beyond. Note that this change is not so bad as it may look: almost always, the old input was a list ending with 0, because this is the constant you usually want.
Thank you. I didn't have time to check when I posted that.
Given your comments, I think this is now at needs review, correct? Feel free to revert if more is still needed.
Actually, I think I am mostly ready to turn this to a positive review if the patchbot comes back green.
comment:56 Changed 3 months ago by
I take that back slightly. I think we should have uniformity with the get item and that f[:5]
should work for all series because they all have an approximate valuation.
comment:57 followup: 59 Changed 3 months ago by
I am now (i.e., within the hour) implementing coefficients
. Other than that, it is ready!
(to be honest: !!!!!!!!! with just as many smileys!)
Explanation for the last push: we actually change _aproximate_valuation already in a place, and I think we will want to do that more often, and I certainly do not want that the result of getitem depends on that.
comment:58 followup: 60 Changed 3 months ago by
Actually, I just discovered something quite disturbing:
sage: L.<x> = PowerSeriesRing(QQ) sage: f = x^5/(12*x) sage: f[:10] <ipythoninput11f66b5b8a9c22>:1: DeprecationWarning: polynomial slicing with a start index is deprecated, use list() and slice the resulting list instead See http://trac.sagemath.org/18940 for details. f[:Integer(10)] 32 + 64*x + 128*x^2 + 256*x^3 + 512*x^4 + 1024*x^5 + 2048*x^6 + 4096*x^7 + 8192*x^8 + 16384*x^9 + O(x^15)
I find this rather hard to believe.
comment:59 Changed 3 months ago by
Replying to Martin Rubey:
Explanation for the last push: we actually change _aproximate_valuation already in a place, and I think we will want to do that more often, and I certainly do not want that the result of
__getitem__
depends on that.
I was misremembering what we implemented as I thought it did strip the leading zeros, but this is not the case:
sage: L.<x> = LazyLaurentSeriesRing(QQ) sage: f = L(lambda n: 1, valuation=0) sage: fp = (f  1) sage: fp[:5] [0, 1, 1, 1, 1]
I would say we should start from the approximate order up to the stop and then strip leading zeros (with updating the valuation accordingly). I find the f[:5]
syntax really nice. It also means we promise you have all nonzero coefficients up to the stop without having to compute the valuation (in particular, this would work for zero series).
This could be made consistent with f[a:b]
that does return the leading 0
's because the input is clearly different (and we would not update the approximate valuation because we are not using it).
The major point with updating the current behavior is there is no other way to guarantee that we have computed enough using f[a:b]
to guarantee all coefficients of degree less than a
are zero because we have not made the approximate valuation part of the API. Otherwise I would want a way to get that so I can guarantee my computations. (Of course, I can but don't want to keep track of this by hand.)
comment:60 Changed 3 months ago by
Replying to Martin Rubey:
Actually, I just discovered something quite disturbing:
sage: L.<x> = PowerSeriesRing(QQ) sage: f = x^5/(12*x) sage: f[:10] <ipythoninput11f66b5b8a9c22>:1: DeprecationWarning: polynomial slicing with a start index is deprecated, use list() and slice the resulting list instead See http://trac.sagemath.org/18940 for details. f[:Integer(10)] 32 + 64*x + 128*x^2 + 256*x^3 + 512*x^4 + 1024*x^5 + 2048*x^6 + 4096*x^7 + 8192*x^8 + 16384*x^9 + O(x^15)I find this rather hard to believe.
That is disturbing on many different levels for FPS. For polynomials, it probably would be better for them to be done as lists rather than polynomials, but it was somewhat natural there to return a polynomial as we often want to lop off terms. For FPS, I think slicing makes less sense to return a FPS because we are actually interested in a certain range of coefficients. We aren't really manipulating parts of the polynomial.
Since this is deprecated and could be removed now, we are free to implement our own version of slicing.
comment:61 followup: 63 Changed 3 months ago by
I am absolutely against starting with _approximate_order
, since this is not welldefined.
ANd I absolutely want a method (I do not care which name it has) which gives me the list or lazy list of all coefficients, beginning with a specified degree. This is one of the most important use cases for me, for example, I feed this list to oeis, or to the guessing packages, or I plot it.
comment:62 Changed 3 months ago by
(Let me stress again that _approximate_order may change after certain operations, and may depend on the way we construct the series.)
comment:63 Changed 3 months ago by
Replying to Martin Rubey:
I am absolutely against starting with
_approximate_order
, since this is not welldefined.
With slicing returning leading zeros, I completely agree that we should not do this. My proposal would be to strip the leading zeros and start at the actual valuation (or return an empty list if the valuation is at least as large as stop
).
ANd I absolutely want a method (I do not care which name it has) which gives me the list or lazy list of all coefficients, beginning with a specified degree. This is one of the most important use cases for me, for example, I feed this list to oeis, or to the guessing packages, or I plot it.
Big +1 as well. I am not proposing a change to the slicing f[start:stop]
, which would still return a list.
Basically, what I would have is a lazy series f
with:
 approximate valuation
a
 actual valuation
v
behave as follows:
 For
f[:stop]
, then get the list of coefficients fromf[a]
tof[stop1]
and strip the leading zeros and update the approximate valuation as appropriate.  For
f[start:start]
, return[f[i] for i in range(start, stop)]
.
As a concrete example, suppose we have g
with approximate valuation 3
but actual valuation 2
. Then doing the following (with a freshly created g
):
g[:5]
returns[2, 3, 4]
and sets the approximate valuation to2
.g[1:5]
returns[0, 0, 0, 2, 3, 4]
.g[:1]
returns[]
and sets the approximate valuation to1
.
Does that clarify what I am proposing? How do you feel about that? I can implement this if you want too.
comment:64 Changed 3 months ago by
Yes, I think that distinguishing between start=None
and start=d
in this way is a good idea.
I'd also propose to propagate this change to all polynomiallike __getitem__
methods (in a later ticket).
I have roughly half an hour left, which I will invest into the coefficient method. The tricky part is to get multivariate and graded series right, to be consistent with
sage: P.<x,y> = QQ[] sage: f = (x+2*y^3 +3*x*y + 5) sage: f.coefficients() [2, 3, 1, 5]
comment:65 Changed 3 months ago by
Hmm...that is a bit tricky. Can you just concatenate the coefficients()
of each of the components by degree?
comment:67 Changed 3 months ago by
Commit:  9629b2878b228fb2c5623f4d2b1df60e2c4bb287 → 037f3f7cad9b7bfb955f9cb3d1aaf7a4363c7ccb 

Branch pushed to git repo; I updated commit sha1. New commits:
037f3f7  implement coefficients

comment:68 Changed 3 months ago by
Commit:  037f3f7cad9b7bfb955f9cb3d1aaf7a4363c7ccb → 787197ed609f8291ba23d2c102686b2d8353ec9d 

Branch pushed to git repo; I updated commit sha1. New commits:
787197e  fix some bugs

comment:69 Changed 3 months ago by
Commit:  787197ed609f8291ba23d2c102686b2d8353ec9d → 8f46e127e30e1f0a3e6d1a6997e806baf7254710 

Branch pushed to git repo; I updated commit sha1. New commits:
8f46e12  implement new semantics of __getitem__

comment:71 Changed 3 months ago by
Commit:  8f46e127e30e1f0a3e6d1a6997e806baf7254710 → f706c53ec1c8d197b845488d763a975b886d1354 

Branch pushed to git repo; I updated commit sha1. New commits:
0de1c13  add a warning and an example for the arithmetic product with a constant term

8e5de35  add missing whitespace in docstring

f706c53  Merge branch 'u/mantepse/implement_arithmetic_product_of_lazy_symmetric_functions' of trac.sagemath.org:sage into t/32367/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series

comment:72 Changed 3 months ago by
The using you should use the approximate order instead of the actual order is that you do not need to compute it exactly. It should be “up to the (necessarily specified) stop
” so it will still work for series that do not know they are zero (nor compute more than they need to).
I also think the result of coefficients()
should all lie in the base ring.
I have a number of other small changes that I will do tomorrow, along with the above two if you don’t disagree.
comment:73 Changed 3 months ago by
Re stop: Oh, yes, I agree! I overlooked that!
Re base ring: indeed. (I thought they did.)
comment:74 followup: 78 Changed 3 months ago by
There is a bug in LazyModuleElement.map_coefficients
: it applies the map to the coefficients of the stream, rather than the coefficients of the series. It is unfortunate that we call the elements of the stream coefficients, we should call them graded pieces or something like that.
comment:75 Changed 3 months ago by
Commit:  f706c53ec1c8d197b845488d763a975b886d1354 → 3410eda2266a4ed0c38dc8ae053bd13194e26d62 

Branch pushed to git repo; I updated commit sha1. New commits:
3410eda  fix map_coefficients

comment:76 Changed 3 months ago by
Commit:  3410eda2266a4ed0c38dc8ae053bd13194e26d62 → 8fc2dd1a5c5de7f393de048c920e1d49be4660ba 

Branch pushed to git repo; I updated commit sha1. New commits:
8fc2dd1  fix doctests because of __getitem__ change

comment:77 Changed 3 months ago by
Commit:  8fc2dd1a5c5de7f393de048c920e1d49be4660ba → 2a3389e6ae2c5be15a44f52d4b2fb339e63bc827 

Branch pushed to git repo; I updated commit sha1. New commits:
2a3389e  compute no more elements than necessary in __getitem__

comment:78 Changed 3 months ago by
Replying to Martin Rubey:
There is a bug in
LazyModuleElement.map_coefficients
: it applies the map to the coefficients of the stream, rather than the coefficients of the series. It is unfortunate that we call the elements of the stream coefficients, we should call them graded pieces or something like that.
Good catch. The fix seems good, although it seems a bit brittle. I will see if I can come up with a better test, but this can work if I can’t.
comment:79 Changed 3 months ago by
Commit:  2a3389e6ae2c5be15a44f52d4b2fb339e63bc827 → 89d9cccd0e5db988005e3503f86efd7283fea038 

Branch pushed to git repo; I updated commit sha1. New commits:
89d9ccc  fix pyflakes warnings

comment:80 Changed 3 months ago by
Given ticket:34470#comment:19, possibly we should do

src/sage/rings/lazy_series_ring.py
diff git a/src/sage/rings/lazy_series_ring.py b/src/sage/rings/lazy_series_ring.py index 98e40a41c8f..5164e19dd2e 100644
a b class LazyCompletionGradedAlgebra(LazySeriesRing): 1578 1578 if basis in Algebras.TensorProducts: 1579 1579 self._arity = len(basis._sets) 1580 1580 else: 1581 if basis not in Algebras.Graded:1581 if basis not in GradedAlgebrasWithBasis 1582 1582 raise ValueError("basis should be a graded algebra") 1583 1583 self._arity = 1 1584 1584 category = Algebras(base_ring.category())
comment:81 Changed 3 months ago by
Indeed, we should do that. I didn’t realize I did it like this at the time.
comment:82 Changed 3 months ago by
Commit:  89d9cccd0e5db988005e3503f86efd7283fea038 → cf2b0cb9456298c279d7270d4c44e208929370e4 

Branch pushed to git repo; I updated commit sha1. New commits:
cf2b0cb  require a graded basis

comment:83 Changed 3 months ago by
Do you think we are ready? The part of comment:72, concerning coefficients being in the base ring might still be open.
I am guessing that this will not make it into the upcoming release, right?
comment:84 Changed 3 months ago by
Branch:  u/mantepse/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series → u/tscrim/replace_lazy_power_series32367 

Commit:  cf2b0cb9456298c279d7270d4c44e208929370e4 → 3bd9f8c26726786ff40db79486a1cb0b3cd58b00 
Reviewers:  → Martin Rubey, Travis Scrimshaw 
Indeed, I think this is ready.
My last commit makes sure the coefficients are in the correct ring, which was not the case before for the test I added.
I did a few other small touch ups that I saw (mainly breaking very long doctest output lines) and marking one test as long.
If my changes are good, then positive review.
Indeed, this will definitely not be in 9.7 since we are in the rc stages. However, that is probably better so it can get lots of testing during the beta releases.
New commits:
c90cb91  Merge branch 'u/mantepse/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series' of https://github.com/sagemath/sagetracmirror into u/tscrim/replace_lazy_power_series32367

3bd9f8c  Fixing coefficients bug and some doc tweaks.

comment:85 Changed 3 months ago by
I'm afraid, there is something we did not want:
sage: L.<x> = LazyPowerSeriesRing(GF(2)) sage: f=L(lambda n: n) sage: f._coeff_stream._cache {} sage: f x + x^3 + x^5 + O(x^7) sage: f._coeff_stream._cache {0: 0, 1: 1, 2: 2, 3: 3, 4: 4, 5: 5, 6: 6}
I am quite sure we wanted that the cache contains the coefficients in the correct ring, no?
comment:86 Changed 3 months ago by
I think I forgot to adapt the _element_constructor_
of all rings except for the LazyTaylorSeriesRing
in https://git.sagemath.org/sage.git/commit/?h=3bd9f8c26726786ff40db79486a1cb0b3cd58b00&id=60103619caa275cb7087e425d25afacdba48af9b
I am guessing that your change to coefficients is masking that.
I have to leave now, I'll try to fix it in the evening.
comment:87 Changed 3 months ago by
Branch:  u/tscrim/replace_lazy_power_series32367 → u/mantepse/replace_lazy_power_series32367 

comment:88 Changed 3 months ago by
Commit:  3bd9f8c26726786ff40db79486a1cb0b3cd58b00 → 555d1b07df4f0ba82f23392142037114c03cd0e9 

Done! I'm happy for the moment. Can't wait to see this in sage develop!
I'll wait with the other tickets until it is, so that I can see the diff better.
New commits:
555d1b0  fix LazyPowerSeries._element_constructor_ when passed a function

comment:89 Changed 3 months ago by
I lean towards not having the cached coefficients to be in the base ring as much as possible so intermediate computations could succeed. I saw I could do the change you did and explicitly did not do this because of potential intermediate computations. Although it would make the behavior independent of the arity, which is good. I don't hold a strong opinion here (right now). So your change is fine with me, and you can also remove the extra mappings to the base ring.
Also, I forgot to mention I did find a test for checking how the coefficients behave that I think is more robust (at least, with the current implementation):
 if self.base_ring() == self.parent()._internal_poly_ring.base_ring(): + if P._internal_poly_ring.base_ring() is not P._laurent_poly_ring:
What I am worried about is you might possibly get a false equality in the first test. I am not 100% certain this could happen (maybe with Dirichlet series and SR
), but with the changed test, those in the new test are identical if and only if we are using the internal polynomial ring to encode the degree.
comment:90 followup: 93 Changed 3 months ago by
In all other rings we have in lazy_series_ring.py
we put the coefficients into the base ring using the element constructor. This change was done in #34473, where I overlooked the case of power series. It should also be consistent with the other ways to construct an element, intentionwise.
There are two reasons I think that this is a good idea:
 the conversion into the base ring may be computationally expensive, especially in the case of symmetric functions.
 if a coefficient happens to be zero in the base ring, but not in the given ring,
Stream_map_coefficients
will apply the function in one case, but not the other. I think that this could lead to rather obscure bugs.
I believe I removed the extra mappings in my last commit, and I also saw your new test, which is fine with me.
comment:91 Changed 3 months ago by
Commit:  555d1b07df4f0ba82f23392142037114c03cd0e9 → baea68dbd79e0a0774f2763c12aa776a01a5f0c5 

Branch pushed to git repo; I updated commit sha1. New commits:
baea68d  pass functions using coefficients to element constructor

comment:93 followup: 94 Changed 3 months ago by
Replying to Martin Rubey:
In all other rings we have in
lazy_series_ring.py
we put the coefficients into the base ring using the element constructor. This change was done in #34473, where I overlooked the case of power series. It should also be consistent with the other ways to construct an element, intentionwise.
I missed this change on my review of #34473. I would have raised it then, but now it might be too late.
There are two reasons I think that this is a good idea:
 the conversion into the base ring may be computationally expensive, especially in the case of symmetric functions.
This seems to be a reason to not do it until the very end.
 if a coefficient happens to be zero in the base ring, but not in the given ring,
Stream_map_coefficients
will apply the function in one case, but not the other. I think that this could lead to rather obscure bugs.
A good point, but are we guaranteed to not get the same behavior with other streams? I think so, but it is not clear. I am wondering if we want to actually go back to including the ring for Stream_map_coefficients
to really make sure we have the correct inputs. IIRC, we removed that to uniformly have no stream take a ring as input, but this one appears to be really special.
I believe I removed the extra mappings in my last commit, and I also saw your new test, which is fine with me.
Thank you.
comment:94 followup: 95 Changed 3 months ago by
 the conversion into the base ring may be computationally expensive, especially in the case of symmetric functions.
This seems to be a reason to not do it until the very end.
I should have been more precise: if we do the conversion before putting things into the cache, we only do it once, and use it for subsequent computations. Otherwise, we may do it many times.
Of course, if computations in one base ring turn out to be more efficient than in another one, then it may make sense to do a change_ring
at the very end.
A good point, but are we guaranteed to not get the same behavior with other streams? I think so, but it is not clear. I am wondering if we want to actually go back to including the ring for Stream_map_coefficients to really make sure we have the correct inputs. IIRC, we removed that to uniformly have no stream take a ring as input, but this one appears to be really special.
In principle, I like the current setup, but I agree that the burden on developers is quite big. However, I think I'd prefer to do this in a different ticket, after this one is in develop. I find it already quite hard to see the modifications we make.
Once this is positive review, I would write a short summary describing what we did for the mailing list. I think we have done something really awesome!
comment:95 Changed 3 months ago by
Replying to Martin Rubey:
 the conversion into the base ring may be computationally expensive, especially in the case of symmetric functions.
This seems to be a reason to not do it until the very end.
I should have been more precise: if we do the conversion before putting things into the cache, we only do it once, and use it for subsequent computations. Otherwise, we may do it many times.
Of course, if computations in one base ring turn out to be more efficient than in another one, then it may make sense to do a
change_ring
at the very end.
Indeed, this is hard to say. I would prefer to leave it as much as possible in the hands of the user to decide, but this is a fairly technical thing to document.
A good point, but are we guaranteed to not get the same behavior with other streams? I think so, but it is not clear. I am wondering if we want to actually go back to including the ring for Stream_map_coefficients to really make sure we have the correct inputs. IIRC, we removed that to uniformly have no stream take a ring as input, but this one appears to be really special.
In principle, I like the current setup, but I agree that the burden on developers is quite big.
A bit of a side effect of being packed with awesomeness. The current setup is sufficient for me. I am just wondering while we wait if it can be made better easily.
However, I think I'd prefer to do this in a different ticket, after this one is in develop. I find it already quite hard to see the modifications we make.
Easiest way would be to run git diff
locally between this branch and the dependency.
Once this is positive review, I would write a short summary describing what we did for the mailing list. I think we have done something really awesome!
+1 I have already put this code to good use with a problem I have been working on.
There are still a few things to be improved upon, but this seems to be quite fast and very powerful.
comment:98 Changed 3 months ago by
Commit:  baea68dbd79e0a0774f2763c12aa776a01a5f0c5 → 7745337b260ad2e37c1c30b04756606e0e775610 

Branch pushed to git repo; I updated commit sha1. New commits:
7745337  remove unused variable detected by pyflakes

comment:99 Changed 3 months ago by
Commit:  7745337b260ad2e37c1c30b04756606e0e775610 → cdad494f9eb5f409f987301f4a8fd073c846f1b2 

Branch pushed to git repo; I updated commit sha1. New commits:
cdad494  do not advertise L(None) in favour of L.define()

comment:100 Changed 3 months ago by
Commit:  cdad494f9eb5f409f987301f4a8fd073c846f1b2 → f541f3d184e8b2468da5c2953383a4979996ec5c 

comment:101 Changed 3 months ago by
Description:  modified (diff) 

Summary:  Replace Lazy Power Series in species directory with the new Lazy Taylor Series → Replace Lazy Power Series in species directory 
comment:102 Changed 3 months ago by
Commit:  f541f3d184e8b2468da5c2953383a4979996ec5c → 1df62c8481a2bf52542f5e50a6b5d8610a8ac4b9 

Branch pushed to git repo; I updated commit sha1. New commits:
1df62c8  fix wrong ring in doctest

comment:105 Changed 3 months ago by
From the patchbot, I caught a few errors. I have mostly fixed them as they are trivial, but there is one that requires discussion.
In particular, changing the base ring to QQ
leads to not implemented methods because of the change in category. Some of these are noted on #34470. My proposal is we first add in these methods (possibly on a separate ticket) and the uncontroversial changes from #34470. (I already have a branch that does this. I can handle the merge with the current branch here and my other changes.)
Now we could just sweep this under the rug by skipping the _test_not_implemented
, but I think it is easier and better to just fix them first.
What do you think?
comment:106 followup: 107 Changed 3 months ago by
I don't quite understand: I see three patchbot runs (one from github and two at https://patchbot.sagemath.org/ticket/32367/), but all three of them say "all tests passed". Also, the pyflakes doesn't mention any nonexpected errors. Where can I find the report you mention?
Apart from that: of course, please go ahead!
Apart from that: is any of the changes in the branch attached to #34470 controversial? Here is a diff. An implementation of residue_field
is still missing. More generally, I did not find out how to activate the _test_not_implemented
gadget.

src/sage/rings/lazy_series.py
diff git a/src/sage/rings/lazy_series.py b/src/sage/rings/lazy_series.py index 4e2f35f2c3b..23992898376 100644
a b class LazyLaurentSeries(LazyCauchyProductSeries): 2934 2934 sage: f = 1 / (1  z  z^2) 2935 2935 sage: TestSuite(f).run() 2936 2936 """ 2937 def _im_gens_(self, codomain, im_gens, base_map=None): 2938 """ 2939 Returns the image of ``self`` under the map that sends the 2940 generators of the parent of ``self`` to the elements of the 2941 tuple ``im_gens``. 2942 2943 EXAMPLES:: 2944 2945 sage: Z.<x> = ZZ[] 2946 sage: K.<i> = NumberField(x^2 + 1) 2947 sage: R.<t> = LazyLaurentSeriesRing(K) 2948 sage: f = R(lambda n: i^n, valuation=2) 2949 sage: f 2950 t^2  i*t^1 + 1 + i*t  t^2  i*t^3 + t^4 + O(t^5) 2951 sage: f._im_gens_(R, [t + t^2]) 2952 t^2 + (i + 2)*t^1 + (i  2) + 4*t + (2*i  6)*t^2 + (2*i + 4)*t^3 + (2*i  7)*t^4 + O(t^5) 2953 2954 sage: cc = K.hom([i]) 2955 sage: f._im_gens_(R, [t + t^2], base_map=cc) 2956 t^2 + (i + 2)*t^1 + (i  2) + 4*t + (2*i  6)*t^2 + (2*i + 4)*t^3 + (2*i  7)*t^4 + O(t^5) 2957 2958 """ 2959 if base_map is None: 2960 return codomain(self(im_gens[0])) 2961 2962 return codomain(self.map_coefficients(base_map)(im_gens[0])) 2937 2963 2938 2964 def __call__(self, g, *, check=True): 2939 2965 r""" … … class LazyPowerSeries(LazyCauchyProductSeries): 3844 3870 deprecation(32367, "the method compute_coefficients obsolete and has no effect.") 3845 3871 return 3846 3872 3873 def _im_gens_(self, codomain, im_gens, base_map=None): 3874 """ 3875 Returns the image of ``self`` under the map that sends the 3876 generators of the parent of ``self`` to the elements of the 3877 tuple ``im_gens``. 3878 3879 EXAMPLES:: 3880 3881 sage: Z.<x> = QQ[] 3882 sage: R.<q, t> = LazyPowerSeriesRing(Z) 3883 sage: f = 1/(1qt) 3884 sage: f 3885 1 + (q+t) + (q^2+2*q*t+t^2) + (q^3+3*q^2*t+3*q*t^2+t^3) + (q^4+4*q^3*t+6*q^2*t^2+4*q*t^3+t^4) + (q^5+5*q^4*t+10*q^3*t^2+10*q^2*t^3+5*q*t^4+t^5) + (q^6+6*q^5*t+15*q^4*t^2+20*q^3*t^3+15*q^2*t^4+6*q*t^5+t^6) + O(q,t)^7 3886 sage: S.<s> = LazyPowerSeriesRing(Z) 3887 sage: f._im_gens_(S, [s, x*s]) 3888 1 + ((x+1)*s) + ((x^2+2*x+1)*s^2) + ((x^3+3*x^2+3*x+1)*s^3) + ((x^4+4*x^3+6*x^2+4*x+1)*s^4) + ((x^5+5*x^4+10*x^3+10*x^2+5*x+1)*s^5) + ((x^6+6*x^5+15*x^4+20*x^3+15*x^2+6*x+1)*s^6) + O(s^7) 3889 3890 sage: cc = Z.hom([x]) 3891 sage: f = 1/(1+x*qt) 3892 sage: f._im_gens_(S, [s, x*s], base_map=cc) 3893 1 + 2*x*s + 4*x^2*s^2 + 8*x^3*s^3 + 16*x^4*s^4 + 32*x^5*s^5 + 64*x^6*s^6 + O(s^7) 3894 """ 3895 if base_map is None: 3896 return codomain(self(*im_gens)) 3897 3898 return codomain(self.map_coefficients(base_map)(*im_gens)) 3899 3847 3900 def __call__(self, *g, check=True): 3848 3901 r""" 3849 3902 Return the composition of ``self`` with ``g``. 
src/sage/rings/lazy_series_ring.py
diff git a/src/sage/rings/lazy_series_ring.py b/src/sage/rings/lazy_series_ring.py index acaef041566..7b8a001ca1d 100644
a b class LazyPowerSeriesRing(LazySeriesRing): 1201 1201 1202 1202 sage: L = LazyPowerSeriesRing(ZZ, 't') 1203 1203 sage: TestSuite(L).run(skip=['_test_elements', '_test_associativity', '_test_distributivity', '_test_zero']) 1204 1205 sage: L = LazyPowerSeriesRing(ZZ, 's, t') 1206 sage: TestSuite(L).run(skip=['_test_elements', '_test_associativity', '_test_distributivity', '_test_zero']) 1207 1208 Check that :trac:`34470` is fixed:: 1209 1210 sage: L.<t> = LazyPowerSeriesRing(QQ) 1211 sage: L in CompleteDiscreteValuationRings 1212 True 1213 sage: L.uniformizer() 1214 t 1215 sage: lcm(1/(1  t^2)  1, t) 1216 t^2 1217 1218 sage: L.<t> = LazyPowerSeriesRing(ZZ) 1219 sage: L in PrincipalIdealDomains 1220 False 1221 1222 sage: L = LazyPowerSeriesRing(QQ, 's, t') 1223 sage: L in PrincipalIdealDomains 1224 False 1204 1225 """ 1205 1226 from sage.structure.category_object import normalize_names 1206 1227 names = normalize_names(1, names) … … class LazyPowerSeriesRing(LazySeriesRing): 1213 1234 else: 1214 1235 self._internal_poly_ring = PolynomialRing(self._laurent_poly_ring, "DUMMY_VARIABLE") 1215 1236 category = Algebras(base_ring.category()) 1216 if base_ring in Fields() :1237 if base_ring in Fields() and self._arity == 1: 1217 1238 category &= CompleteDiscreteValuationRings() 1239 self.uniformizer = lambda: self.gen() 1218 1240 elif base_ring in IntegralDomains(): 1219 1241 category &= IntegralDomains() 1220 1242 elif base_ring in Rings().Commutative(): … … class LazyCompletionGradedAlgebra(LazySeriesRing): 1610 1632 sage: L = LazySymmetricFunctions(s) 1611 1633 sage: TestSuite(L).run(skip=['_test_elements', '_test_associativity', '_test_distributivity', '_test_zero']) 1612 1634 1635 Check that :trac:`34470` is fixed:: 1636 1637 sage: s = SymmetricFunctions(QQ).s() 1638 sage: L = LazySymmetricFunctions(s) 1639 sage: L in PrincipalIdealDomains 1640 False 1641 1613 1642 Check that a basis which is not graded is not enough:: 1614 1643 1615 1644 sage: ht = SymmetricFunctions(ZZ).ht() … … class LazyCompletionGradedAlgebra(LazySeriesRing): 1627 1656 raise ValueError("basis should be in GradedAlgebrasWithBasis") 1628 1657 self._arity = 1 1629 1658 category = Algebras(base_ring.category()) 1630 if base_ring in Fields(): 1631 category &= CompleteDiscreteValuationRings() 1632 elif base_ring in IntegralDomains(): 1659 if base_ring in IntegralDomains(): 1633 1660 category &= IntegralDomains() 1634 1661 elif base_ring in Rings().Commutative(): 1635 1662 category = category.Commutative()
comment:107 Changed 3 months ago by
Replying to Martin Rubey:
I don't quite understand: I see three patchbot runs (one from github and two at https://patchbot.sagemath.org/ticket/32367/), but all three of them say "all tests passed". Also, the pyflakes doesn't mention any nonexpected errors. Where can I find the report you mention?
If you look at the coverage, you see that there are missing doctests. self:
is not sage:
, so those tests are not getting run. So I changed them to run the TestSuite
on the corresponding classes, which lead to failures of the form:
sage: L.<z> = LazyLaurentSeriesRing(QQ) sage: L._test_not_implemented_methods()
which we were not previously testing in our old code.
Apart from that: of course, please go ahead!
I will merge it into this ticket and push shortly.
Apart from that: is any of the changes in the branch attached to #34470 controversial?
As I mentioned on #34470, not currently, but there remains the question of putting things in UFD or not. As well as if we want them to be graded (by default) or not.
Here is a diff. An implementation of
residue_field
is still missing. More generally, I did not find out how to activate the_test_not_implemented
gadget.
Thank you for the diff.
The biggest change I made is that uniformizer
and residue_field
are always included as public methods on the ring. I want to avoid the monkey patching (which should generally be avoided), which not only hides documentation, there is a a technical difference as well:
sage: class Foo: ....: def __init__(self): ....: self.bar = lambda: 5 ....: def baz(self): ....: return 5 ....: sage: F = Foo() sage: F.bar() 5 sage: F.baz() 5 sage: F.bar <function Foo.__init__.<locals>.<lambda> at 0x7f9d6bea1630> sage: F.baz <bound method Foo.baz of <__main__.Foo object at 0x7f9d728a2530>>
While I doubt this is important in practice, it is generally considered bad practice to monkey patch. If you really don't want these methods there, I can implement the appropriate subclasses with the appropriate dispatching using the standard __classcall_private__
mechanism (like, e.g., Partitions
).
comment:108 Changed 3 months ago by
Branch:  u/mantepse/replace_lazy_power_series32367 → u/tscrim/replace_lazy_power_series32367 

Commit:  1df62c8481a2bf52542f5e50a6b5d8610a8ac4b9 → 69d44abf4ce2ec4fb652ec8e39dac42cd044537f 
New commits:
23d951f  Doing some reviewer changes.

6a3559e  Merge branch 'u/mantepse/implement_arithmetic_product_of_lazy_symmetric_functions' of https://github.com/sagemath/sagetracmirror into public/lazy_series/dvr_methodsTBA

6969549  Implementing methods for DVF.

91244f3  Merge branch 'public/lazy_series/dvr_methodsTBA' into u/tscrim/replace_lazy_power_series32367

0a8b4c6  implement _im_gens_, part 1

380ddc9  adapt doctests

69d44ab  Removing old use of LazyTaylorSeriesRing from my changes.

comment:112 Changed 3 months ago by
The bug above was easy to fix (I'll do it in #34470, because that's where I noticed it), but the following garbageingarbageout is really hard to detect:
sage: L.<t> = LazyPowerSeriesRing(QQ) sage: t*((1+t)/(tt^2)) 1 + t + t^2 + O(t^3) sage: t*(1+t)/(tt^2) 1 + 2*t + 2*t^2 + 2*t^3 + O(t^4)
comment:113 Changed 3 months ago by
Did we want to error out when doing the division by a positive valuation series? I don’t remember. Although these are integral domains, right (of course, assuming the base ring is)? So we could (and probably should) follow many other things in Sage and go to the formal fraction field and do the computation there, although that possibly needs the gcd
to be implemented. I am not sure what the best way forward with division. In any case, we should end up with the second answer (and likely consistently in the fraction field).
comment:114 Changed 3 months ago by
Fraction field sounds great.
The problem i see with error: we can only check once a coefficient is requested. If we check already in div itself, implicit definitions might not work anymore.
comment:115 Changed 3 months ago by
Yea, I don’t see an easy way out of something that has an uninitialized series. Although I was thinking of erroring out whenever the approximate valuation was positive.
With fraction fields, we run into the problem of if we want to always return something in the fraction field or not. If we do, then we need to teach using more //
instead of /
. If we don’t, then we need to check somehow when to give something in the fraction field, which is basically the same problem.
Maybe we could get around this with using the same implementation for the fraction field with just a different parent. This might help with making things work more seamlessly.
For the shortterm, I propose we raise an NotImplementedError
whenever the denominator is known to have a positive valuation.
comment:116 Changed 3 months ago by
OK, any way you like. I admit that I like the idea of //
. I remember that in FriCAS it was sometimes buggy to go from the fraction field to the ring, if mathematically possible.
comment:117 Changed 3 months ago by
A fraction field shouldn't be too hard to implement, but it shouldn't be something we should hold up this ticket for. I have a good idea for how to do it now. Well, for right now, it is GIGO, so we can leave it be other than explicitly telling the user when giving a positive valuation series (and promising we will implement it via the NotImplementedError
, as we know those cases are bad). What do you think?
comment:118 Changed 3 months ago by
I keep saying to myself: let me just do this tiny thing, and then I'll stop and continue with my research (one week to go until term starts).
Now it's saturday evening, and I haven't even started what I promised to do...
It is fun, though.
comment:119 Changed 3 months ago by
It wasn't as easy as I thought to raise an error and be lazy at the same time, but its done.
Again, I pushed this to #34470.
I used the TestSuite.run()
a lot to discover bugs, and I found many. Whenever I thought I finally did it right, the TestSuite.run()
taught me otherwise. We should absolutely put more effort into these.
comment:120 Changed 3 months ago by
From a quick look and what I was thinking, it seems like an overly complicated solution. I was just thinking of putting if self._stream._approximate_order > 0:
in the inverse/division methods, then in the division/inverse streams, adding a check
attribute and, e.g., if check and not self.right[0]:
. I will take a closer look today.
Indeed, that is the idea behind the TestSuite
, and one of the reasons why I make sure to add it somewhere, typically for the __init__
doctests.
comment:121 Changed 3 months ago by
Well, part of the problem was that we were not lazy enough for certain recursive definitions. For example
sage: P.<x> = LazyPowerSeriesRing(QQ) sage: f = P.undefined() sage: f.define(1  ~f*x) sage: f 1  x  x^2  2*x^3  5*x^4  14*x^5  42*x^6 + O(x^7) sage: D = LazyDirichletSeriesRing(QQ, "s") sage: g = D([0, 1]) sage: f = D.undefined() sage: f.define(1 + ~f*g) sage: f 1 + 1/(2^s)  1/(4^s) + O(1/(8^s))
did not work previously.
Also, equality testing should be clearer now. I don't know why we decided to check terms in the range
n = min(self._coeff_stream._approximate_order, other._coeff_stream._approximate_order) m = max(self._coeff_stream._approximate_order, other._coeff_stream._approximate_order)
previously  it seems to me that this computes coefficients for no good reason. (it won't hurt, but it won't help either).
Other than that, the diff is not really large.
I believe that giving a name to the true order, if it is known, might be helpful for further improvements, but I did not check.
I also did not check whether we should update _approximate_order
in Stream_inexact.__getitem__
. I would like to first implement more _test_XXX
methods (in particular for composition and reversion), to get a better picture of performance.
comment:122 Changed 3 months ago by
Commit:  69d44abf4ce2ec4fb652ec8e39dac42cd044537f → 3d5b8cf4c836ceb8813aa6311fe18b1f5fbffc6a 

Branch pushed to git repo; I updated commit sha1. New commits:
3d5b8cf  Last little things from the patchbot.

comment:123 Changed 3 months ago by
Well, for now, let's bring this to a positive review and move onto the next ticket (assuming we get a green bot this time because I didn't do anything stupid). I will do stuff there starting from your changes.
I did a few little things from what I saw from the last patchbot report (the other failure seems unrelated). I made species library lazily imported to decrease the startup modules number.
comment:124 Changed 3 months ago by
Milestone:  sage9.7 → sage9.8 

comment:125 Changed 3 months ago by
The Returns
in the doctring noticed by the patchbot is fixed in the followup ticket #34470.
The pyflakes warnings about imported but unused modules in species/library.py
, are, as far as I know, unavoidable.
The ellipsis in the doctest in sfa.py
is misinterpreted by the patchbot as a doctest continuation, but is correct, as far as I know.
I am therefore setting this to positive review.
comment:126 Changed 3 months ago by
Status:  needs_review → positive_review 

comment:127 Changed 2 months ago by
Branch:  u/tscrim/replace_lazy_power_series32367 → u/mantepse/replace_lazy_power_series32367 

comment:128 Changed 2 months ago by
Commit:  3d5b8cf4c836ceb8813aa6311fe18b1f5fbffc6a → 4172688e381f6b8e617bcd1a7583aec50f346307 

Status:  positive_review → needs_review 
Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. Last 10 new commits:
5d76825  Merge branch 'u/chapoton/34494' of https://github.com/sagemath/sagetracmirror into u/tscrim/derivatives_lazy_series34413

97df300  Updating sf/sfa.py doctest due to #34494.

1388b8a  Merge branch 'u/chapoton/34494' of https://github.com/sagemath/sagetracmirror into public/rings/lazy_series_revert34383

7bfe5f7  Merge branch 'public/rings/lazy_series_revert34383' of https://github.com/sagemath/sagetracmirror into public/rings/lazy_series_revert34383

2039990  Updating doctest due to changes from #34494.

cdea820  Merge branch 'public/rings/lazy_series_revert34383' of https://github.com/sagemath/sagetracmirror into u/tscrim/derivatives_lazy_series34413

b7f04ed  Merge branch 'develop' of trac.sagemath.org:sage into t/34413/derivatives_lazy_series34413

2325826  Merge branch 'u/mantepse/derivatives_lazy_series34413' of trac.sagemath.org:sage into t/34422/implement_functorial_composition_of_lazy_symmetric_functiosn

ebcf5c3  Merge branch 'u/mantepse/implement_functorial_composition_of_lazy_symmetric_functiosn' of trac.sagemath.org:sage into t/34423/implement_arithmetic_product_of_lazy_symmetric_functions

4172688  Merge branch 'u/mantepse/implement_arithmetic_product_of_lazy_symmetric_functions' of trac.sagemath.org:sage into t/32367/replace_lazy_power_series32367

comment:129 Changed 2 months ago by
Status:  needs_review → positive_review 

trivial (autopmatic) merge, necessary to make the patchbots happy.
comment:130 Changed 2 months ago by
Commit:  4172688e381f6b8e617bcd1a7583aec50f346307 → 4fc981b458f8d628684decf221618911ace06333 

Status:  positive_review → needs_review 
comment:131 Changed 2 months ago by
Status:  needs_review → positive_review 

trivial fixes to make patchbot happy.
comment:132 Changed 2 months ago by
Branch:  u/mantepse/replace_lazy_power_series32367 → 4fc981b458f8d628684decf221618911ace06333 

Resolution:  → fixed 
Status:  positive_review → closed 
Branch pushed to git repo; I updated commit sha1. Last 10 new commits:
Revert "Merge branch 'u/ghtejasvicsr1/special_functions_for_lazy_series' of git://trac.sagemath.org/sage into t/32324/lazy_taylor_series"
fix invert
Small reworkings and fixes. Adding another composition test.
Adding documentation to the userfacing class level documentation.
Refactoring we discussed at the meeting today.
only get coefficient if necessary
Merge branch 'u/mantepse/dense_lls31897' of git://trac.sagemath.org/sage into t/32324/lazy_taylor_series
fix and document series reversion
Merge remotetracking branch 'trac/u/mantepse/lazy_taylor_series' into t/32367/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series
Merged new Taylor series and deleted new_gs.py