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: sage-9.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:

Status badges

Description (last modified by Martin Rubey)

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 Tejasvi Chebrolu

Branch: u/gh-tejasvicsr1/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series

comment:2 Changed 16 months ago by git

Commit: 0d56b0227713d7a6b26612593b311071a05782b8

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

651b372Revert "Merge branch 'u/gh-tejasvicsr1/special_functions_for_lazy_series' of git://trac.sagemath.org/sage into t/32324/lazy_taylor_series"
7c9a5ddfix invert
51357a0Small reworkings and fixes. Adding another composition test.
7501204Adding documentation to the user-facing class level documentation.
4723f6bRefactoring we discussed at the meeting today.
651c3edonly get coefficient if necessary
65e604dMerge branch 'u/mantepse/dense_lls-31897' of git://trac.sagemath.org/sage into t/32324/lazy_taylor_series
b332728fix and document series reversion
6a2391eMerge remote-tracking branch 'trac/u/mantepse/lazy_taylor_series' into t/32367/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series
0d56b02Merged new Taylor series and deleted new_gs.py

comment:3 Changed 16 months ago by Travis Scrimshaw

Keywords: gsoc2021 added; GSoC21 removed

comment:4 Changed 16 months ago by Travis Scrimshaw

Dependencies: #32324

comment:5 Changed 16 months ago by git

Commit: 0d56b0227713d7a6b26612593b311071a05782b839993e3ac56e0e26b69dd3a889c2aa8c030558fb

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

39993e3Fixed the initialising issue.

comment:6 Changed 16 months ago by git

Commit: 39993e3ac56e0e26b69dd3a889c2aa8c030558fbdb206803e6c2847bb01600f1ba63ba9539894408

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

73b9964Fixed naming issue.
db20680Changed _gs_iterator to callables

comment:7 Changed 16 months ago by git

Commit: db206803e6c2847bb01600f1ba63ba95398944087d491f6f7deee6cf4755c94442212366a0d96322

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

974fa4fChanged _list and _term methods.
6cfdf8dCreated restrict method in the series helper function.
eef15d8Changed the name of iterator to callable.
7d491f6Raised a not implemented error for all not corrected methods in the generating_series file.

comment:8 Changed 16 months ago by Martin Rubey

Branch: u/gh-tejasvicsr1/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_seriesu/mantepse/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series

comment:9 Changed 16 months ago by git

Commit: 7d491f6f7deee6cf4755c94442212366a0d96322a9084c6d28f3ecbfe544133741415731279df1b6

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

661c3d4backport two fixes from 32324
8a7dcd3fix oversight in shift
78b8fffLast trivial fix for clean pyflakes.
96771f0Merge branch 'u/tscrim/dense_lls-31897' of git://trac.sagemath.org/sage into t/32324/lazy_taylor_series
86925afinitial version of lazy symmetric functions
aa8a250initial and insanely stupid plethysm implementation
1a36f59add (currently failing) doctest for plethysm
03198a0fix bug in plethysm
eb3e88fmore doctests for composition and plethysm
a9084c6Merge 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 git

Commit: a9084c6d28f3ecbfe544133741415731279df1b64b76e93596f91a5068a5d2dbba31a65f748dea73

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

4b76e93switch to power sum by default

comment:11 Changed 16 months ago by Tejasvi Chebrolu

Branch: u/mantepse/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_seriesu/gh-tejasvicsr1/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series

comment:12 Changed 16 months ago by git

Commit: 4b76e93596f91a5068a5d2dbba31a65f748dea7348e8e2686f68a911c5a705b91971e0f3d5924113

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

48e8e26Changed more methods.

comment:13 Changed 16 months ago by git

Commit: 48e8e2686f68a911c5a705b91971e0f3d592411378a85aeeaeabf44e6a6ebd9c58a486cebf373eca

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

78a85aeSome more work in generating series.

comment:14 Changed 12 months ago by Matthias Köppe

Milestone: sage-9.5sage-9.6

comment:15 Changed 8 months ago by Matthias Köppe

Milestone: sage-9.6sage-9.7

comment:16 Changed 4 months ago by Martin Rubey

Branch: u/gh-tejasvicsr1/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_seriesu/mantepse/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series

