#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: |
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 set to u/dkrenn/roots-of-unity-group
comment:2 Changed 4 years ago by
- Commit set to b3c17bf2c2f1b1a01901eefbab326cbc8b7a6429
comment:3 Changed 4 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 repr-code (by using existing stuff)
|
24dd452 | U: extend error message in ArgumentGroup-factory
|
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 changed from 27ec54260c575610e099909a8a607d0ff67e7a64 to 541148b67ca2e3589713dc27be37a75202edf0b5
comment:5 Changed 4 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 4 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 4 years ago by
- Commit changed from 4b87cb4c7a0ef122c73c2408308dc922a49d82fc to 07a398834cbe70773f5bbf573429f4df9105a264
comment:8 Changed 4 years ago by
- Cc behackl added
- Status changed from new to needs_review
comment:9 Changed 4 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 4 years ago by
Small Python3-related update concerning #26587.
comment:11 follow-up: ↓ 14 Changed 4 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 3 years 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/roots-of-unity-group
|
comment:13 Changed 3 years ago by
- Commit changed from 01aef0f16ba011eb4021c062eacbb67d11f3bc80 to 9aab7299683393897f5c63a3fbd85579994b2add
comment:14 in reply to: ↑ 11 Changed 3 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 cherry-picked.
comment:15 Changed 3 years ago by
- Status changed from needs_work to needs_review
comment:16 follow-up: ↓ 18 Changed 3 years ago by
- 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
- 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 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
- 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 3 years 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 3 years ago by
- 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
- 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: ↓ 23 Changed 3 years 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 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