Opened 4 years ago
Closed 4 years ago
#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, GitHub, GitLab) | 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 4 years ago by
comment:2 Changed 4 years ago by
- Branch set to u/jdemeyer/clean_up_mpolynomialring_generic_completion
comment:3 Changed 4 years ago by
- Commit set to 8108128f84bd2148eff99fc40b9399340bbbdd93
- Status changed from new to needs_review
comment:4 follow-up: ↓ 6 Changed 4 years ago by
The doc for completion is no longer correct (it mentions p
). Also, do we want to support R.completion('x,y')
?
comment:5 Changed 4 years ago by
- Status changed from needs_review to needs_work
comment:6 in reply to: ↑ 4 ; follow-up: ↓ 9 Changed 4 years ago by
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 4 years ago by
- Commit changed from 8108128f84bd2148eff99fc40b9399340bbbdd93 to 7cfb4afaa04d3469c8989b401648e89727614bb2
comment:8 Changed 4 years ago by
- Status changed from needs_work to needs_review
comment:9 in reply to: ↑ 6 ; follow-ups: ↓ 10 ↓ 13 Changed 4 years ago by
- 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 4 years ago by
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: ↓ 12 Changed 4 years ago by
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: ↓ 16 Changed 4 years ago by
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: ↓ 17 Changed 4 years ago by
Replying to tscrim:
I ask because it feels like we should pass
names
off tonormalize_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 4 years ago by
- Commit changed from 7cfb4afaa04d3469c8989b401648e89727614bb2 to 52e13d82d1fd973d523ec520387fbec10e3f32cc
Branch pushed to git repo; I updated commit sha1. New commits:
52e13d8 | Add deprecation for completion(p=...)
|
comment:15 Changed 4 years ago by
- Status changed from needs_info to needs_review
comment:16 in reply to: ↑ 12 Changed 4 years ago by
comment:17 in reply to: ↑ 13 Changed 4 years ago by
- 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 tonormalize_names
There is a different here for
names
than thePolynomialRing
constructor for example: thenames
are not arbitrary names, they refer to an element ofself
. 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 4 years ago by
- Branch changed from u/jdemeyer/clean_up_mpolynomialring_generic_completion to 52e13d82d1fd973d523ec520387fbec10e3f32cc
- Resolution set to fixed
- Status changed from positive_review to closed
New commits:
Clean up MPolynomialRing_generic.completion