comment:17 Changed 4 months ago by git

Commit: 78a85aeeaeabf44e6a6ebd9c58a486cebf373ecad4f2e5cc8b49b70138ab1f56fb0062d22c83491c

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

e4479edfix missing space
3da6b3dbetter docstring
8798139Adding a slightly more complicated doctest and other small tweaks.
75e3f3bMerge branch 'public/rings/laurent_quo_rem_bug-34330' into public/rings/lazy_talyor_series-32324
c74fd71improve documentation, move options to abstract base class
9d6579bimprove documentation, move zero, one, characteristic, etc. to ABC
feba6b8Working more on __call__ for LazySymFunc.
3f3e0f2Merge branch 'public/rings/lazy_talyor_series-32324' of https://github.com/sagemath/sagetrac-mirror into public/rings/lazy_talyor_series-32324
6727228Merge branch 'public/rings/lazy_talyor_series-32324' of trac.sagemath.org:sage into t/32324/public/rings/lazy_talyor_series-32324
d4f2e5cMerge branch 'public/rings/lazy_talyor_series-32324' 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 git

Commit: d4f2e5cc8b49b70138ab1f56fb0062d22c83491c1b34e62a503050fd50c831d87746f5740a0df92d

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

028796dFixing numerous issues with __call__ and expanding its functionality. Moving plethysm to a Stream_plethysm.
9fb155fRemoving unused code from previous version.
1b34e62Merge branch 'public/rings/lazy_talyor_series-32324' 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 git

Commit: 1b34e62a503050fd50c831d87746f5740a0df92d5ba18e80f4f99f03393056a33cfeb42b413e9066

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

7f9dbb1Some last doc fixes and tweaks.
4e03feeremove unused local variable
e780472Addressing the linter complaint.
5ba18e8Merge branch 'public/rings/lazy_talyor_series-32324' of trac.sagemath.org:sage into t/32367/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_series

comment:20 Changed 4 months ago by Travis Scrimshaw

Is this ready for review?

comment:21 Changed 4 months ago by Martin Rubey

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 git

Commit: 5ba18e80f4f99f03393056a33cfeb42b413e90661e8f69dfeab7f92a5579fbcb488f6759cedc569f

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

50ce450derivative for LazyTaylorSeries
10cfc11derivative for LazySymmetricFunctions
7be0b86functorial composition for LazySymmetricFunctions
c755d71implement arithmetic product for LazySymmetricFunction
c36796eMerge 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
18429efdo not use change_ring, because it is brittle, and take care of zero coefficients which are not symmetric functions
481d78dMerge 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
b469431do not use change_ring, because it is brittle
56ee69eMerge 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
1e8f69dremove LazyPowerSeries and Stream

comment:23 Changed 3 months ago by git

Commit: 1e8f69dfeab7f92a5579fbcb488f6759cedc569fd75ac136b99030723109a279f29534d9cbedb726

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

