Opened 9 years ago
Closed 8 years ago
#15140 closed enhancement (fixed)
AbelianGroups should be in Groups().Commutative() / Groups().Commutative().Finite()
Reported by: | nthiery | Owned by: | |
---|---|---|---|
Priority: | major | Milestone: | sage-6.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/problem-with-abeliangroupcayley_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 9 years ago by
- Description modified (diff)
comment:2 Changed 8 years ago by
- Milestone changed from sage-6.1 to sage-6.2
comment:3 Changed 8 years ago by
- Milestone changed from sage-6.2 to sage-6.3
comment:4 Changed 8 years ago by
- Cc tscrim added
comment:5 Changed 8 years ago by
- Branch set to public/groups/abelian_groups_category-15140
- Commit set to 757c0449bb1dcc52ab36c11658d8ba653769ca78
- Dependencies changed from #10963 to #10963 #16725
- Status changed from new to needs_review
comment:6 Changed 8 years ago by
- Milestone changed from sage-6.3 to sage-6.4
comment:7 Changed 8 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 follow-up: ↓ 10 Changed 8 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 8 years ago by
- Status changed from needs_review to needs_info
comment:10 in reply to: ↑ 8 ; follow-up: ↓ 11 Changed 8 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 ; follow-up: ↓ 14 Changed 8 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 8 years ago by
- Commit changed from 757c0449bb1dcc52ab36c11658d8ba653769ca78 to 6fd6eda32d044bdca460bfa89a0b870adb1c0553
comment:13 Changed 8 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_category-15140
|
comment:14 in reply to: ↑ 11 ; follow-up: ↓ 15 Changed 8 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 8 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 8 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 8 years ago by
- Branch changed from public/groups/abelian_groups_category-15140 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 8 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 try-except 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.