Opened 2 years ago

Closed 21 months ago

Last modified 16 months 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) 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 dkrenn

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

comment:2 Changed 2 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 2 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 2 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 2 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 2 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 2 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 2 years ago by dkrenn

  • Cc behackl added
  • Status changed from new to needs_review

comment:9 Changed 2 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 2 years ago by dkrenn

Small Python3-related update concerning #26587.

comment:11 follow-up: Changed 2 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 22 months 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 22 months 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 22 months 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 22 months ago by dkrenn

  • Status changed from needs_work to needs_review

comment:16 follow-up: Changed 21 months 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 21 months 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 21 months 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 21 months 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 21 months 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 21 months 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 16 months 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 16 months 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.