Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

meataxe interface #3683

Closed
wdjoyner opened this issue Jul 20, 2008 · 16 comments
Closed

meataxe interface #3683

wdjoyner opened this issue Jul 20, 2008 · 16 comments

Comments

@wdjoyner
Copy link

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

CC: @wdjoyner

Component: group theory

Issue created by migration from https://trac.sagemath.org/ticket/3683

@wdjoyner wdjoyner added this to the sage-3.1.2 milestone Jul 20, 2008
@wdjoyner wdjoyner self-assigned this Jul 20, 2008
@wdjoyner
Copy link
Author

comment:2

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.

@wdjoyner wdjoyner changed the title meataxe interface [with patch, not ready for review] meataxe interface [with patch, ready for review] Jul 27, 2008
@simon-king-jena
Copy link
Member

comment:3

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"))]]

@wdjoyner
Copy link
Author

wdjoyner commented Aug 3, 2008

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

@wdjoyner
Copy link
Author

wdjoyner commented Aug 3, 2008

comment:4

Attachment: 10128.patch.gz

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

@sagetrac-mabshoff sagetrac-mabshoff mannequin changed the title meataxe interface [with patch, ready for review] [ready for review] meataxe interface Aug 11, 2008
@simon-king-jena
Copy link
Member

comment:6

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

@simon-king-jena simon-king-jena changed the title [ready for review] meataxe interface [positive review pending] meataxe interface Aug 12, 2008
@wdjoyner
Copy link
Author

comment:7

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.

@wdjoyner
Copy link
Author

Attachment: 10129.patch.gz

based on 3.1.alpha0

@wdjoyner
Copy link
Author

comment:8

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

@simon-king-jena
Copy link
Member

comment:9

Replying to @wdjoyner:

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

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Aug 13, 2008

comment:10

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

@simon-king-jena
Copy link
Member

comment:11

Replying to @sagetrac-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?).

@simon-king-jena simon-king-jena changed the title [positive review pending] meataxe interface meataxe interface Aug 13, 2008
@wdjoyner
Copy link
Author

based on sage-3.1.alpha0, as the others

@wdjoyner
Copy link
Author

comment:12

Attachment: 10130.patch.gz

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.

@sagetrac-aginiewicz
Copy link
Mannequin

sagetrac-aginiewicz mannequin commented Aug 14, 2008

comment:13

Replying to @simon-king-jena:

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

@sagetrac-aginiewicz sagetrac-aginiewicz mannequin reopened this Aug 14, 2008
@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Aug 14, 2008

comment:14

Replying to @sagetrac-aginiewicz:

Replying to @simon-king-jena:

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

@sagetrac-mabshoff
Copy link
Mannequin

sagetrac-mabshoff mannequin commented Aug 22, 2008

comment:15

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

@sagetrac-mabshoff sagetrac-mabshoff mannequin closed this as completed Aug 22, 2008
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants