Replace Lazy Power Series in species directory

[tejasvicsr1]

In this ticket we finalize the implementation of lazy series, and replace the old lazy series framework.

Depends on #34423

CC: @mantepse @tscrim @pfili @zimmermann6 @sagetrac-tmonteil

Component: combinatorics

Keywords: LazyPowerSeries, FormalSeries, gsoc2021

Author: Tejasvi Chebrolu, Martin Rubey, Travis Scrimshaw

Branch/Commit: 4fc981b

Reviewer: Martin Rubey, Travis Scrimshaw

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

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 8fc2dd1 to 2a3389e

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

2a3389e

compute no more elements than necessary in __getitem__

[tscrim]

comment:78

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.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

89d9ccc

fix pyflakes warnings

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 2a3389e to 89d9ccc

[mantepse]

comment:80

Given #34470 comment:19, possibly we should do

diff --git a/src/sage/rings/lazy_series_ring.py b/src/sage/rings/lazy_series_ring.py
index 98e40a41c8f..5164e19dd2e 100644
--- a/src/sage/rings/lazy_series_ring.py
+++ b/src/sage/rings/lazy_series_ring.py
@@ -1578,7 +1578,7 @@ class LazyCompletionGradedAlgebra(LazySeriesRing):
         if basis in Algebras.TensorProducts:
             self._arity = len(basis._sets)
         else:
-            if basis not in Algebras.Graded:
+            if basis not in GradedAlgebrasWithBasis
                 raise ValueError("basis should be a graded algebra")
             self._arity = 1
         category = Algebras(base_ring.category())

[tscrim]

comment:81

Indeed, we should do that. I didn’t realize I did it like this at the time.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

cf2b0cb

require a graded basis

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 89d9ccc to cf2b0cb

[mantepse]

comment:83

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?

[tscrim]

Changed commit from cf2b0cb to 3bd9f8c

[tscrim]

comment:84

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/sagetrac-mirror into u/tscrim/replace_lazy_power_series-32367
3bd9f8c	Fixing coefficients bug and some doc tweaks.

[tscrim]

Reviewer: Martin Rubey, Travis Scrimshaw

[tscrim]

Changed branch from u/mantepse/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series to u/tscrim/replace_lazy_power_series-32367

[mantepse]

comment:85

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?

[mantepse]

comment:86

I think I forgot to adapt the _element_constructor_ of all rings except for the LazyTaylorSeriesRing in https://github.com/sagemath/sagetrac-mirror/commit/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.

[mantepse]

Changed branch from u/tscrim/replace_lazy_power_series-32367 to u/mantepse/replace_lazy_power_series-32367

[mantepse]

comment:88

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

[mantepse]

Changed commit from 3bd9f8c to 555d1b0

[tscrim]

comment:89

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.

[mantepse]

comment:90

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.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 555d1b0 to baea68d

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

baea68d

pass functions using coefficients to element constructor

[mantepse]

comment:92

Sorry, I had to fix another thing.

But now - ready, set, go!

[tscrim]

comment:93

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.

[mantepse]

comment:94

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

[tscrim]

comment:95

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.

[mantepse]

comment:96

So, are you happy for the moment?

[tscrim]

comment:97

Indeed I am.

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

7745337

remove unused variable detected by pyflakes

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from baea68d to 7745337

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from 7745337 to cdad494

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

cdad494

do not advertise L(None) in favour of L.define()

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

e8c7636	advertise passing None to the constructor
f541f3d	add links to relaxed p-adics

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from cdad494 to f541f3d

[mantepse]

Description changed:

--- 
+++ 
@@ -1 +1,2 @@
-Replace the functionalities and add combinatorial meaning for the coefficients in the series
+In this ticket we finalize the implementation of lazy series, and replace the old lazy series framework.
+

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

Changed commit from f541f3d to 1df62c8

[7ed8c4ca-6d56-4ae9-953a-41e42b4ed313]

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

1df62c8

