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

Priority:  major  Milestone:  sage8.8 
Component:  asymptotic expansions  Keywords:  
Cc:  behackl  Merged in:  
Authors:  Daniel Krenn  Reviewers:  Benjamin Hackl 
Report Upstream:  N/A  Work issues:  
Branch:  9d7173d (Commits)  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 2 years ago by
 Branch set to u/dkrenn/rootsofunitygroup
comment:2 Changed 2 years ago by
 Commit set to b3c17bf2c2f1b1a01901eefbab326cbc8b7a6429
comment:3 Changed 2 years ago by
 Commit changed from b3c17bf2c2f1b1a01901eefbab326cbc8b7a6429 to 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 2 years ago by
 Commit changed from 27ec54260c575610e099909a8a607d0ff67e7a64 to 541148b67ca2e3589713dc27be37a75202edf0b5
comment:5 Changed 2 years ago by
 Commit changed from 541148b67ca2e3589713dc27be37a75202edf0b5 to da0d3dcd77eebc93ac83647a6486ec29455bb046
Branch pushed to git repo; I updated commit sha1. New commits:
da0d3dc  U: index new groups

comment:6 Changed 2 years ago by
 Commit changed from da0d3dcd77eebc93ac83647a6486ec29455bb046 to 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 2 years ago by
 Commit changed from 4b87cb4c7a0ef122c73c2408308dc922a49d82fc to 07a398834cbe70773f5bbf573429f4df9105a264
comment:8 Changed 2 years ago by
 Cc behackl added
 Status changed from new to needs_review
comment:9 Changed 2 years ago by
 Commit changed from 07a398834cbe70773f5bbf573429f4df9105a264 to b2857e95ce725c5c47337844aba13d181089b789
Branch pushed to git repo; I updated commit sha1. New commits:
b2857e9  U: absolute value

comment:10 Changed 2 years ago by
Small Python3related update concerning #26587.
comment:11 followup: ↓ 14 Changed 2 years ago by
 Status changed from needs_review to 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 22 months ago by
 Commit changed from b2857e95ce725c5c47337844aba13d181089b789 to 01aef0f16ba011eb4021c062eacbb67d11f3bc80
Branch pushed to git repo; I updated commit sha1. New commits:
01aef0f  Merge tag '8.7' into u/dkrenn/rootsofunitygroup

comment:13 Changed 22 months ago by
 Commit changed from 01aef0f16ba011eb4021c062eacbb67d11f3bc80 to 9aab7299683393897f5c63a3fbd85579994b2add
comment:14 in reply to: ↑ 11 Changed 22 months 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 22 months ago by
 Status changed from needs_work to needs_review
comment:16 followup: ↓ 18 Changed 21 months ago by
 Branch changed from u/dkrenn/rootsofunitygroup to u/behackl/asy/rootsofunitygroups
 Commit changed from 9aab7299683393897f5c63a3fbd85579994b2add to 7c7eb1fcf6b52d026a8f151c61a34d5ba7d2ce31
 Reviewers set to Benjamin Hackl
 Status changed from needs_review to 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 21 months ago by
 Branch changed from u/behackl/asy/rootsofunitygroups to u/dkrenn/asy/rootsofunitygroups
comment:18 in reply to: ↑ 16 Changed 21 months ago by
 Commit changed from 7c7eb1fcf6b52d026a8f151c61a34d5ba7d2ce31 to 31668d62d90847dfaf8d5016b5717f33d953f80d
 Status changed from needs_work to 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 21 months ago by
 Commit changed from 31668d62d90847dfaf8d5016b5717f33d953f80d to 9d7173d695e314e70e0986d49d37e8c105f34e42
Branch pushed to git repo; I updated commit sha1. New commits:
9d7173d  Trac #26588: add doctest for normalize=False

comment:20 Changed 21 months ago by
 Milestone changed from sage8.5 to sage8.8
 Status changed from needs_review to positive_review
I am happy with all changes.
comment:21 Changed 21 months ago by
 Branch changed from u/dkrenn/asy/rootsofunitygroups to 9d7173d695e314e70e0986d49d37e8c105f34e42
 Resolution set to fixed
 Status changed from positive_review to closed
comment:22 followup: ↓ 23 Changed 16 months ago by
 Commit 9d7173d695e314e70e0986d49d37e8c105f34e42 deleted
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 in reply to: ↑ 22 Changed 16 months 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