Opened 5 years ago

Closed 5 years ago

#15369 closed defect (fixed)

groups.misc.AdditiveCyclic

Reported by: ncohen Owned by:
Priority: trivial Milestone: sage-6.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 ncohen)

Following the discussion at ​https://groups.google.com/d/msg/sage-devel/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 5 years ago by ncohen

  • Branch set to u/ncohen/15369
  • Status changed from new to needs_review

comment:2 Changed 5 years ago by git

  • Commit set to e6b908f801621f808601646f4c5a16b1aeee6c0e

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

e6b908fIntegerVectors/FreeModules/AdditiveAbelianGroup? of dimension 1 do not accept 5 as input and expect [5]

comment:3 follow-up: Changed 5 years ago by darij

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 follow-up: Changed 5 years ago by vbraun

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 ; follow-ups: Changed 5 years ago by ncohen

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

  • Commit changed from e6b908f801621f808601646f4c5a16b1aeee6c0e to 7df421a36ad515f6b2c3cf9178dbcccfb05161c8

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

7df421aFreeModules/AdditiveAbelianGroup? of dimension 1 do not accept 5 as input and expect [5]

comment:7 in reply to: ↑ 3 Changed 5 years ago by ncohen

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:

7df421aFreeModules/AdditiveAbelianGroup? of dimension 1 do not accept 5 as input and expect [5]

comment:8 in reply to: ↑ 5 ; follow-up: Changed 5 years ago by nbruin

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 5 years ago by ncohen

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 5 years ago by vdelecroix

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 5 years ago by ncohen

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 5 years ago by vbraun

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 5 years ago by ncohen

.......

Okay. Then, as we DO need to build easily a Z/nZ somewhere, would you agree to review a 3-lines patch that adds an alias to IntegerModRing as groups.misc.AdditiveCyclic ?

Nathann

comment:14 Changed 5 years ago by vbraun

That would be fine with me.

comment:15 Changed 5 years ago by ncohen

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

  • Commit changed from 7df421a36ad515f6b2c3cf9178dbcccfb05161c8 to 16d141c68a3441a398e14ba36c313ee8e7253181

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

16d141cAlias groups.misc.AdditiveCyclic? to IntegerModRing?
c6bb227Depends on #15368
0ad88a6Additions to groups.misc : AdditiveAbelian?, Free, Braid, SemimonomialTransformation?
3d75bd3Move QuaternionMatrixGroupGF3 from groups.misc to groups.matrix
2a68051Add affine groups to groups.<tab>
017084fBroken link in groups catalog doc

comment:17 Changed 5 years ago by ncohen

  • Component changed from combinatorics to group theory

comment:18 Changed 5 years ago by ncohen

I will rebase this thing in a second to clean the dependencies.

Nathann

comment:19 Changed 5 years ago by git

  • Commit changed from 16d141c68a3441a398e14ba36c313ee8e7253181 to 6c7e2f4b47869968952219143af61be270b152a3

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

6c7e2f4trac #15369: Alias groups.misc.AdditiveCyclic to IntegerModRing

comment:20 Changed 5 years ago by ncohen

  • Priority changed from major to trivial

comment:21 Changed 5 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_review

lgtm

comment:22 Changed 5 years ago by tscrim

I just did the same thing... :P

comment:23 Changed 5 years ago by vbraun

  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.