#26588 closed enhancement (fixed)
groups for handling arguments
Reported by:  Daniel Krenn  Owned by:  

Priority:  major  Milestone:  sage8.8 
Component:  asymptotic expansions  Keywords:  
Cc:  Benjamin Hackl  Merged in:  
Authors:  Daniel Krenn  Reviewers:  Benjamin Hackl 
Report Upstream:  N/A  Work issues:  
Branch:  9d7173d (Commits, GitHub, GitLab)  Commit:  
Dependencies:  Stopgaps: 
Description
Implement groups that handle complex arguments, points on the unit circle, roots of unit etc.
These groups are needed for growth groups of asymptotic rings in relation to #26587.
Change History (23)
comment:1 Changed 4 years ago by
Branch:  → u/dkrenn/rootsofunitygroup 

comment:2 Changed 4 years ago by
Commit:  → b3c17bf2c2f1b1a01901eefbab326cbc8b7a6429 

comment:3 Changed 4 years ago by
Commit:  b3c17bf2c2f1b1a01901eefbab326cbc8b7a6429 → 27ec54260c575610e099909a8a607d0ff67e7a64 

Branch pushed to git repo; I updated commit sha1. New commits:
1d7098e  U: cleanup

7627b79  U: simplify reprcode (by using existing stuff)

24dd452  U: extend error message in ArgumentGroupfactory

4087aba  U: rename to argument_groups

b429e37  I: imaginary groups initial commit

99c7af7  I: imaginary elements

d60a1a5  I: imaginary group

b07eb40  U: extend error message for conversion

27ec542  U: fix imports in doctests after previous move/rename

comment:4 Changed 4 years ago by
Commit:  27ec54260c575610e099909a8a607d0ff67e7a64 → 541148b67ca2e3589713dc27be37a75202edf0b5 

comment:5 Changed 4 years ago by
Commit:  541148b67ca2e3589713dc27be37a75202edf0b5 → da0d3dcd77eebc93ac83647a6486ec29455bb046 

Branch pushed to git repo; I updated commit sha1. New commits:
da0d3dc  U: index new groups

comment:6 Changed 4 years ago by
Commit:  da0d3dcd77eebc93ac83647a6486ec29455bb046 → 4b87cb4c7a0ef122c73c2408308dc922a49d82fc 

Branch pushed to git repo; I updated commit sha1. New commits:
3817de0  U: extend __pow__ to take care of extensions

d788f68  U: simplify element_constructor

333411b  U: methods for conversion to symbolic

fe54d42  U: use new _symbolic_

dc3ae71  U: Action of RootsOfUnityGroup

ce8b438  U: implement SignGroup

11df6a0  U: is_one, is_minus_one for Sign

72fc938  U: Action of Sign

4b87cb4  U: doctests and docstrings (minor)

comment:7 Changed 4 years ago by
Commit:  4b87cb4c7a0ef122c73c2408308dc922a49d82fc → 07a398834cbe70773f5bbf573429f4df9105a264 

comment:8 Changed 4 years ago by
Cc:  Benjamin Hackl added 

Status:  new → needs_review 
comment:9 Changed 4 years ago by
Commit:  07a398834cbe70773f5bbf573429f4df9105a264 → b2857e95ce725c5c47337844aba13d181089b789 

Branch pushed to git repo; I updated commit sha1. New commits:
b2857e9  U: absolute value

comment:11 followup: 14 Changed 4 years ago by
Status:  needs_review → needs_work 

I have started to look at your changes. It seems that the patchbot is unhappy:
sage t long src/sage/groups/misc_gps/argument_groups.py ********************************************************************** File "src/sage/groups/misc_gps/argument_groups.py", line 658, in sage.groups.misc_gps.argument_groups.UnitCircleGroup._repr_short_ Failed example: UnitCircleGroup(RR)._repr_short_() Expected: 'U_RR' Got: 'U_(Real Field with 53 bits of precision)' ********************************************************************** File "src/sage/groups/misc_gps/argument_groups.py", line 1253, in sage.groups.misc_gps.argument_groups.ArgumentByElementGroup._repr_short_ Failed example: ArgumentByElementGroup(CC)._repr_short_() Expected: 'Arg_CC' Got: 'Arg_Complex Field with 53 bits of precision' ********************************************************************** 2 items had failures: 1 of 3 in sage.groups.misc_gps.argument_groups.ArgumentByElementGroup._repr_short_ 1 of 3 in sage.groups.misc_gps.argument_groups.UnitCircleGroup._repr_short_ [336 tests, 2 failures, 0.13 s]
comment:12 Changed 4 years ago by
Commit:  b2857e95ce725c5c47337844aba13d181089b789 → 01aef0f16ba011eb4021c062eacbb67d11f3bc80 

Branch pushed to git repo; I updated commit sha1. New commits:
01aef0f  Merge tag '8.7' into u/dkrenn/rootsofunitygroup

comment:13 Changed 4 years ago by
Commit:  01aef0f16ba011eb4021c062eacbb67d11f3bc80 → 9aab7299683393897f5c63a3fbd85579994b2add 

comment:14 Changed 4 years ago by
Replying to behackl:
I have started to look at your changes. It seems that the patchbot is unhappy:
sage t long src/sage/groups/misc_gps/argument_groups.py [...]
Indeed there was a commit missing; now cherrypicked.
comment:15 Changed 4 years ago by
Status:  needs_work → needs_review 

comment:16 followup: 18 Changed 3 years ago by
Branch:  u/dkrenn/rootsofunitygroup → u/behackl/asy/rootsofunitygroups 

