Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#26588 closed enhancement (fixed)

groups for handling arguments

Reported by: Daniel Krenn Owned by:
Priority: major Milestone: sage-8.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:

Status badges

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 Daniel Krenn

Branch: u/dkrenn/roots-of-unity-group

comment:2 Changed 4 years ago by git

Commit: b3c17bf2c2f1b1a01901eefbab326cbc8b7a6429

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

02eef7bU: more imports cleanup in doctests
db30a45U: implement __pow__
3b7b75cU: major restructuring of classes (factor out AbstractArgumentGroup)
1bfff61U: ArgumentByElementGroup
2fbf67eU: ArgumentGroupFactory
e03bd7fU: fixup: spaces
adbeca0U: fixup imports
51e5ee6U: update copyright
b3c17bfU: module description

comment:3 Changed 4 years ago by git

Commit: b3c17bf2c2f1b1a01901eefbab326cbc8b7a642927ec54260c575610e099909a8a607d0ff67e7a64

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

1d7098eU: cleanup
7627b79U: simplify repr-code (by using existing stuff)
24dd452U: extend error message in ArgumentGroup-factory
4087abaU: rename to argument_groups
b429e37I: imaginary groups initial commit
99c7af7I: imaginary elements
d60a1a5I: imaginary group
b07eb40U: extend error message for conversion
27ec542U: fix imports in doctests after previous move/rename

comment:4 Changed 4 years ago by git

Commit: 27ec54260c575610e099909a8a607d0ff67e7a64541148b67ca2e3589713dc27be37a75202edf0b5

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

0e68723I: add a doctest
541148bU: make the docs build

comment:5 Changed 4 years ago by git

Commit: 541148b67ca2e3589713dc27be37a75202edf0b5da0d3dcd77eebc93ac83647a6486ec29455bb046

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

da0d3dcU: index new groups

comment:6 Changed 4 years ago by git

Commit: da0d3dcd77eebc93ac83647a6486ec29455bb0464b87cb4c7a0ef122c73c2408308dc922a49d82fc

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

3817de0U: extend __pow__ to take care of extensions
d788f68U: simplify element_constructor
333411bU: methods for conversion to symbolic
fe54d42U: use new _symbolic_
dc3ae71U: Action of RootsOfUnityGroup
ce8b438U: implement SignGroup
11df6a0U: is_one, is_minus_one for Sign
72fc938U: Action of Sign
4b87cb4U: doctests and docstrings (minor)

comment:7 Changed 4 years ago by git

Commit: 4b87cb4c7a0ef122c73c2408308dc922a49d82fc07a398834cbe70773f5bbf573429f4df9105a264

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

83c8114U: use sign group in factory
029ce0aU: fix doctests
9fa8cecU: implement coercion
c31cfcdU: extend element construction (related to coercion(
07a3988U: coercion for argument by element groups

comment:8 Changed 4 years ago by Daniel Krenn

Cc: Benjamin Hackl added
Status: newneeds_review

comment:9 Changed 4 years ago by git

Commit: 07a398834cbe70773f5bbf573429f4df9105a264b2857e95ce725c5c47337844aba13d181089b789

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

b2857e9U: absolute value

comment:10 Changed 4 years ago by Daniel Krenn

Small Python3-related update concerning #26587.

comment:11 Changed 4 years ago by Benjamin Hackl

Status: needs_reviewneeds_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 git

Commit: b2857e95ce725c5c47337844aba13d181089b78901aef0f16ba011eb4021c062eacbb67d11f3bc80

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

01aef0fMerge tag '8.7' into u/dkrenn/roots-of-unity-group

comment:13 Changed 4 years ago by git

Commit: 01aef0f16ba011eb4021c062eacbb67d11f3bc809aab7299683393897f5c63a3fbd85579994b2add

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

48523adextend conversion repr<-->parent (cherry-pick)
9aab729remove import

comment:14 in reply to:  11 Changed 4 years ago by Daniel Krenn

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 cherry-picked.

comment:15 Changed 4 years ago by Daniel Krenn

Status: needs_workneeds_review

comment:16 Changed 3 years ago by Benjamin Hackl

Branch: u/dkrenn/roots-of-unity-groupu/behackl/asy/roots-of-unity-groups
Commit: 9aab7299683393897f5c63a3fbd85579994b2add7c7eb1fcf6b52d026a8f151c61a34d5ba7d2ce31
Reviewers: Benjamin Hackl
Status: needs_reviewneeds_work

Added two commits. Some further comments:

  • argument_groups.py
    1. L67: What is the intention of the normalize-keyword and how should it be used? Currently, only for UnitCirclePoint the argument is normalized to be contained within the interval [0,1). For ArgumentByElement 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 from AbstractArgumentGroup?
    2. L297: __abs__ returns the Python int 1. (Fixed via 980825c561)
    3. I wanted to have some additional doctests where normalization is involved (c3d5c15f47).
    4. L1713: I think that exactly_one_is_true is quite useful and should maybe live in misc/misc.py.
  • imaginary_groups.py:
    1. After implementing imag and real, also the global methods real_part and imag_part can be called; this should be tested (7c7eb1fcf6)

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:

980825cabs(AbstractArgument) should not return Python int
c3d5c15additional doctests w.r.t. normalization
7c7eb1ftest that global methods real_part and imag_part also work

comment:17 Changed 3 years ago by Daniel Krenn

Branch: u/behackl/asy/roots-of-unity-groupsu/dkrenn/asy/roots-of-unity-groups

comment:18 in reply to:  16 Changed 3 years ago by Daniel Krenn

Commit: 7c7eb1fcf6b52d026a8f151c61a34d5ba7d2ce3131668d62d90847dfaf8d5016b5717f33d953f80d
Status: needs_workneeds_review

Replying to behackl:

Added two commits.

LGTM, thanks.

  • argument_groups.py
    1. L67: What is the intention of the normalize-keyword and how should it be used? Currently, only for UnitCirclePoint the argument is normalized to be contained within the interval [0,1). For ArgumentByElement 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 from AbstractArgumentGroup?

kwds are now passed on the element during construction.

  1. 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.

  1. I wanted to have some additional doctests where normalization is involved (c3d5c15f47).

LGTM

  1. L1713: I think that exactly_one_is_true is quite useful and should maybe live in misc/misc.py.

Done.

  • imaginary_groups.py:
    1. After implementing imag and real, also the global methods real_part and imag_part can be called; this should be tested (7c7eb1fcf6)

LGTM

Needs small cross review of changes.


New commits:

7620a4fTrac #26588: kwds passed on to element during construction
3d72ec3Trac #26588: return type of __abs__
31668d6Trac #26588: move exactly_one_is_true to sage.misc.misc

comment:19 Changed 3 years ago by git

Commit: 31668d62d90847dfaf8d5016b5717f33d953f80d9d7173d695e314e70e0986d49d37e8c105f34e42

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

9d7173dTrac #26588: add doctest for normalize=False

comment:20 Changed 3 years ago by Benjamin Hackl

Milestone: sage-8.5sage-8.8
Status: needs_reviewpositive_review

I am happy with all changes.

comment:21 Changed 3 years ago by Volker Braun

Branch: u/dkrenn/asy/roots-of-unity-groups9d7173d695e314e70e0986d49d37e8c105f34e42
Resolution: fixed
Status: positive_reviewclosed

comment:22 Changed 3 years ago by Marc Mezzarobba

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 in reply to:  22 Changed 3 years ago by Daniel Krenn

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

Note: See TracTickets for help on using tickets.