fix wrong ring in doctest

[mantepse]

comment:103

I start to hate doctests.

Let's try again.

[mantepse]

comment:104

YES!

[tscrim]

comment:105

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?

[mantepse]

comment:106

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

diff --git a/src/sage/rings/lazy_series.py b/src/sage/rings/lazy_series.py
index 4e2f35f2c3b..23992898376 100644
--- a/src/sage/rings/lazy_series.py
+++ b/src/sage/rings/lazy_series.py
@@ -2934,6 +2934,32 @@ class LazyLaurentSeries(LazyCauchyProductSeries):
         sage: f = 1 / (1 - z - z^2)
         sage: TestSuite(f).run()
     """
+    def _im_gens_(self, codomain, im_gens, base_map=None):
+        """
+        Returns the image of ``self`` under the map that sends the
+        generators of the parent of ``self`` to the elements of the
+        tuple ``im_gens``.
+
+        EXAMPLES::
+
+            sage: Z.<x> = ZZ[]
+            sage: K.<i> = NumberField(x^2 + 1)
+            sage: R.<t> = LazyLaurentSeriesRing(K)
+            sage: f = R(lambda n: i^n, valuation=-2)
+            sage: f
+            -t^-2 - i*t^-1 + 1 + i*t - t^2 - i*t^3 + t^4 + O(t^5)
+            sage: f._im_gens_(R, [t + t^2])
+            -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)
+
+            sage: cc = K.hom([-i])
+            sage: f._im_gens_(R, [t + t^2], base_map=cc)
+            -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)
+
+        """
+        if base_map is None:
+            return codomain(self(im_gens[0]))
+
+        return codomain(self.map_coefficients(base_map)(im_gens[0]))

     def __call__(self, g, *, check=True):
         r"""
@@ -3844,6 +3870,33 @@ class LazyPowerSeries(LazyCauchyProductSeries):
         deprecation(32367, "the method compute_coefficients obsolete and has no effect.")
         return

+    def _im_gens_(self, codomain, im_gens, base_map=None):
+        """
+        Returns the image of ``self`` under the map that sends the
+        generators of the parent of ``self`` to the elements of the
+        tuple ``im_gens``.
+
+        EXAMPLES::
+
+            sage: Z.<x> = QQ[]
+            sage: R.<q, t> = LazyPowerSeriesRing(Z)
+            sage: f = 1/(1-q-t)
+            sage: f
+            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
+            sage: S.<s> = LazyPowerSeriesRing(Z)
+            sage: f._im_gens_(S, [s, x*s])
+            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)
+
+            sage: cc = Z.hom([-x])
+            sage: f = 1/(1+x*q-t)
+            sage: f._im_gens_(S, [s, x*s], base_map=cc)
+            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)
+        """
+        if base_map is None:
+            return codomain(self(*im_gens))
+
+        return codomain(self.map_coefficients(base_map)(*im_gens))
+
     def __call__(self, *g, check=True):
         r"""
         Return the composition of ``self`` with ``g``.
diff --git a/src/sage/rings/lazy_series_ring.py b/src/sage/rings/lazy_series_ring.py
index acaef041566..7b8a001ca1d 100644
--- a/src/sage/rings/lazy_series_ring.py
+++ b/src/sage/rings/lazy_series_ring.py
@@ -1201,6 +1201,27 @@ class LazyPowerSeriesRing(LazySeriesRing):

             sage: L = LazyPowerSeriesRing(ZZ, 't')
             sage: TestSuite(L).run(skip=['_test_elements', '_test_associativity', '_test_distributivity', '_test_zero'])
+
+            sage: L = LazyPowerSeriesRing(ZZ, 's, t')
+            sage: TestSuite(L).run(skip=['_test_elements', '_test_associativity', '_test_distributivity', '_test_zero'])
+
+        Check that :trac:`34470` is fixed::
+
+            sage: L.<t> = LazyPowerSeriesRing(QQ)
+            sage: L in CompleteDiscreteValuationRings
+            True
+            sage: L.uniformizer()
+            t
+            sage: lcm(1/(1 - t^2) - 1, t)
+            t^2
+
+            sage: L.<t> = LazyPowerSeriesRing(ZZ)
+            sage: L in PrincipalIdealDomains
+            False
+
+            sage: L = LazyPowerSeriesRing(QQ, 's, t')
+            sage: L in PrincipalIdealDomains
+            False
         """
         from sage.structure.category_object import normalize_names
         names = normalize_names(-1, names)
