Opened 7 years ago

Closed 6 years ago

#13102 closed enhancement (fixed)

PermutationGroup.all_blocks from GAP

Reported by: ncohen Owned by: joyner
Priority: major Milestone: sage-5.8
Component: group theory Keywords:
Cc: wdj, was, jasonbhill Merged in: sage-5.8.beta3
Authors: Nathann Cohen Reviewers: Benjamin Jones
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by benjaminfjones)

Helloooooooo !!

This patch exposes the all_blocks method from GAP. I am not familiar with Sage's groups nor with the interface with GAP, so please feel free to teach me the basics while reviewing this patch ^^;

Nathann

---

Apply:

Attachments (3)

trac_13102.2.patch (4.1 KB) - added by nborie 6 years ago.
With dots for doc…
trac_13102.patch (4.6 KB) - added by ncohen 6 years ago.
trac_13102_reviewer_patch.patch (1.4 KB) - added by benjaminfjones 6 years ago.
reviewer patch

Download all attachments as: .zip

Change History (19)

comment:1 Changed 7 years ago by ncohen

  • Cc wdj was added
  • Status changed from new to needs_review

comment:2 Changed 6 years ago by jasonbhill

  • Cc jasonbhill added

comment:3 Changed 6 years ago by jasonbhill

I like this, but I'd like to bring up some issues and propose solutions. I'll help with writing, as much of this will depend on code I've written to the relevant file.

Issues:

  1. We should implement more than just "AllBlocks" from GAP. First, this creates a naming problem for tab completion in sage. I propose the names such as "blocks," "blocks_maximal," "blocks_all_representatives," and so on.
  2. GAP and Sage view permutation group domains differently. With certain automatically generated structures, this won't be an issue. But, with a group like <(2,3)>, which is viewed as transitive in GAP and intransitive in Sage (it fixes 1), we have an issue. I'm going to write the "non_fixed_points()" command that currently exists as an alias to "support()" and redefine the transitivity, regularity, etc. commands with this instead. (That will be backwards compatible. It's just that "support()" is more consistent with literature than "non_fixed_points()" and we should be using that if we're talking about blocks.) That's really a trivial change, but I should have realized this when I wrote "non_fixed_points()" in the first place. My bad. It will still exist and function in any case.
  3. What isn't trivial is how these functions should apply to various groups. I've seen on the GAP lists some questions about GAP's block functions. The literature does largely discuss blocks for only transitive groups, but the definition of a block doesn't require it. As my research area is intransitive groups, this annoys me and GAP's functions are pretty inadequate. I think it should be relatively obvious that a maximal non-trivial block system for an intransitive group should simply consist of the orbits. A generic block system for an intransitive group should return a non-trivial block system on orbits, including the consideration of fixed points sitting outside of a transitive component (as happens in Sage and does not in GAP). I'll work on this and come up with something that makes sense.
  4. As in GAP, we should have optional domain and seed parameters. This should function seamlessly with issue (3). I'll work on that.

comment:4 Changed 6 years ago by ncohen

Hellooooooooooooooo Jason !

Well, I just updated my patch to address your 1) point, but I do not really see how I can be of help with the rest :-)

Nathann

comment:5 Changed 6 years ago by ncohen

Ping ? :-P

Nathann

Changed 6 years ago by nborie

With dots for doc...

comment:6 Changed 6 years ago by benjaminfjones

Hi Nathann,

Looks good, I'm happy to see more of GAP wrapped. A couple of suggestions,

  1. Strictly speaking, the function doesn't return a list of blocks, but as the GAP manual puts it: "a list of representatives of all block systems for a permutation group G acting transitively on the points moved by the group." I think it's clear from the output that what a user is getting is not a partition of the set, but a list of reps for an invariant partition, but it would be nice if the documentation mentions this.
  1. In the INPUT description, I would change representants --> representatives.
  1. Add a description of OUTPUT. In particular, I would have expected the function to return a list of lists of ints in all cases (each inner list representing a partition of the set). Instead, it returns either a list of lists, or a list of lists of lists depending on the input.

Otherwise, looks great to me! I think the enhancements @jasonbhill has brought up should be made a new ticket (which he can create if he's still interested).

comment:7 Changed 6 years ago by benjaminfjones

  • Reviewers set to Benjamin Jones
  • Status changed from needs_review to needs_work
  • Work issues set to doc changes

comment:8 Changed 6 years ago by ncohen

  • Status changed from needs_work to needs_review

Helloooooooooooooooooooo !!!

Thank you for your help !

I just tried to make those changes (tell me what you think). Your remarks were totally right, but there was no obvious way to explain cleanly the output of this method, so I hope you will like the one I chosed. Oh, and do tell me if some sentences are awkward, I am not used to this vocabulary at all :-)

Nathann

Changed 6 years ago by ncohen

comment:9 Changed 6 years ago by benjaminfjones

Yeah, the output description makes it clear now what the user can expect to get as a result. I see a couple of very minor grammatical issues. It's probably easier if I just post a reviewer patch and this should be good to go!

comment:10 Changed 6 years ago by ncohen

Well, then you can either post your patch or tell me what they are ! :-P

Nathann

Changed 6 years ago by benjaminfjones

reviewer patch

comment:11 Changed 6 years ago by benjaminfjones

  • Work issues doc changes deleted

There you go! If you're ok with the changes I made, I'll set to positive review.

Patchbot: apply attachment: trac_13102.patch, trac_13102_reviewer_patch.patch

comment:12 Changed 6 years ago by ncohen

Oh. Right. I should have seen those ! You can set the ticket to positive_review whenever you want, then. THank youuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuuu for this review !!! :-)

Nathann

comment:13 Changed 6 years ago by benjaminfjones

  • Description modified (diff)

comment:14 Changed 6 years ago by benjaminfjones

Patchbot: apply trac_13102.patch trac_13102_reviewer_patch.patch

comment:15 Changed 6 years ago by benjaminfjones

  • Status changed from needs_review to positive_review

comment:16 Changed 6 years ago by jdemeyer

  • Merged in set to sage-5.8.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.