Opened 6 years ago
Closed 6 years ago
#15369 closed defect (fixed)
groups.misc.AdditiveCyclic
Reported by:  ncohen  Owned by:  

Priority:  trivial  Milestone:  sage6.1 
Component:  group theory  Keywords:  
Cc:  vbraun, fhivert, nthiery, vdelecroix, jhpalmieri, kcrisman  Merged in:  
Authors:  Nathann Cohen  Reviewers:  Volker Braun 
Report Upstream:  N/A  Work issues:  
Branch:  u/ncohen/15369 (Commits)  Commit:  6c7e2f4b47869968952219143af61be270b152a3 
Dependencies:  #15368  Stopgaps: 
Description (last modified by )
Following the discussion at https://groups.google.com/d/msg/sagedevel/tyAxhqxk3ZI/SuBg2Ukwwj4J3
There is currently no way to create a group representing Z/nZ
easily. And the one acknowledged way to do that being Integers(n)
(i.e. IntegerModRing(n)
), this patch exposes it as groups.misc.AdditiveCyclic
.
Nathann
Change History (23)
comment:1 Changed 6 years ago by
 Branch set to u/ncohen/15369
 Status changed from new to needs_review
comment:2 Changed 6 years ago by
 Commit set to e6b908f801621f808601646f4c5a16b1aeee6c0e
comment:3 followup: ↓ 7 Changed 6 years ago by
The patch so far is incomplete  for instance, G=FreeModule(QQ,1); G(1)
isn't working. The FreeModule
class is a factory and it would be best to fix it for all of its implementations.
Also, please add a doctest!
comment:4 followup: ↓ 5 Changed 6 years ago by
IMHO it is very confusing that G(1) is not the identity element. I would prefer to always require lists, since that is what you need in general.
comment:5 in reply to: ↑ 4 ; followups: ↓ 8 ↓ 10 Changed 6 years ago by
IMHO it is very confusing that G(1) is not the identity element.
Even in (ZZ,+)
? I find it rather natural.
I would prefer to always require lists, since that is what you need in general.
Come on Volker. I want to create Z/5Z
, and you think more natural to input lists of size 1 than integers ?
Nathann
comment:6 Changed 6 years ago by
 Commit changed from e6b908f801621f808601646f4c5a16b1aeee6c0e to 7df421a36ad515f6b2c3cf9178dbcccfb05161c8
Branch pushed to git repo; I updated commit sha1. New commits:
7df421a  FreeModules/AdditiveAbelianGroup? of dimension 1 do not accept 5 as input and expect [5] 
comment:7 in reply to: ↑ 3 Changed 6 years ago by
Hellooooo ! I rewrote this a bit differently, without touching integer vectors. Could you tell me if it works on principle ? I'll add doctests immediatly in that case. As I really don't feel comfortable touching this code :/
THaaaaaaaaaaaaaaanks !
Nathann
New commits:
7df421a  FreeModules/AdditiveAbelianGroup? of dimension 1 do not accept 5 as input and expect [5] 
comment:8 in reply to: ↑ 5 ; followup: ↓ 9 Changed 6 years ago by
Replying to ncohen:
I would prefer to always require lists, since that is what you need in general.
Come on Volker. I want to create
Z/5Z
, and you think more natural to input lists of size 1 than integers ?
That's not what you're asking sage to do. You're asking for a finite abelian group, written additively, that happens to be cyclic of order 5. If you know your group is going to be cyclic, you'd probably be better off asking for a cyclic group explicitly.
So I'd agree with Volker that lists are more natural here. At the same time you're asking for conversion, which is supposed to make a best effort, so adding a special case wouldn't be out of the question, even if it's an ugly special case.
comment:9 in reply to: ↑ 8 Changed 6 years ago by
That's not what you're asking sage to do. You're asking for a finite abelian group, written additively, that happens to be cyclic of order 5. If you know your group is going to be cyclic, you'd probably be better off asking for a cyclic group explicitly.
Well. I want an additive Cyclic group. How can I build that ? This was my original problem, and I was told AdditiveAbelianGroup? was the way to obtain it.
So I'd agree with Volker that lists are more natural here.
If there exists an Additive Cyclic group somewhere, I agree.
At the same time you're asking for conversion, which is supposed to make a best effort, so adding a special case wouldn't be out of the question, even if it's an ugly special case.
I'm sorry if it is ugly. I really don't know these classes and they are all using each other, so I am always afraid something wrong might happen somewhere :/
Nathann
comment:10 in reply to: ↑ 5 Changed 6 years ago by
I would prefer to always require lists, since that is what you need in general.
Come on Volker. I want to create
Z/5Z
, and you think more natural to input lists of size 1 than integers ?
And you would like that the following works ?
sage: V = VectorSpace(RR,1) sage: V(4.123)
comment:11 Changed 6 years ago by
Well, I wouldn't see the problem indeed. I don't know if the last commit does that, though. It touches FreeModule?.
Nathann
comment:12 Changed 6 years ago by
If you want want a cyclic group with nice map Z > Z/nZ then just use IntegerModRing
.
The notation that you propose can certainly be implemented, but its a confusing special case that can't work for a general Abelian group. So 1 from me, consistency is more important than saving two keystrokes.
comment:13 Changed 6 years ago by
.......
Okay. Then, as we DO need to build easily a Z/nZ
somewhere, would you agree to review a 3lines patch that adds an alias to IntegerModRing
as groups.misc.AdditiveCyclic
?
Nathann
comment:14 Changed 6 years ago by
That would be fine with me.
comment:15 Changed 6 years ago by
 Dependencies set to #15368
 Description modified (diff)
 Summary changed from IntegerVectors/FreeModules/AdditiveAbelianGroup of dimension 1 do not accept 5 as input and expect [5] to groups.misc.AdditiveCyclic
Done !
comment:16 Changed 6 years ago by
 Commit changed from 7df421a36ad515f6b2c3cf9178dbcccfb05161c8 to 16d141c68a3441a398e14ba36c313ee8e7253181
Branch pushed to git repo; I updated commit sha1. New commits:
16d141c  Alias groups.misc.AdditiveCyclic? to IntegerModRing? 
c6bb227  Depends on #15368 
0ad88a6  Additions to groups.misc : AdditiveAbelian?, Free, Braid, SemimonomialTransformation? 
3d75bd3  Move QuaternionMatrixGroupGF3 from groups.misc to groups.matrix 
2a68051  Add affine groups to groups.<tab> 
017084f  Broken link in groups catalog doc 
comment:17 Changed 6 years ago by
 Component changed from combinatorics to group theory
comment:18 Changed 6 years ago by
I will rebase this thing in a second to clean the dependencies.
Nathann
comment:19 Changed 6 years ago by
 Commit changed from 16d141c68a3441a398e14ba36c313ee8e7253181 to 6c7e2f4b47869968952219143af61be270b152a3
Branch pushed to git repo; I updated commit sha1. New commits:
6c7e2f4  trac #15369: Alias groups.misc.AdditiveCyclic to IntegerModRing

comment:20 Changed 6 years ago by
 Priority changed from major to trivial
comment:21 Changed 6 years ago by
 Reviewers set to Volker Braun
 Status changed from needs_review to positive_review
lgtm
comment:22 Changed 6 years ago by
I just did the same thing... :P
comment:23 Changed 6 years ago by
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits: