#23377 closed enhancement (fixed)

Clean up MPolynomialRing_generic.completion

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.1
Component: algebra Keywords:
Cc: tscrim Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 52e13d8 (Commits) Commit: 52e13d82d1fd973d523ec520387fbec10e3f32cc
Dependencies: Stopgaps:

Description

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.

Change History (18)

comment:1 Changed 23 months ago by jdemeyer

  • Authors set to Jeroen Demeyer

comment:2 Changed 23 months ago by jdemeyer

  • Branch set to u/jdemeyer/clean_up_mpolynomialring_generic_completion

comment:3 Changed 23 months ago by jdemeyer

  • Commit set to 8108128f84bd2148eff99fc40b9399340bbbdd93
  • Status changed from new to needs_review

New commits:

8108128Clean up MPolynomialRing_generic.completion

comment:4 follow-up: Changed 23 months ago by tscrim

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

comment:5 Changed 23 months ago by jdemeyer

  • Status changed from needs_review to needs_work

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

comment:7 Changed 21 months ago by git

  • Commit changed from 8108128f84bd2148eff99fc40b9399340bbbdd93 to 7cfb4afaa04d3469c8989b401648e89727614bb2

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

92bafe2Merge tag '8.1.beta2' into t/23377/clean_up_mpolynomialring_generic_completion
7cfb4afFix doc of completion()

comment:8 Changed 21 months ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:9 in reply to: ↑ 6 ; follow-ups: Changed 21 months ago by tscrim

  • Status changed from needs_review to needs_info

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'))?

comment:10 in reply to: ↑ 9 Changed 21 months ago by jdemeyer

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.

comment:11 follow-up: Changed 21 months ago by tscrim

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.

comment:12 in reply to: ↑ 11 ; follow-up: Changed 21 months ago by 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...

comment:13 in reply to: ↑ 9 ; follow-up: Changed 21 months ago by 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.

comment:14 Changed 21 months ago by git

  • Commit changed from 7cfb4afaa04d3469c8989b401648e89727614bb2 to 52e13d82d1fd973d523ec520387fbec10e3f32cc

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

52e13d8Add deprecation for completion(p=...)

comment:15 Changed 21 months ago by jdemeyer

  • Status changed from needs_info to needs_review

comment:16 in reply to: ↑ 12 Changed 21 months ago by tscrim

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. :/

comment:17 in reply to: ↑ 13 Changed 21 months ago by tscrim

  • Milestone changed from sage-8.0 to sage-8.1
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

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.

comment:18 Changed 21 months ago by vbraun

  • Branch changed from u/jdemeyer/clean_up_mpolynomialring_generic_completion to 52e13d82d1fd973d523ec520387fbec10e3f32cc
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.