Commit:  9aab7299683393897f5c63a3fbd85579994b2add → 7c7eb1fcf6b52d026a8f151c61a34d5ba7d2ce31 
Reviewers:  → Benjamin Hackl 
Status:  needs_review → needs_work 
Added two commits. Some further comments:
argument_groups.py
 L67: What is the intention of the
normalize
keyword and how should it be used? Currently, only forUnitCirclePoint
the argument is normalized to be contained within the interval [0,1). ForArgumentByElement
the element is not changed. Both of that is fine, however: the keyword is not respected by any of the_element_constructor_
methods. Is it only there for potential future groups inheriting fromAbstractArgumentGroup
?  L297:
__abs__
returns the Python int 1. (Fixed via 980825c561)  I wanted to have some additional doctests where normalization is involved (c3d5c15f47).
 L1713: I think that
exactly_one_is_true
is quite useful and should maybe live inmisc/misc.py
.
 L67: What is the intention of the
imaginary_groups.py
: After implementing
imag
andreal
, also the global methodsreal_part
andimag_part
can be called; this should be tested (7c7eb1fcf6)
 After implementing
Other than that, I have no further comments and am happy with the code. Having these rather basic groups allows to build a solution regarding the handling of the (incomparable) asymptotic growth elements in an AsymptoticRing
 which we desire a lot.
New commits:
980825c  abs(AbstractArgument) should not return Python int

c3d5c15  additional doctests w.r.t. normalization

7c7eb1f  test that global methods real_part and imag_part also work

comment:17 Changed 3 years ago by
Branch:  u/behackl/asy/rootsofunitygroups → u/dkrenn/asy/rootsofunitygroups 

comment:18 Changed 3 years ago by
Commit:  7c7eb1fcf6b52d026a8f151c61a34d5ba7d2ce31 → 31668d62d90847dfaf8d5016b5717f33d953f80d 

Status:  needs_work → needs_review 
Replying to behackl:
Added two commits.
LGTM, thanks.
argument_groups.py
 L67: What is the intention of the
normalize
keyword and how should it be used? Currently, only forUnitCirclePoint
the argument is normalized to be contained within the interval [0,1). ForArgumentByElement
the element is not changed. Both of that is fine, however: the keyword is not respected by any of the_element_constructor_
methods. Is it only there for potential future groups inheriting fromAbstractArgumentGroup
?
kwds
are now passed on the element during construction.
 L297:
__abs__
returns the Python int 1. (Fixed via 980825c561)
sage: type(exp(QQ(0))) <type 'sage.rings.integer.Integer'>
so I've changed it to have the same behavior.
 I wanted to have some additional doctests where normalization is involved (c3d5c15f47).
LGTM
 L1713: I think that
exactly_one_is_true
is quite useful and should maybe live inmisc/misc.py
.
Done.
imaginary_groups.py
:
 After implementing
imag
andreal
, also the global methodsreal_part
andimag_part
can be called; this should be tested (7c7eb1fcf6)
LGTM
Needs small cross review of changes.
New commits:
7620a4f  Trac #26588: kwds passed on to element during construction

3d72ec3  Trac #26588: return type of __abs__

31668d6  Trac #26588: move exactly_one_is_true to sage.misc.misc

comment:19 Changed 3 years ago by
Commit:  31668d62d90847dfaf8d5016b5717f33d953f80d → 9d7173d695e314e70e0986d49d37e8c105f34e42 

Branch pushed to git repo; I updated commit sha1. New commits:
9d7173d  Trac #26588: add doctest for normalize=False

comment:20 Changed 3 years ago by
Milestone:  sage8.5 → sage8.8 

Status:  needs_review → positive_review 
I am happy with all changes.
comment:21 Changed 3 years ago by
Branch:  u/dkrenn/asy/rootsofunitygroups → 9d7173d695e314e70e0986d49d37e8c105f34e42 

Resolution:  → fixed 
Status:  positive_review → closed 
comment:22 followup: 23 Changed 3 years ago by
Commit:  9d7173d695e314e70e0986d49d37e8c105f34e42 

Can you please explain what this doctest is for?
sage: i = CIF(I) sage: J(3*i) Traceback (most recent call last): ... ValueError: 3 (Real Interval Field with 53 bits of precision) is not in Integer Ring > *previous* TypeError: unable to coerce <type 'sage.rings.real_mpfi.RealIntervalFieldElement'> to an integer
I don't understand why this needs to fail while, according to a very similar test a few lines above, J(3*CBF(i))
is supposed to succeed.
After #28517, J(3*CIF(i))
would return 3*I as well. Is that acceptable?
Thanks!
comment:23 Changed 3 years ago by
Replying to mmezzarobba:
Can you please explain what this doctest is for? [...] I don't understand why this needs to fail while, according to a very similar test a few lines above,
J(3*CBF(i))
is supposed to succeed.After #28517,
J(3*CIF(i))
would return 3*I as well. Is that acceptable?
Indeed, this should not fail and is therefore very acceptable. (Not sure why I did not open a ticket for this or wrote at least some kind of remark.)
Thanks for catching this and also for asking back to me; very much appreciated. :)
Branch pushed to git repo; I updated commit sha1. New commits:
U: more imports cleanup in doctests
U: implement __pow__
U: major restructuring of classes (factor out AbstractArgumentGroup)
U: ArgumentByElementGroup
U: ArgumentGroupFactory
U: fixup: spaces
U: fixup imports
U: update copyright
U: module description