@@ -1213,8 +1234,9 @@ class LazyPowerSeriesRing(LazySeriesRing):
         else:
             self._internal_poly_ring = PolynomialRing(self._laurent_poly_ring, "DUMMY_VARIABLE")
         category = Algebras(base_ring.category())
-        if base_ring in Fields():
+        if base_ring in Fields() and self._arity == 1:
             category &= CompleteDiscreteValuationRings()
+            self.uniformizer = lambda: self.gen()
         elif base_ring in IntegralDomains():
             category &= IntegralDomains()
         elif base_ring in Rings().Commutative():
@@ -1610,6 +1632,13 @@ class LazyCompletionGradedAlgebra(LazySeriesRing):
             sage: L = LazySymmetricFunctions(s)
             sage: TestSuite(L).run(skip=['_test_elements', '_test_associativity', '_test_distributivity', '_test_zero'])

+        Check that :trac:`34470` is fixed::
+
+            sage: s = SymmetricFunctions(QQ).s()
+            sage: L = LazySymmetricFunctions(s)
+            sage: L in PrincipalIdealDomains
+            False
+
         Check that a basis which is not graded is not enough::

             sage: ht = SymmetricFunctions(ZZ).ht()
@@ -1627,9 +1656,7 @@ class LazyCompletionGradedAlgebra(LazySeriesRing):
                 raise ValueError("basis should be in GradedAlgebrasWithBasis")
             self._arity = 1
         category = Algebras(base_ring.category())
-        if base_ring in Fields():
-            category &= CompleteDiscreteValuationRings()
-        elif base_ring in IntegralDomains():
+        if base_ring in IntegralDomains():
             category &= IntegralDomains()
         elif base_ring in Rings().Commutative():
             category = category.Commutative()

[tscrim]

comment:107

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 non-expected 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).

[tscrim]

Changed commit from 1df62c8 to 69d44ab

[tscrim]

Changed branch from u/mantepse/replace_lazy_power_series-32367 to u/tscrim/replace_lazy_power_series-32367

[tscrim]

New commits:

23d951f	Doing some reviewer changes.
6a3559e	Merge branch 'u/mantepse/implement_arithmetic_product_of_lazy_symmetric_functions' of https://github.com/sagemath/sagetrac-mirror into public/lazy_series/dvr_methods-TBA
6969549	Implementing methods for DVF.
91244f3	Merge branch 'public/lazy_series/dvr_methods-TBA' into u/tscrim/replace_lazy_power_series-32367
0a8b4c6	implement `_im_gens_`, part 1
380ddc9	adapt doctests
69d44ab	Removing old use of LazyTaylorSeriesRing from my changes.

[mantepse]

comment:109

Oh wow, thank you! I love self:!

[tscrim]

comment:110

But too much self: love is bad. :P

[mantepse]

comment:111

sage: L.<t> = LazyPowerSeriesRing(QQ)
sage: t/L(1)
t^2

[mantepse]

comment:112

The bug above was easy to fix (I'll do it in #34470, because that's where I noticed it), but the following garbage-in-garbage-out is really hard to detect:

sage: L.<t> = LazyPowerSeriesRing(QQ)
sage: t*((1+t)/(t-t^2))
1 + t + t^2 + O(t^3)
sage: t*(1+t)/(t-t^2)
1 + 2*t + 2*t^2 + 2*t^3 + O(t^4)