e1e7f0dMerge branch 'u/mantepse/implement_derivatives_of_lazy_series' of trac.sagemath.org:sage into t/34473/remove_rings_from_streams
6010361remove ring argument from Stream_map_coefficient and Stream_function
aa15394Merge branch 'u/mantepse/remove_rings_from_streams' of trac.sagemath.org:sage into t/34422/implement_functorial_composition_of_lazy_symmetric_functiosn
760372aaddress reviewer's comments
8ca3624polish functorial composition
b2ef1e4allow SymmetricFunctions as arguments of functorial composition
83e7dd3Merge 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
d22f87aadapt code after merge
f2a3cafreimplement arithmetic_product by using the symmetric function version (which is more robust
d75ac13Merge 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 git

Commit: d75ac136b99030723109a279f29534d9cbedb726293b4392f318a6a6a487dc87fa8c8d64864fbf23

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

293b439adapt doctests

comment:25 Changed 3 months ago by Martin Rubey

Status: newneeds_review

A small step but a giant leap!

comment:26 Changed 3 months ago by Martin Rubey

Dependencies: #32324#34423

comment:27 Changed 3 months ago by Martin Rubey

Authors: Tejasvi ChebroluTejasvi Chebrolu, Martin Rubey, Travis Scrimshaw

comment:28 Changed 3 months ago by Travis Scrimshaw

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 git

Commit: 293b4392f318a6a6a487dc87fa8c8d64864fbf2389270ebb1e49759628c1db69de93744a0d3b29df

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

8b6a752no period at end of input description, break long doctest
f88a5c8fix arithmetic product with 0 and finite support
89270ebMerge 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 Martin Rubey

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 Martin Rubey

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 Martin Rubey

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 Martin Rubey

Similarly, also in src/sage/tests/books/computational-mathematics-with-sagemath/, 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 Martin Rubey

Cc: Paul Zimmermann added
Status: needs_reviewneeds_info

comment:35 Changed 3 months ago by Travis Scrimshaw

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 Martin Rubey

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 non-zero 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 Travis Scrimshaw

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 git

Commit: 89270ebb1e49759628c1db69de93744a0d3b29dfb76d8f40ac50839297cb25fe23da16c0c0a8c49c

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

b76d8f4allow to define lazy series using iterators, adapt doctest

comment:39 Changed 3 months ago by Martin Rubey

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/(1-x)^3)[:5]
[1, 3, 6]
sage: L(lambda n: n*(n-1)/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 Thierry Monteil

Cc: Thierry Monteil added

comment:41 Changed 3 months ago by git

Commit: b76d8f40ac50839297cb25fe23da16c0c0a8c49c4eb4dbd483cbac03078e0d709a8487c4f18ad219

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

4eb4dbdfix behaviour of __getitem__ for slices, provide missing doctests, minor simplifications in species directory

comment:42 Changed 3 months ago by git

Commit: 4eb4dbd483cbac03078e0d709a8487c4f18ad2196f76b00e9f2aa3e39118b8278349f48de0753c05

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

6f76b00replace LazyTaylorSeriesRing and LazyTaylorSeries with LazyPowerSeriesRing and LazyPowerSeries and add appropriate deprecations

comment:43 Changed 3 months ago by Martin Rubey

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 in reply to:  39 ; Changed 3 months ago by Travis Scrimshaw

Replying to Martin Rubey:

I think that for LazyTaylorSeries, LazyDirichletSeries, LazySymmetricFunction, etc. we really don't want

sage: L.<x> = LazyTaylorSeriesRing(QQ)
sage: (x^2/(1-x)^3)[:5]
[1, 3, 6]
sage: L(lambda n: n*(n-1)/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 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`.

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 is sparse=False, whereas in all other cases, it is sparse=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 had L([1,2,3]) what would now be L([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 in reply to:  44 ; Changed 3 months ago by Martin Rubey

Replying to Travis Scrimshaw:

Replying to Martin Rubey:

I think that for LazyTaylorSeries, LazyDirichletSeries, LazySymmetricFunction, etc. we really don't want

sage: L.<x> = LazyTaylorSeriesRing(QQ)
sage: (x^2/(1-x)^3)[:5]
[1, 3, 6]
sage: L(lambda n: n*(n-1)/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 had L([1,2,3]) what would now be L([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 non-zero 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 git

Commit: 6f76b00e9f2aa3e39118b8278349f48de0753c05f5e3f2ef6f0c1757881e16c1563833cf119f4a83

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

f5e3f2efix documentation

comment:47 Changed 3 months ago by Martin Rubey

I added a warning for the new behaviour concerning the specification of the constant.

comment:48 Changed 3 months ago by git

Commit: f5e3f2ef6f0c1757881e16c1563833cf119f4a83c5bfba5f8e8d41bc6341b7a490ae56469e2adcae

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

c5bfba5adapt a doctest

comment:49 in reply to:  45 ; Changed 3 months ago by Travis Scrimshaw

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 backwards-incompatible 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 a NotImplementedError for multivariate series.

I think it is better to be consistent about this.

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.

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 non-zero 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 of l since it has a chance to be read as I 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 class-level ring doc, its _element_constructor_, the element class’s class-level doc; the module-level doc if there are examples there).

comment:50 in reply to:  49 ; Changed 3 months ago by Martin Rubey

Replying to Travis Scrimshaw:

  • Use L instead of l since it has a chance to be read as I 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 class-level ring doc, its _element_constructor_, the element class’s class-level doc; the module-level 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 git

Commit: c5bfba5f8e8d41bc6341b7a490ae56469e2adcaeceecc89392820fbbf07f2ad1b9915ac02ac4d6d4

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

ceecc89don't use l

comment:52 Changed 3 months ago by Frédéric Chapoton

Hello, we are supposed to be in maintenance mode today, as announced on sage-devel and trac main page. I think everything is now in order, but please try clicking around..

comment:53 Changed 3 months ago by Travis Scrimshaw

Status: needs_infoneeds_review

Everything seems to be okay.

comment:54 Changed 3 months ago by git

Commit: ceecc89392820fbbf07f2ad1b9915ac02ac4d6d49629b2878b228fb2c5623f4d2b1df60e2c4bb287

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

9629b28require the start value for __getitem__ of LazyLaurentSeries

comment:55 in reply to:  50 Changed 3 months ago by Travis Scrimshaw

Replying to Martin Rubey:

Replying to Travis Scrimshaw:

  • Use L instead of l since it has a chance to be read as I 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.

Thank you.

  • Make sure there is an easy to see example of the new constant behavior is clearly visible (in the class-level ring doc, its _element_constructor_, the element class’s class-level doc; the module-level 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 Travis Scrimshaw

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 Changed 3 months ago by Martin Rubey

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 Changed 3 months ago by Martin Rubey

Actually, I just discovered something quite disturbing:

sage: L.<x> = PowerSeriesRing(QQ)
sage: f = x^-5/(1-2*x)
sage: f[:10]
<ipython-input-11-f66b5b8a9c22>: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.

Last edited 3 months ago by Martin Rubey (previous) (diff)

comment:59 in reply to:  57 Changed 3 months ago by Travis Scrimshaw

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 in reply to:  58 Changed 3 months ago by Travis Scrimshaw

Replying to Martin Rubey:

Actually, I just discovered something quite disturbing:

sage: L.<x> = PowerSeriesRing(QQ)
sage: f = x^-5/(1-2*x)
sage: f[:10]
<ipython-input-11-f66b5b8a9c22>: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 Changed 3 months ago by Martin Rubey

I am absolutely against starting with _approximate_order, since this is not well-defined.

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 Martin Rubey

(Let me stress again that _approximate_order may change after certain operations, and may depend on the way we construct the series.)

comment:63 in reply to:  61 Changed 3 months ago by Travis Scrimshaw

Replying to Martin Rubey:

I am absolutely against starting with _approximate_order, since this is not well-defined.

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:

  1. For f[:stop], then get the list of coefficients from f[a] to f[stop-1] and strip the leading zeros and update the approximate valuation as appropriate.
  2. 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):

  1. g[:5] returns [2, 3, 4] and sets the approximate valuation to 2.
  2. g[-1:5] returns [0, 0, 0, 2, 3, 4].
  3. g[:1] returns [] and sets the approximate valuation to 1.

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 Martin Rubey

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 polynomial-like __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 Travis Scrimshaw

Hmm...that is a bit tricky. Can you just concatenate the coefficients() of each of the components by degree?

comment:66 Changed 3 months ago by Martin Rubey

I'm almost there :-)

comment:67 Changed 3 months ago by git

Commit: 9629b2878b228fb2c5623f4d2b1df60e2c4bb287037f3f7cad9b7bfb955f9cb3d1aaf7a4363c7ccb

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

037f3f7implement coefficients

comment:68 Changed 3 months ago by git

Commit: 037f3f7cad9b7bfb955f9cb3d1aaf7a4363c7ccb787197ed609f8291ba23d2c102686b2d8353ec9d

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

787197efix some bugs

comment:69 Changed 3 months ago by git

Commit: 787197ed609f8291ba23d2c102686b2d8353ec9d8f46e127e30e1f0a3e6d1a6997e806baf7254710

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

8f46e12implement new semantics of __getitem__

comment:70 Changed 3 months ago by Martin Rubey

Ready!

comment:71 Changed 3 months ago by git

Commit: 8f46e127e30e1f0a3e6d1a6997e806baf7254710f706c53ec1c8d197b845488d763a975b886d1354

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

0de1c13add a warning and an example for the arithmetic product with a constant term
8e5de35add missing whitespace in docstring
f706c53Merge 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 Travis Scrimshaw

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 Martin Rubey

Re stop: Oh, yes, I agree! I overlooked that!

Re base ring: indeed. (I thought they did.)

comment:74 Changed 3 months ago by 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.

comment:75 Changed 3 months ago by git

Commit: f706c53ec1c8d197b845488d763a975b886d13543410eda2266a4ed0c38dc8ae053bd13194e26d62

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

3410edafix map_coefficients

comment:76 Changed 3 months ago by git

Commit: 3410eda2266a4ed0c38dc8ae053bd13194e26d628fc2dd1a5c5de7f393de048c920e1d49be4660ba

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

8fc2dd1fix doctests because of __getitem__ change

comment:77 Changed 3 months ago by git

Commit: 8fc2dd1a5c5de7f393de048c920e1d49be4660ba2a3389e6ae2c5be15a44f52d4b2fb339e63bc827

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

2a3389ecompute no more elements than necessary in __getitem__

comment:78 in reply to:  74 Changed 3 months ago by Travis Scrimshaw

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 git

Commit: 2a3389e6ae2c5be15a44f52d4b2fb339e63bc82789d9cccd0e5db988005e3503f86efd7283fea038

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

89d9cccfix pyflakes warnings

comment:80 Changed 3 months ago by Martin Rubey

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): 
    15781578        if basis in Algebras.TensorProducts:
    15791579            self._arity = len(basis._sets)
    15801580        else:
    1581             if basis not in Algebras.Graded:
     1581            if basis not in GradedAlgebrasWithBasis
    15821582                raise ValueError("basis should be a graded algebra")
    15831583            self._arity = 1
    15841584        category = Algebras(base_ring.category())

comment:81 Changed 3 months ago by Travis Scrimshaw

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

comment:82 Changed 3 months ago by git

Commit: 89d9cccd0e5db988005e3503f86efd7283fea038cf2b0cb9456298c279d7270d4c44e208929370e4

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

cf2b0cbrequire a graded basis

comment:83 Changed 3 months ago by Martin Rubey

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 Travis Scrimshaw

Branch: u/mantepse/replace_lazy_power_series_in_species_directory_with_the_new_lazy_taylor_seriesu/tscrim/replace_lazy_power_series-32367
Commit: cf2b0cb9456298c279d7270d4c44e208929370e43bd9f8c26726786ff40db79486a1cb0b3cd58b00
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:

c90cb91Merge 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
3bd9f8cFixing coefficients bug and some doc tweaks.

comment:85 Changed 3 months ago by Martin Rubey

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 Martin Rubey

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 Martin Rubey

Branch: u/tscrim/replace_lazy_power_series-32367u/mantepse/replace_lazy_power_series-32367

comment:88 Changed 3 months ago by Martin Rubey

Commit: 3bd9f8c26726786ff40db79486a1cb0b3cd58b00555d1b07df4f0ba82f23392142037114c03cd0e9

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:

555d1b0fix LazyPowerSeries._element_constructor_ when passed a function

comment:89 Changed 3 months ago by Travis Scrimshaw

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 Changed 3 months ago by 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.

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 git

Commit: 555d1b07df4f0ba82f23392142037114c03cd0e9baea68dbd79e0a0774f2763c12aa776a01a5f0c5

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

baea68dpass functions using coefficients to element constructor

comment:92 Changed 3 months ago by Martin Rubey

Sorry, I had to fix another thing.

But now - ready, set, go!

comment:93 in reply to:  90 ; Changed 3 months ago by Travis Scrimshaw

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 in reply to:  93 ; Changed 3 months ago by 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.

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 in reply to:  94 Changed 3 months ago by Travis Scrimshaw

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:96 Changed 3 months ago by Martin Rubey

So, are you happy for the moment?

comment:97 Changed 3 months ago by Travis Scrimshaw

Indeed I am.

comment:98 Changed 3 months ago by git

Commit: baea68dbd79e0a0774f2763c12aa776a01a5f0c57745337b260ad2e37c1c30b04756606e0e775610

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

7745337remove unused variable detected by pyflakes

comment:99 Changed 3 months ago by git

Commit: 7745337b260ad2e37c1c30b04756606e0e775610cdad494f9eb5f409f987301f4a8fd073c846f1b2

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

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

comment:100 Changed 3 months ago by git

Commit: cdad494f9eb5f409f987301f4a8fd073c846f1b2f541f3d184e8b2468da5c2953383a4979996ec5c

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

e8c7636advertise passing None to the constructor
f541f3dadd links to relaxed p-adics

comment:101 Changed 3 months ago by Martin Rubey

Description: modified (diff)
Summary: Replace Lazy Power Series in species directory with the new Lazy Taylor SeriesReplace Lazy Power Series in species directory

comment:102 Changed 3 months ago by git

Commit: f541f3d184e8b2468da5c2953383a4979996ec5c1df62c8481a2bf52542f5e50a6b5d8610a8ac4b9

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

1df62c8fix wrong ring in doctest

comment:103 Changed 3 months ago by Martin Rubey

I start to hate doctests.

Let's try again.

comment:104 Changed 3 months ago by Martin Rubey

YES!

comment:105 Changed 3 months ago by Travis Scrimshaw

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 Changed 3 months ago by 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?

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): 
    29342934        sage: f = 1 / (1 - z - z^2)
    29352935        sage: TestSuite(f).run()
    29362936    """
     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]))
    29372963
    29382964    def __call__(self, g, *, check=True):
    29392965        r"""
    class LazyPowerSeries(LazyCauchyProductSeries): 
    38443870        deprecation(32367, "the method compute_coefficients obsolete and has no effect.")
    38453871        return
    38463872
     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/(1-q-t)
     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*q-t)
     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
    38473900    def __call__(self, *g, check=True):
    38483901        r"""
    38493902        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): 
    12011201
    12021202            sage: L = LazyPowerSeriesRing(ZZ, 't')
    12031203            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
    12041225        """
    12051226        from sage.structure.category_object import normalize_names
    12061227        names = normalize_names(-1, names)
    class LazyPowerSeriesRing(LazySeriesRing): 
    12131234        else:
    12141235            self._internal_poly_ring = PolynomialRing(self._laurent_poly_ring, "DUMMY_VARIABLE")
    12151236        category = Algebras(base_ring.category())
    1216         if base_ring in Fields():
     1237        if base_ring in Fields() and self._arity == 1:
    12171238            category &= CompleteDiscreteValuationRings()
     1239            self.uniformizer = lambda: self.gen()
    12181240        elif base_ring in IntegralDomains():
    12191241            category &= IntegralDomains()
    12201242        elif base_ring in Rings().Commutative():
    class LazyCompletionGradedAlgebra(LazySeriesRing): 
    16101632            sage: L = LazySymmetricFunctions(s)
    16111633            sage: TestSuite(L).run(skip=['_test_elements', '_test_associativity', '_test_distributivity', '_test_zero'])
    16121634
     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
    16131642        Check that a basis which is not graded is not enough::
    16141643
    16151644            sage: ht = SymmetricFunctions(ZZ).ht()
    class LazyCompletionGradedAlgebra(LazySeriesRing): 
    16271656                raise ValueError("basis should be in GradedAlgebrasWithBasis")
    16281657            self._arity = 1
    16291658        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():
    16331660            category &= IntegralDomains()
    16341661        elif base_ring in Rings().Commutative():
    16351662            category = category.Commutative()

comment:107 in reply to:  106 Changed 3 months ago by Travis Scrimshaw

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

comment:108 Changed 3 months ago by Travis Scrimshaw

Branch: u/mantepse/replace_lazy_power_series-32367u/tscrim/replace_lazy_power_series-32367
Commit: 1df62c8481a2bf52542f5e50a6b5d8610a8ac4b969d44abf4ce2ec4fb652ec8e39dac42cd044537f

New commits:

23d951fDoing some reviewer changes.
6a3559eMerge branch 'u/mantepse/implement_arithmetic_product_of_lazy_symmetric_functions' of https://github.com/sagemath/sagetrac-mirror into public/lazy_series/dvr_methods-TBA
6969549Implementing methods for DVF.
91244f3Merge branch 'public/lazy_series/dvr_methods-TBA' into u/tscrim/replace_lazy_power_series-32367
0a8b4c6implement _im_gens_, part 1
380ddc9adapt doctests
69d44abRemoving old use of LazyTaylorSeriesRing from my changes.

comment:109 Changed 3 months ago by Martin Rubey

Oh wow, thank you! I love self:!

comment:110 Changed 3 months ago by Travis Scrimshaw

But too much self: love is bad. :P

comment:111 Changed 3 months ago by Martin Rubey

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

comment:112 Changed 3 months ago by Martin Rubey

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)

comment:113 Changed 3 months ago by Travis Scrimshaw

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 Martin Rubey

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 Travis Scrimshaw

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 short-term, I propose we raise an NotImplementedError whenever the denominator is known to have a positive valuation.

comment:116 Changed 3 months ago by Martin Rubey

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.

Last edited 3 months ago by Martin Rubey (previous) (diff)

comment:117 Changed 3 months ago by Travis Scrimshaw

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 Martin Rubey

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 Martin Rubey

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 Travis Scrimshaw

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 Martin Rubey

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 git

Commit: 69d44abf4ce2ec4fb652ec8e39dac42cd044537f3d5b8cf4c836ceb8813aa6311fe18b1f5fbffc6a

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

3d5b8cfLast little things from the patchbot.

comment:123 Changed 3 months ago by Travis Scrimshaw

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 Matthias Köppe

Milestone: sage-9.7sage-9.8

comment:125 Changed 3 months ago by Martin Rubey

The Returns in the doctring noticed by the patchbot is fixed in the follow-up 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 Martin Rubey

Status: needs_reviewpositive_review

comment:127 Changed 2 months ago by Martin Rubey

Branch: u/tscrim/replace_lazy_power_series-32367u/mantepse/replace_lazy_power_series-32367

comment:128 Changed 2 months ago by git

Commit: 3d5b8cf4c836ceb8813aa6311fe18b1f5fbffc6a4172688e381f6b8e617bcd1a7583aec50f346307
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. Last 10 new commits:

5d76825Merge branch 'u/chapoton/34494' of https://github.com/sagemath/sagetrac-mirror into u/tscrim/derivatives_lazy_series-34413
97df300Updating sf/sfa.py doctest due to #34494.
1388b8aMerge branch 'u/chapoton/34494' of https://github.com/sagemath/sagetrac-mirror into public/rings/lazy_series_revert-34383
7bfe5f7Merge branch 'public/rings/lazy_series_revert-34383' of https://github.com/sagemath/sagetrac-mirror into public/rings/lazy_series_revert-34383
2039990Updating doctest due to changes from #34494.
cdea820Merge branch 'public/rings/lazy_series_revert-34383' of https://github.com/sagemath/sagetrac-mirror into u/tscrim/derivatives_lazy_series-34413
b7f04edMerge branch 'develop' of trac.sagemath.org:sage into t/34413/derivatives_lazy_series-34413
2325826Merge branch 'u/mantepse/derivatives_lazy_series-34413' of trac.sagemath.org:sage into t/34422/implement_functorial_composition_of_lazy_symmetric_functiosn
ebcf5c3Merge 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
4172688Merge branch 'u/mantepse/implement_arithmetic_product_of_lazy_symmetric_functions' of trac.sagemath.org:sage into t/32367/replace_lazy_power_series-32367

comment:129 Changed 2 months ago by Martin Rubey

Status: needs_reviewpositive_review

trivial (autopmatic) merge, necessary to make the patchbots happy.

comment:130 Changed 2 months ago by git

Commit: 4172688e381f6b8e617bcd1a7583aec50f3463074fc981b458f8d628684decf221618911ace06333
Status: positive_reviewneeds_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

67dff95Merge branch 'develop' of trac.sagemath.org:sage into t/32367/replace_lazy_power_series-32367
4fc981bfix for pyflakes and blocks

comment:131 Changed 2 months ago by Martin Rubey

Status: needs_reviewpositive_review

trivial fixes to make patchbot happy.

comment:132 Changed 2 months ago by Volker Braun

Branch: u/mantepse/replace_lazy_power_series-323674fc981b458f8d628684decf221618911ace06333
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.