Clean up MPolynomialRing_generic.completion

[jdemeyer]

In src/sage/rings/polynomial/multi_polynomial_ring_generic.pyx, the method completion() constructs a PowerSeriesRing either from variable names or from an polynomial ring element. The latter must be changed for #10483. Moreover, the error checking also needs to be improved.

CC: @tscrim

Component: algebra

Author: Jeroen Demeyer

Branch/Commit: 52e13d8

Reviewer: Travis Scrimshaw

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

[jdemeyer]

Author: Jeroen Demeyer

[jdemeyer]

Branch: u/jdemeyer/clean_up_mpolynomialring_generic_completion

[jdemeyer]

Commit: 8108128

[jdemeyer]

New commits:

8108128

Clean up MPolynomialRing_generic.completion

[tscrim]

comment:4

The doc for completion is no longer correct (it mentions p). Also, do we want to support R.completion('x,y')?

[jdemeyer]

comment:6

Replying to @tscrim:

Also, do we want to support R.completion('x,y')?

Not sure. So far, there doesn't seem to be a need.

[sagetrac-git]

Changed commit from 8108128 to 7cfb4af

[sagetrac-git]

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

92bafe2	Merge tag '8.1.beta2' into t/23377/clean_up_mpolynomialring_generic_completion
7cfb4af	Fix doc of completion()

[tscrim]

comment:9

Replying to @jdemeyer:

Replying to @tscrim:

Also, do we want to support R.completion('x,y')?

Not sure. So far, there doesn't seem to be a need.

I ask because it feels like we should pass names off to normalize_names, which will then support that semantic. Is there a reason why you are not using that?

Also, could we add the @rename_keyword(deprecation=23377, p='names') in order to avoid backwards incompatible changes (just in case someone in the wild is explicitly doing R.completion(p='x'))?

[jdemeyer]

comment:10

Replying to @tscrim:

just in case someone in the wild is explicitly doing R.completion(p='x'))?

Python should really invent syntax for positional-only arguments. They have keyword-only arguments now in Python 3, but why not positional-only arguments? My favourite consequence:

sage: R.<self,other> = QQ[[]]
sage: a = R.random_element(); a
-3/4*self^4 + 12*self^5*other - 3/2*self^4*other^3 + self^3*other^7 + 1/2*self^6*other^5 + O(self, other)^12
sage: a(self=1)
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-13-6b78fae4891c> in <module>()
----> 1 a(self=Integer(1))

TypeError: __call__() got multiple values for keyword argument 'self'

Anyway... I'll add the deprecation since you asked.

[tscrim]

comment:11

Python does have a way for positional only arguments. You just have to do foo(self, *args, **kwds) and manually handle keywords. However, that is indeed a fun examples.

[jdemeyer]

comment:12

Replying to @tscrim:

Python does have a way for positional only arguments. You just have to do foo(self, *args, **kwds) and manually handle keywords.

Yes, that works but it's not a very satisfactory solution...

[jdemeyer]

comment:13

Replying to @tscrim:

I ask because it feels like we should pass names off to normalize_names

There is a different here for names than the PolynomialRing constructor for example: the names are not arbitrary names, they refer to an element of self. The fact that we actually allow to give an element, not just a string, is another example of that.

I would argue that giving an element is actually recommend, because it makes sense mathematically: you have a ring and you are completing it w.r.t. a given element from that ring.

For me, the main reason to allow strings is to allow the C.<...> = R.completion() syntax.

[sagetrac-git]

Changed commit from 7cfb4af to 52e13d8

[sagetrac-git]

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

52e13d8

Add deprecation for completion(p=...)

[tscrim]

comment:16

Replying to @jdemeyer:

Replying to @tscrim:

Python does have a way for positional only arguments. You just have to do foo(self, *args, **kwds) and manually handle keywords.

Yes, that works but it's not a very satisfactory solution...

I agree completely, but its the constraints we must live in. :/

[tscrim]

Reviewer: Travis Scrimshaw

[tscrim]

comment:17

Replying to @jdemeyer:

Replying to @tscrim:

I ask because it feels like we should pass names off to normalize_names

There is a different here for names than the PolynomialRing constructor for example: the names are not arbitrary names, they refer to an element of self. The fact that we actually allow to give an element, not just a string, is another example of that.

I would argue that giving an element is actually recommend, because it makes sense mathematically: you have a ring and you are completing it w.r.t. a given element from that ring.

For me, the main reason to allow strings is to allow the C.<...> = R.completion() syntax.

Ah, I see. LGTM. Thanks.

[vbraun]

Changed branch from u/jdemeyer/clean_up_mpolynomialring_generic_completion to 52e13d8