fix LazyPowerSeriesRing in other variable than x

[dkrenn]

sage: LazyPowerSeriesRing(QQ, 'z').gen()

gives an error.

Component: basic arithmetic

Author: Daniel Krenn

Branch/Commit: bf30d1f

Reviewer: Travis Scrimshaw

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

[dkrenn]

Branch: u/dkrenn/lazy-power-series-in-z

[sagetrac-git]

Commit: 5695d3d

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

c8cca02	Trac #27931: fix handling of names in LazyPowerSeriesRing
f99a8cd	Trac #27931: fix calling with other variable name (in LazyPowerSeriesRing)
07f9e47	Trac #27931: add doctests
5695d3d	Trac #27931: cleanup zero

[dkrenn]

Author: Daniel Krenn

[dkrenn]

comment:3

Did also some small cleanup.

[tscrim]

comment:4

I think we should actually go a bit further and make thing proper. Set the class-level attribute Element and deprecate the element_class input.

So there is a reason for storing self._zero, each of those indirections self.parent().base_ring().zero() is a (python?) function call, which is relatively slow. While I am not entirely sure this makes that much of a difference, it does seem to be called with some frequency, thus it might have some non-trivial effect. There is no real harm in having this (hidden) attribute IMO.

[dkrenn]

comment:5

Replying to @tscrim:

I think we should actually go a bit further and make thing proper. Set the class-level attribute Element and deprecate the element_class input.

Yes, I agree. Tried that yesterday while working on this ticket, and then gave up after an hour or so... (There were a lot of strange error messages, infinite recursion loops etc. That all not in series.py which worked perfectly, but in species.py and generating_series.py which I tried to adapt as well.)

So there is a reason for storing self._zero, each of those indirections self.parent().base_ring().zero() is a (python?) function call, which is relatively slow.

Really?....my feeling was that storing zero in each element seems somehow a waste.

What if we store it in the parent and call it by self._parent._zero_base_ring or so?

While I am not entirely sure this makes that much of a difference, it does seem to be called with some frequency, thus it might have some non-trivial effect. There is no real harm in having this (hidden) attribute IMO.

Getting this zero should clearly be done via the parent as it is not something that depends on the particular element.

[tscrim]

comment:6

Replying to @dkrenn:

Replying to @tscrim:

I think we should actually go a bit further and make thing proper. Set the class-level attribute Element and deprecate the element_class input.

Yes, I agree. Tried that yesterday while working on this ticket, and then gave up after an hour or so... (There were a lot of strange error messages, infinite recursion loops etc. That all not in series.py which worked perfectly, but in species.py and generating_series.py which I tried to adapt as well.)

I see the problem: It is using the old-style parent still. This is something that should be fixed, but that would be much more invasive than should be on this ticket.

So there is a reason for storing self._zero, each of those indirections self.parent().base_ring().zero() is a (python?) function call, which is relatively slow.

Really?....my feeling was that storing zero in each element seems somehow a waste.

What if we store it in the parent and call it by self._parent._zero_base_ring or so?

While I am not entirely sure this makes that much of a difference, it does seem to be called with some frequency, thus it might have some non-trivial effect. There is no real harm in having this (hidden) attribute IMO.

Getting this zero should clearly be done via the parent as it is not something that depends on the particular element.

This is just one pointer, so it is such a small thing in terms of memory for something that is used with some frequency by the element. So I think there is a benefit to doing this here. However, my suspicion is that this is not going to matter too much in terms of "real-world" computations. If you do put it in the parent with simple attribute indirections, then that is a good compromise. I don't have too strong of an opinion on this matter.

[dkrenn]

comment:7

Replying to @tscrim:

This is just one pointer, so it is such a small thing in terms of memory for something that is used with some frequency by the element. So I think there is a benefit to doing this here. However, my suspicion is that this is not going to matter too much in terms of "real-world" computations. If you do put it in the parent with simple attribute indirections, then that is a good compromise. I don't have too strong of an opinion on this matter.

So somehow I am not very successful today... In
https://github.com/sagemath/sagetrac-mirror/commits/57ae6a712e9929247f8bcd2e420b66fba5e83404
I get

AttributeError: type object 'LazyPowerSeriesRing_with_category' has no attribute '_zero_base_ring'

and don't see why. (I got a problem when using _parent, but I though it might was because of Cythonizing. But now I get it for this argument as well.)
Probably I just overlook something. I also consider simply reverting the zero change and get this ticket done...

[tscrim]

comment:8

Because zero is a method of the parent:

@@ -268,7 +269,7 @@ class LazyPowerSeriesRing(Algebra):
             sage: L.zero()
             0
         """
-        return self.term(self.base_ring().zero(), 0)
+        return self.term(self.parent()._zero_base_ring, 0)
 
     def identity_element(self):
         """

So change this to return self.term(self._zero_base_ring, 0).

[sagetrac-git]

Changed commit from 5695d3d to bf30d1f

[sagetrac-git]

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

57ae6a7	_zero_base_ring in parent
bf30d1f	Trac #27931: fixup zero in parent

[dkrenn]

comment:10

Replying to @tscrim:

Because zero is a method of the parent:
[...]
So change this to return self.term(self._zero_base_ring, 0).

Of course, I was inpatent and overlooked this. Thank you. Fixed now.

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:11

Let it be so.

[vbraun]

Changed branch from u/dkrenn/lazy-power-series-in-z to bf30d1f