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

Fix nice monomorphism dispatch for HallSubgroup #559

Merged
merged 2 commits into from
Jan 31, 2016

Conversation

markuspf
Copy link
Member

Fixes issue #557

Will this work when HallSubgroup returns a list of groups?

Also I will add some tests.

@bh11
Copy link
Contributor

bh11 commented Jan 30, 2016

Could you also add the case when HallSubgroup returns a list of groups? This is the documented behaviour when there is more than one conjugacy class of Hall subgroups. SL(2,11) with π={2,3} is an example where this happens.

@markuspf
Copy link
Member Author

@bh11 I just had to produce an example where HallSubgroup actually is a list of groups (I used PSL(2,7), but needed it's matrix representation, so went to the atlas...) Long story short: Yes, I am going to do this now.

@markuspf
Copy link
Member Author

Voila. Also added test cases.

fail
gap> G := PSL(4,2);;
gap> HallSubgroup(G, [2,3]);
<permutation group of size 576 with 8 generators>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There might be problem running this test with different packages loaded, because the presentation of the answer might change. Maybe ask for an IdGroup, or something similar?

@markuspf
Copy link
Member Author

I did't realise that someone had started adjusting tests for possible isomorphisms. This is certainly not done in many places, and we do know that this is a problem, because sometimes changing algorithms gives different, but isomorphic, results.

In any case, I added IdGroup, it even works for the matrix groups, presumably because they are tiny.

@hulpke
Copy link
Contributor

hulpke commented Jan 31, 2016

@markuspf Thank you very much for adding this -- when I implemented the Hall subgroup code I only tested matrix groups when using the matgrp package, and thus did not spot the need to extend nice monomorphism. Sorry!

@hungaborhorvath
Copy link
Contributor

@markuspf As far As I know, two tests are run, one with all packages loaded and one with no packages loaded. Sometimes this can make differences in the output presentation. Probably @alex-konovalov could explain it better.

Anyway, thanks for this.

markuspf added a commit that referenced this pull request Jan 31, 2016
Fix nice monomorphism dispatch for HallSubgroup
@markuspf markuspf merged commit 1e4dea9 into gap-system:master Jan 31, 2016
@markuspf markuspf deleted the fix-hall-subgroup-dispatch branch February 5, 2017 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants