Ticket #3683 (closed enhancement: fixed)

Opened 5 months ago

Last modified 3 months ago

[with patch, with positive review] meataxe interface

Reported by: wdj Assigned to: joyner
Priority: major Milestone: sage-3.1.2
Component: group_theory Keywords:
Cc: wdj

Description

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

Attachments

10128.patch (5.6 kB) - added by wdj on 08/02/2008 07:56:48 PM.
This is a new patch based on sage 3.1.alpha0. Does not require other patches.
10129.patch (6.2 kB) - added by wdj on 08/12/2008 11:36:47 AM.
based on 3.1.alpha0
10130.patch (5.4 kB) - added by wdj on 08/13/2008 01:48:11 PM.
based on sage-3.1.alpha0, as the others

Change History

07/20/2008 01:15:14 PM changed by was

  • type changed from defect to enhancement.

07/27/2008 07:53:06 AM changed 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.

08/02/2008 02:19:57 PM changed 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"))]]

08/02/2008 07:56:48 PM changed by wdj

  • attachment 10128.patch added.

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

08/02/2008 07:57:32 PM changed by wdj

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

08/10/2008 10:28:36 PM changed by mabshoff

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

08/12/2008 06:38:58 AM changed by SimonKing

  • cc set to wdj.
  • 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]]
    

(follow-up: ↓ 9 ) 08/12/2008 08:36:55 AM changed 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.

08/12/2008 11:36:47 AM changed by wdj

  • attachment 10129.patch added.

based on 3.1.alpha0

08/12/2008 11:37:44 AM changed by wdj

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

(in reply to: ↑ 7 ) 08/13/2008 04:15:08 AM changed by SimonKing

  • cc changed from wdj to mabshoff, wdj.

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

(follow-up: ↓ 11 ) 08/13/2008 08:52:52 AM changed by mabshoff

  • cc changed from mabshoff, wdj to wdj.

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

(in reply to: ↑ 10 ; follow-up: ↓ 13 ) 08/13/2008 09:15:00 AM changed by SimonKing

  • status changed from new to closed.
  • resolution set to fixed.
  • 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?).

08/13/2008 01:48:11 PM changed by wdj

  • attachment 10130.patch added.

based on sage-3.1.alpha0, as the others

08/13/2008 01:57:39 PM changed 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.

(in reply to: ↑ 11 ; follow-up: ↓ 14 ) 08/14/2008 10:36:28 AM changed by aginiewicz

  • status changed from closed to reopened.
  • resolution deleted.

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

(in reply to: ↑ 13 ) 08/14/2008 12:16:10 PM changed 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

08/22/2008 01:01:08 PM changed by mabshoff

  • status changed from reopened to closed.
  • resolution set to fixed.

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