Opened 2 years ago
Closed 2 years ago
#30409 closed enhancement (fixed)
Optional ideal parameter for normalize_coordinates
Reported by:  Alexander Galarraga  Owned by:  

Priority:  minor  Milestone:  sage9.2 
Component:  dynamics  Keywords:  gsoc20 
Cc:  Ben Hutz, Paul Fili  Merged in:  
Authors:  Alexander Galarraga  Reviewers:  Ben Hutz 
Report Upstream:  N/A  Work issues:  
Branch:  fdc5569 (Commits, GitHub, GitLab)  Commit:  fdc5569630dc6775b1758343195d8da3ec1972f0 
Dependencies:  Stopgaps: 
Description
In niche use cases, it is helpful to normalize a dynamical system defined over a number field with respect to a padic absolute value. By adding an optional ideal parameter, we allow for normalization with respect to a padic absolute value. In this context, normalization is scaling so that one coefficient has absolute value 1 while the rest have absolute value less than 1.
Change History (20)
comment:1 Changed 2 years ago by
Branch:  → u/ghEnderWannabe/normalize_coordinates_enhancement 

comment:2 Changed 2 years ago by
Commit:  → 6dc7c5a24b847873e78305cc81145bf08cc700ae 

Status:  new → needs_review 
comment:4 Changed 2 years ago by
Commit:  6dc7c5a24b847873e78305cc81145bf08cc700ae → 6fd203e4d9b20cd239e64f5073231f6e2e86d6b5 

Branch pushed to git repo; I updated commit sha1. New commits:
6fd203e  Merge branch 'develop' into normalize_coordinates_enhancement

comment:5 Changed 2 years ago by
Status:  needs_work → needs_review 

comment:6 Changed 2 years ago by
Status:  needs_review → needs_work 

comment:7 Changed 2 years ago by
This initial implementation looks ok, but I'd like to see the ability to pass in a valuation as well, instead of only an ideal.
comment:8 Changed 2 years ago by
Commit:  6fd203e4d9b20cd239e64f5073231f6e2e86d6b5 → 7d485e88078cf1c85011ac41840dd94f4f8eb685 

Branch pushed to git repo; I updated commit sha1. New commits:
7d485e8  30409: added support for valuations

comment:9 Changed 2 years ago by
Status:  needs_work → needs_review 

comment:10 Changed 2 years ago by
Reviewers:  → Ben Hutz 

Status:  needs_review → needs_work 
Only one complaint here. I think it would be better to use kwds and allow ideal= or valuation= instead of overloading the ideal parameter.
Otherwise, the functionality works as expected.
I was able to produce some cases where the phrasing of the error messages was not quite aligned with the issue, but these are not cases where the function should be working.
P.<x,y>=ProjectiveSpace(QQbar,1) f=DynamicalSystem([x^2 + 2*x*y + 3*y^2, 4*y^2 + 5*x*y+7*x^2]) f.normalize_coordinates(ideal=QQbar(3)) ValueError: ideal was defined over QQ, but the base ring of this morphism is Algebraic Field
This is not an ideal defined over QQ, rather it is the ideal (1) in QQbar.
Similarly, this is an ideal contrary to what the error says
P.<x,y>=ProjectiveSpace(QQbar,1) f=DynamicalSystem([x^2 + 2*x*y + 3*y^2, 4*y^2 + 5*x*y+7*x^2]) f.normalize_coordinates(ideal=QQbar.ideal(3)) TypeError: ideal must be an ideal or a valuation, not Principal ideal (1) of Algebraic Field
I'm not suggesting you special case things like QQbar in your error messages. Rather perhaps make the language somewhat more general. In this second you could say, must be a prime ideal or valuation. The first error may not a bigger language change.
comment:11 Changed 2 years ago by
Commit:  7d485e88078cf1c85011ac41840dd94f4f8eb685 → a9105b3bd9f6a6fdc88b5e791b07cf0547b27119 

Branch pushed to git repo; I updated commit sha1. New commits:
a9105b3  30409: use kwargs, better error messages

comment:12 Changed 2 years ago by
Status:  needs_work → needs_review 

comment:13 Changed 2 years ago by
Status:  needs_review → needs_work 

Those errors I think are less confusing.
two things
 kwds is what should be used
 the kwds should be in a keywords: block of the INPUT section,
comment:14 Changed 2 years ago by
Commit:  a9105b3bd9f6a6fdc88b5e791b07cf0547b27119 → 833e87473db60f88d019d9bb216646d6c997b75f 

Branch pushed to git repo; I updated commit sha1. New commits:
833e874  30409: fixed keywords

comment:15 Changed 2 years ago by
Status:  needs_work → needs_review 

comment:16 Changed 2 years ago by
Status:  needs_review → needs_work 

You need to change the kwargs in the code to match
ideal = kwargs.pop('ideal', None)
and the other one.
comment:17 Changed 2 years ago by
Commit:  833e87473db60f88d019d9bb216646d6c997b75f → fdc5569630dc6775b1758343195d8da3ec1972f0 

Branch pushed to git repo; I updated commit sha1. New commits:
fdc5569  30409: fixed kwds

comment:18 Changed 2 years ago by
Status:  needs_work → needs_review 

comment:19 Changed 2 years ago by
Status:  needs_review → positive_review 

comment:20 Changed 2 years ago by
Branch:  u/ghEnderWannabe/normalize_coordinates_enhancement → fdc5569630dc6775b1758343195d8da3ec1972f0 

Resolution:  → fixed 
Status:  positive_review → closed 
New commits:
30409: initial implementation