Opened 8 years ago
Closed 7 years ago
#15140 closed enhancement (fixed)
AbelianGroups should be in Groups().Commutative() / Groups().Commutative().Finite()
Reported by:  nthiery  Owned by:  

Priority:  major  Milestone:  sage6.4 
Component:  group theory  Keywords:  
Cc:  tscrim, ncohen  Merged in:  
Authors:  Travis Scrimshaw  Reviewers:  Nathann Cohen 
Report Upstream:  N/A  Work issues:  
Branch:  c127173 (Commits, GitHub, GitLab)  Commit:  c127173e1c64fb951cf197c80d288c79eb83aa3c 
Dependencies:  #10963 #16725  Stopgaps: 
Description (last modified by )
Everything is in the title.
This will solve http://ask.sagemath.org/question/2954/problemwithabeliangroupcayley_graph
I made this ticket depend on #10963 (which will be merged a priori soon) to have the nice axioms syntax, but it's not a strong dependency.
Change History (18)
comment:1 Changed 8 years ago by
 Description modified (diff)
comment:2 Changed 7 years ago by
 Milestone changed from sage6.1 to sage6.2
comment:3 Changed 7 years ago by
 Milestone changed from sage6.2 to sage6.3
comment:4 Changed 7 years ago by
 Cc tscrim added
comment:5 Changed 7 years ago by
 Branch set to public/groups/abelian_groups_category15140
 Commit set to 757c0449bb1dcc52ab36c11658d8ba653769ca78
 Dependencies changed from #10963 to #10963 #16725
 Status changed from new to needs_review
comment:6 Changed 7 years ago by
 Milestone changed from sage6.3 to sage6.4
comment:7 Changed 7 years ago by
 Cc ncohen added
Hey Nathann,
Here's somewhat of a followup to #16725. Do you think you'd be willing to do a review? Thanks.
comment:8 followup: ↓ 10 Changed 7 years ago by
Yoooooooooo !!
No problem for those tickets, I more or less understand what is going on ! :)
It looks good but I have two questions:
1) Is there any reason why AbelianGroup? does not appear in groups.<tab>
?
2) When is the code from Groups.ParentMethods.monoid_generators
being used ? You removed a couple of lines as this case is handled by the new FiniteGroups.ParentMethods.monoid_generators
, but then the code of the first method claims that it raises an exception when self is not a FiniteEnumeratedSet
. Couldn't this method be removed too ? I actually do not understand in which situation that fonction worked without using the "if/return" that you removed.
Nathann
comment:9 Changed 7 years ago by
 Status changed from needs_review to needs_info
comment:10 in reply to: ↑ 8 ; followup: ↓ 11 Changed 7 years ago by
 Status changed from needs_info to needs_review
