Opened 4 years ago

Closed 3 years ago

Last modified 3 years ago

#26588 closed enhancement (fixed)

groups for handling arguments

Reported by: dkrenn Owned by:
Priority: major Milestone: sage-8.8
Component: asymptotic expansions Keywords:
Cc: behackl 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 dkrenn

  • Branch set to u/dkrenn/roots-of-unity-group

comment:2 Changed 4 years ago by git

  • Commit set to 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 changed from b3c17bf2c2f1b1a01901eefbab326cbc8b7a6429 to 27ec54260c575610e099909a8a607d0ff67e7a64

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 changed from 27ec54260c575610e099909a8a607d0ff67e7a64 to 541148b67ca2e3589713dc27be37a75202edf0b5

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 changed from 541148b67ca2e3589713dc27be37a75202edf0b5 to da0d3dcd77eebc93ac83647a6486ec29455bb046

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

da0d3dcU: index new groups

comment:6 Changed 4 years ago by git

  • Commit changed from da0d3dcd77eebc93ac83647a6486ec29455bb046 to 4b87cb4c7a0ef122c73c2408308dc922a49d82fc

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 changed from 4b87cb4c7a0ef122c73c2408308dc922a49d82fc to 07a398834cbe70773f5bbf573429f4df9105a264

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 dkrenn

  • Cc behackl added
  • Status changed from new to needs_review

comment:9 Changed 4 years ago by git

  • Commit changed from 07a398834cbe70773f5bbf573429f4df9105a264 to b2857e95ce725c5c47337844aba13d181089b789

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

b2857e9U: absolute value

comment:10 Changed 4 years ago by dkrenn

Small Python3-related update concerning #26587.

comment:11 follow-up: Changed 4 years ago by behackl

  • 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 3 years ago by git

  • Commit changed from b2857e95ce725c5c47337844aba13d181089b789 to 01aef0f16ba011eb4021c062eacbb67d11f3bc80

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 3 years ago by git

  • Commit changed from 01aef0f16ba011eb4021c062eacbb67d11f3bc80 to 9aab7299683393897f5c63a3fbd85579994b2add

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 3 years ago by dkrenn

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 3 years ago by dkrenn

  • Status changed from needs_work to needs_review

comment:16 follow-up: Changed 3 years ago by behackl

  • Branch changed from u/dkrenn/roots-of-unity-group to u/behackl/asy/roots-of-unity-groups
  • 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
    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 dkrenn

  • Branch changed from u/behackl/asy/roots-of-unity-groups to u/dkrenn/asy/roots-of-unity-groups

comment:18 in reply to: ↑ 16 Changed 3 years ago by dkrenn

  • 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
    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 changed from 31668d62d90847dfaf8d5016b5717f33d953f80d to 9d7173d695e314e70e0986d49d37e8c105f34e42

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 behackl

  • Milestone changed from sage-8.5 to sage-8.8
  • Status changed from needs_review to positive_review

I am happy with all changes.

comment:21 Changed 3 years ago by vbraun

  • Branch changed from u/dkrenn/asy/roots-of-unity-groups to 9d7173d695e314e70e0986d49d37e8c105f34e42
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:22 follow-up: Changed 3 years ago by mmezzarobba

  • 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 3 years ago by dkrenn

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.