Opened 8 months ago

Closed 8 months ago

#27931 closed defect (fixed)

fix LazyPowerSeriesRing in other variable than x

Reported by: dkrenn Owned by:
Priority: major Milestone: sage-8.8
Component: basic arithmetic Keywords:
Cc: Merged in:
Authors: Daniel Krenn Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: bf30d1f (Commits) Commit: bf30d1fc2abdae50e173e0274931d67399e4be37
Dependencies: Stopgaps:

Description

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

gives an error.

Change History (12)

comment:1 Changed 8 months ago by dkrenn

  • Branch set to u/dkrenn/lazy-power-series-in-z

comment:2 Changed 8 months ago by git

  • Commit set to 5695d3d844a9c5c89e136ffc5f1ce697e2b1c86d

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

c8cca02Trac #27931: fix handling of names in LazyPowerSeriesRing
f99a8cdTrac #27931: fix calling with other variable name (in LazyPowerSeriesRing)
07f9e47Trac #27931: add doctests
5695d3dTrac #27931: cleanup zero

comment:3 Changed 8 months ago by dkrenn

  • Authors set to Daniel Krenn
  • Status changed from new to needs_review

Did also some small cleanup.

comment:4 follow-up: Changed 8 months ago by 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.

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.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 8 months ago by 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.)

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.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 8 months ago by tscrim

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.

comment:7 in reply to: ↑ 6 Changed 8 months ago by dkrenn

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://git.sagemath.org/sage.git/commit/?h=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...

comment:8 follow-up: Changed 8 months ago by tscrim

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

comment:9 Changed 8 months ago by git

  • Commit changed from 5695d3d844a9c5c89e136ffc5f1ce697e2b1c86d to bf30d1fc2abdae50e173e0274931d67399e4be37

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

57ae6a7_zero_base_ring in parent
bf30d1fTrac #27931: fixup zero in parent

comment:10 in reply to: ↑ 8 Changed 8 months ago by dkrenn

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.

comment:11 Changed 8 months ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Let it be so.

comment:12 Changed 8 months ago by vbraun

  • Branch changed from u/dkrenn/lazy-power-series-in-z to bf30d1fc2abdae50e173e0274931d67399e4be37
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.