Replying to ncohen:
1) Is there any reason why AbelianGroup? does not appear in
groups.<tab>
?
Probably because someone missed it when doing the initial groups catalog. Add it to groups.misc.<tab>
here?
2) When is the code from
Groups.ParentMethods.monoid_generators
being used ? You removed a couple of lines as this case is handled by the newFiniteGroups.ParentMethods.monoid_generators
, but then the code of the first method claims that it raises an exception when self is not aFiniteEnumeratedSet
. Couldn't this method be removed too ? I actually do not understand in which situation that fonction worked without using the "if/return" that you removed.
Groups.ParentMethods.monoid_generators
is used for any parent in the category of Groups
that doesn't implement its own monoid_generators
. If a group is in FiniteGroups
, which is what the if
statement was checking, then (with this change) it calls the FiniteGroups.ParentMethods.monoid_generators
instead (it's closer to the front of the MRO). It's a cleaner design (and something I should've realized on #16725).
comment:11 in reply to: ↑ 10 ; followup: ↓ 14 Changed 7 years ago by
Yooooooooo !
Probably because someone missed it when doing the initial groups catalog. Add it to
groups.misc.<tab>
here?
Well, here or in another ticket. It just takes one lie anyway.
Groups.ParentMethods.monoid_generators
is used for any parent in the category ofGroups
that doesn't implement its ownmonoid_generators
. If a group is inFiniteGroups
, which is what theif
statement was checking, then (with this change) it calls theFiniteGroups.ParentMethods.monoid_generators
instead (it's closer to the front of the MRO). It's a cleaner design (and something I should've realized on #16725).
Yes, but when will the code of Groups.ParentMethods.monoid_generators
be used, if not for finite group ? With you patch all FiniteGroups
have their own implementation now, but which kind of group is not a FiniteGroup
but is a FiniteEnumeratedSet
?
Nathann
comment:12 Changed 7 years ago by
 Commit changed from 757c0449bb1dcc52ab36c11658d8ba653769ca78 to 6fd6eda32d044bdca460bfa89a0b870adb1c0553
comment:13 Changed 7 years ago by
 Commit changed from 6fd6eda32d044bdca460bfa89a0b870adb1c0553 to eb3696ece93e12a34a2d44441cecaecdf9406ad0
Branch pushed to git repo; I updated commit sha1. New commits:
eb3696e  Merge branch 'develop' into public/groups/abelian_groups_category15140

comment:14 in reply to: ↑ 11 ; followup: ↓ 15 Changed 7 years ago by
Replying to ncohen:
Well, here or in another ticket. It just takes one lie anyway.
Done here.
Yes, but when will the code of
Groups.ParentMethods.monoid_generators
be used, if not for finite group ? With you patch allFiniteGroups
have their own implementation now, but which kind of group is not aFiniteGroup
but is aFiniteEnumeratedSet
?
Ah, I see the confusion. I'm checking the group generators are in FiniteEnumeratedSets
, not the group itself. In other words, I'm checking to see if it's finitely generated (since there is no axiom currently for this).
comment:15 in reply to: ↑ 14 Changed 7 years ago by
Yo !
Done here.
Thanks !
Ah, I see the confusion. I'm checking the group generators are in
FiniteEnumeratedSets
, not the group itself. In other words, I'm checking to see if it's finitely generated (since there is no axiom currently for this).
Sorry T_T
One last check and I will set it to positive_review
in 5 minutes.
Nathann
comment:16 Changed 7 years ago by
 Reviewers set to Nathann Cohen
Hello again !
Sorry for the delay, it took longer than expected: I checked the doctests in groups/ and I saw that one was broken, so I had to start a "ptestlong" to find out if there were others. I added a commit at public/15140 that fixes them.
Positive review to your code, and as I added a commit it is your turn to review it and change the ticket's status.
Thanks !
Nathann
comment:17 Changed 7 years ago by
 Branch changed from public/groups/abelian_groups_category15140 to public/15140
 Commit changed from eb3696ece93e12a34a2d44441cecaecdf9406ad0 to c127173e1c64fb951cf197c80d288c79eb83aa3c
 Status changed from needs_review to positive_review
LGTM. Thanks Nathann.
PS  I made the branch public so you (or any one else) could just have pushed to it.
New commits:
c127173  trac #15140: Broken doctests

comment:18 Changed 7 years ago by
 Branch changed from public/15140 to c127173e1c64fb951cf197c80d288c79eb83aa3c
 Resolution set to fixed
 Status changed from positive_review to closed
I've added a general
semigroup_generators
for all monoids, which returns the monoid generators and the unit (and assuming finitely generated). I've also defined amonoid_generators
for finite groups inFiniteGroups
instead of anif
statement as a followup to #16725.New commits:
Added monoid generators for general groups.
Added tryexcept block for group_generators to fallback to monoid_generators.
Added message for NotImplementedError for infinitely generated groups.
Changed behavior of general group_generators method.
Fixed category for abelian groups.
Implemented a general semigroup_generators() for monoids.
Moved monoid_generators to a method for finite_groups.