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: 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:

Status badges

Description (last modified by nthiery)

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 8 years ago by nthiery

  • Description modified (diff)

comment:2 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:3 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:4 Changed 7 years ago by tscrim

  • Cc tscrim added

comment:5 Changed 7 years ago by tscrim

  • Authors set to Travis Scrimshaw
  • 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

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 a monoid_generators for finite groups in FiniteGroups instead of an if statement as a followup to #16725.


New commits:

0791169Added monoid generators for general groups.
e5cd7ccAdded try-except block for group_generators to fallback to monoid_generators.
c64a5fbAdded message for NotImplementedError for infinitely generated groups.
796262dChanged behavior of general group_generators method.
4606f0eFixed category for abelian groups.
2d58f4fImplemented a general semigroup_generators() for monoids.
757c044Moved monoid_generators to a method for finite_groups.

comment:6 Changed 7 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:7 Changed 7 years ago by tscrim

  • 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: Changed 7 years ago by ncohen

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 ncohen

  • Status changed from needs_review to needs_info

comment:10 in reply to: ↑ 8 ; follow-up: Changed 7 years ago by tscrim

  • 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 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.

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

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 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).

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 git

  • Commit changed from 757c0449bb1dcc52ab36c11658d8ba653769ca78 to 6fd6eda32d044bdca460bfa89a0b870adb1c0553

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

e8f3aaaMerge branch 'public/groups/abelian_groups_category-15140' of trac.sagemath.org:sage into public/groups/abelian_groups_category-15140
6fd6edaAdded multiplicative abelian groups to the catalog.

comment:13 Changed 7 years ago by git

  • Commit changed from 6fd6eda32d044bdca460bfa89a0b870adb1c0553 to eb3696ece93e12a34a2d44441cecaecdf9406ad0

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

eb3696eMerge branch 'develop' into public/groups/abelian_groups_category-15140

comment:14 in reply to: ↑ 11 ; follow-up: Changed 7 years ago by tscrim

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 all FiniteGroups have their own implementation now, but which kind of group is not a FiniteGroup but is a FiniteEnumeratedSet ?

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 ncohen

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 ncohen

  • 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 tscrim

  • 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:

c127173trac #15140: Broken doctests

comment:18 Changed 7 years ago by vbraun

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