Opened 13 years ago

Closed 13 years ago

#3683 closed enhancement (fixed)

[with patch, with positive review] meataxe interface

Reported by: wdj Owned by: joyner
Priority: major Milestone: sage-3.1.2
Component: group theory Keywords:
Cc: wdj Merged in:
Authors: Reviewers:
Report Upstream: Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

This is a start for anyone interested in G-module decompositions using GAP's Meataxe implementations.

Attachments (3)

10128.patch (5.6 KB) - added by wdj 13 years ago.
This is a new patch based on sage 3.1.alpha0. Does not require other patches.
10129.patch (6.2 KB) - added by wdj 13 years ago.
based on 3.1.alpha0
10130.patch (5.4 KB) - added by wdj 13 years ago.
based on sage-3.1.alpha0, as the others

Download all attachments as: .zip

Change History (18)

comment:1 Changed 13 years ago by was

  • Type changed from defect to enhancement

comment:2 Changed 13 years ago by wdj

  • Summary changed from meataxe interface [with patch, not ready for review] to meataxe interface [with patch, ready for review]

The patch 10104 contains all the changes, is based on 3.0.6.rc0, and passed sage -testall. It does not require 10092, so you should ignore that one. This patch adds module_composition_factors (an interface to GAP's meataxe implementation) and as_permutation_group (which returns an isomorphic PermutationGroup?). The function module_composition_factors is needed for a research paper (in progress, joint with Amy Ksir and Darren Glass) which will probably be presented at the AMS national meeting in January 2009.

comment:3 Changed 13 years ago by SimonKing

Sorry that i was unable to look at this earlier. I'll wait for a patch based on a more recent sage-version before writing a full review.

Note, however, that patch 10104 doesn't do what it is supposed to do.

The doc-string says: Returns a list of triples consisting of [base field, dimension, irreducibility], for each of the Meataxe composition factors modules.

But it only returns a list, as can be seen in the example from the doc-string:

sage: G.module_composition_factors() 
[Finite Field of size 7, 2, True] 

The reason is the line 896:
L = L + [sage_eval(gap.eval("MCF.field")), eval(gap.eval("MCF.dimension")), sage_eval(gap.eval("MCF.IsIrreducible"))]
which should be
L = L + [[sage_eval(gap.eval("MCF.field")), eval(gap.eval("MCF.dimension")), sage_eval(gap.eval("MCF.IsIrreducible"))]]

Changed 13 years ago by wdj

This is a new patch based on sage 3.1.alpha0. Does not require other patches.

comment:4 Changed 13 years ago by wdj

The attached patch 10128 fixes the bug Simon found (thanks Simon!).

comment:5 Changed 13 years ago by mabshoff

  • Summary changed from meataxe interface [with patch, ready for review] to [with patch, ready for review] meataxe interface

comment:6 Changed 13 years ago by SimonKing

  • Cc wdj added
  • Summary changed from [with patch, ready for review] meataxe interface to [with patch, positive review pending] meataxe interface

The patch applies cleanly to SAGE Version 3.1.alpha0, Release Date: 2008-08-01. It seems to do what it is supposed to do, and the doc-tests for matrix_group.py pass.

Therefore, i recommend inclusion of the patch.

However, i would be glad about "stronger" examples.

  • Is there an example for as_permutation_group where the option method="smaller" actually yields a smaller result? Then it would be nice to include such example.
  • It would be nice to see an example where module_composition_factors yields a non-trivial decomposition. Such as here:
    sage: F=GF(3);MS=MatrixSpace(F,4,4)
    sage: M=MS(0)
    sage: M[0,1]=1;M[1,2]=1;M[2,3]=1;M[3,0]=1
    sage: G.module_composition_factors()
    
    [[Finite Field of size 3, 1, True],
     [Finite Field of size 3, 1, True],
     [Finite Field of size 3, 2, True]]
    

comment:7 follow-up: Changed 13 years ago by wdj

Definitely, I'm happy to add the example to the dcstring of module_composition_factors. Thanks for that.

Regarding a "better" "smaller" example, they are not so easy to find! I did find one though. The problem is that the generators are returned randomly. Michael Abshoff told me he doesn't like " # random output" comments in docstrings, so I added a the command current_randstate().set_seed_gap(). This does not work as I think it should, so I don't know the right way to proceed. I guess I'll post a patch that pases tests and worry about the random output stuff later.

Changed 13 years ago by wdj

based on 3.1.alpha0

comment:8 Changed 13 years ago by wdj

This latest patch passes sage -testall and adds the examples suggested by the referee. Thanks Simon!

comment:9 in reply to: ↑ 7 Changed 13 years ago by SimonKing

  • Cc mabshoff added

Replying to wdj:

Regarding a "better" "smaller" example, they are not so easy to find! I did find one though. The problem is that the generators are returned randomly. Michael Abshoff told me he doesn't like " # random output" comments in docstrings,

Cc to Michael Abshoff.

I understand that Gap uses a randomized algorithm when getting method="smaller". Hence, if one wants to show the full functionality of a method to the user (which i find important!), one can not avoid to have #random in the doc-tests.

Michael, what do you think?

I think:

  • Starting with 3.1.alpha0, applying patch 10128 and then applying patch 10129 works.
  • The methods are useful.
  • The doc-string shows the functionality
  • The doc-tests pass

Hence i give it a positive review, but make it dependent on Michael's opinion on random doc-tests and/or on the idea to use current_randstate().set_seed_gap().

comment:10 follow-up: Changed 13 years ago by mabshoff

  • Cc mabshoff removed

David,

no need to CC me, I read every ticket anyway.

About randomness: GAP should behave deterministically unless there is a third rng we do not know about. For now it seems fine to add the #random to the doctests, but you might want to raise the issue on sage-devel so that Carl Witty can give his input on the problem.

Cheers,

Michael

comment:11 in reply to: ↑ 10 ; follow-up: Changed 13 years ago by SimonKing

  • Resolution set to fixed
  • Status changed from new to closed
  • Summary changed from [with patch, positive review pending] meataxe interface to [with patch, with positive review] meataxe interface

Replying to mabshoff:

no need to CC me, I read every ticket anyway.

Very impressive!

... For now it seems fine to add the #random to the doctests, ...

Then there is a positive review from my side and (if i am allowed to do so) I resolve the ticket as fixed (or is this only allowed to administrators?).

Changed 13 years ago by wdj

based on sage-3.1.alpha0, as the others

comment:12 Changed 13 years ago by wdj

The last patch 10130 is a docstring change only.

Following Michael Abshoff's suggestion, I emailed sage-devel and mentioned the problem I was having with the random comments. It seems I was using the current_randstate().set_seed_gap() command incorrectly for the situation. I added some set_random_seed(n) statements (where n is chosen in a specific way) and removed the "# random output" comments. I did multiple test passes and this seems to work each time now.

Hopefully, with 10130, everyone is okay with this now.

comment:13 in reply to: ↑ 11 ; follow-up: Changed 13 years ago by aginiewicz

  • Resolution fixed deleted
  • Status changed from closed to reopened

Replying to SimonKing:

Then there is a positive review from my side and (if i am allowed to do so) I resolve the ticket as fixed (or is this only allowed to administrators?).

iirc it was that only things that get merged in are closed by admins? :)

comment:14 in reply to: ↑ 13 Changed 13 years ago by mabshoff

Replying to aginiewicz:

Replying to SimonKing:

Then there is a positive review from my side and (if i am allowed to do so) I resolve the ticket as fixed (or is this only allowed to administrators?).

iirc it was that only things that get merged in are closed by admins? :)

Yes, the release manager closes tickets once the patch/spkg has been merged. How else would be keep track of all the patches?

Cheers,

Michael

comment:15 Changed 13 years ago by mabshoff

  • Resolution set to fixed
  • Status changed from reopened to closed

Merged 10128.patch, 10129.patch and 10130.patch in Sage 3.1.2.alpha0

Note: See TracTickets for help on using tickets.