Skip to content

src/sage/groups/perm_gps/permgroup.py: bullet-proof a test #40441

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

Merged
merged 1 commit into from
Aug 2, 2025

Conversation

orlitzky
Copy link
Contributor

There's a test in this file that finds a random subgroup of SymmetricGroup(6) with three generators, round-trips it through a finite presentation, and then checks the result for isomorphism with itself. If you are unlucky, however, GAP will get confused, and raise a GAPError during the round trip. This was originally reported in #32141, and a workaround was added at some point.

The workaround is a bit ugly though, and it does not make a lot of sense to test something that in fact does not work. I have tried all 2024 of them, and the same problem does not arise with subgroups of SymmetricGroup(4), so this commit reverts the workaround and then changes the 6 to a 4. (Five has the same problem that six does.)

Closes: #32141

There's a test in this file that finds a random subgroup of
SymmetricGroup(6) with three generators, round-trips it through a
finite presentation, and then checks the result for isomorphism with
itself. If you are unlucky, however, GAP will get confused, and raise
a GAPError during the round trip. This was originally reported in
issue 32141, and a workaround was added at some point.

The workaround is a bit ugly though, and it does not make a lot of
sense to test something that in fact does not work. I have tried all
2024 of them, and the same problem does not arise with subgroups of
SymmetricGroup(4), so this commit reverts the workaround and then
changes the 6 to a 4. (Five has the same problem that six does.)

Closes: sagemath#32141
@user202729
Copy link
Contributor

The old test asserts that either .is_isomorphic() returns True, or that it raises a GAPError. In either case it is not returning False. So it's indeed testing for something.

This pull request doesn't fix the underlying issue (that Gap sometimes complains) either.

@orlitzky
Copy link
Contributor Author

What should I do instead? The check for GAPError/ValueError was added to get the test passing while we were in the process of making the random tests actually be random (they used to use a fixed seed, so this test would always pass).

GAP gets overwhelmed trying to convert the presentation back to a permutation group, but that's not actually a failure in the method we are testing (as_finitely_presented_group). Even with SymmetricGroup(4), it's still testing the right thing. The finite presentation must be right because we can convert back to some isomorphic permutation group.

@user202729
Copy link
Contributor

On further investigation, this is actually reasonable.

That said, the limit parameter is rather opaque, so I don't know what it is doing. If (⟨ gens | rels⟩) as_permutation_group() (somehow) enumerated the cosets of ⟨rels⟩ in ⟨gens⟩, then it would be sufficient for limit to be set ≥ the size of G, which is 720.

As can be seen from the error, this is not the case.

I cannot reproduce the issue locally either.

@orlitzky
Copy link
Contributor Author

Here is a reproducer:

sage: G = PermutationGroup([(2,5,4,3,6), (1,3,4,6,5), (1,3,2,6,5,4)])
sage: G.as_finitely_presented_group().as_permutation_group().is_isomorphic(G)
---------------------------------------------------------------------------
GAPError                                  Traceback (most recent call last)
...
GAPError: Error, the coset enumeration has defined more than 4096000 cosets
...
ValueError: Coset enumeration exceeded limit, is the group finite?

Though for the mysteriousest of reasons, running the same line again just works:

sage: G.as_finitely_presented_group().as_permutation_group().is_isomorphic(G)
True

@orlitzky
Copy link
Contributor Author

I'm still not sure why running the same command twice works only the second time, but it's probably related to

sage: G.as_finitely_presented_group() == G.as_finitely_presented_group()
False

vbraun pushed a commit to vbraun/sage that referenced this pull request Jul 26, 2025
sagemathgh-40441: src/sage/groups/perm_gps/permgroup.py: bullet-proof a test
    
There's a test in this file that finds a random subgroup of
`SymmetricGroup(6)` with three generators, round-trips it through a
finite presentation, and then checks the result for isomorphism with
itself. If you are unlucky, however, GAP will get confused, and raise a
`GAPError` during the round trip. This was originally reported in
sagemath#32141, and a workaround was
added at some point.

The workaround is a bit ugly though, and it does not make a lot of sense
to test something that in fact does not work. I have tried all 2024 of
them, and the same problem does not arise with subgroups of
`SymmetricGroup(4)`, so this commit reverts the workaround and then
changes the 6 to a 4. (Five has the same problem that six does.)

Closes: sagemath#32141
    
URL: sagemath#40441
Reported by: Michael Orlitzky
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Jul 26, 2025
sagemathgh-40441: src/sage/groups/perm_gps/permgroup.py: bullet-proof a test
    
There's a test in this file that finds a random subgroup of
`SymmetricGroup(6)` with three generators, round-trips it through a
finite presentation, and then checks the result for isomorphism with
itself. If you are unlucky, however, GAP will get confused, and raise a
`GAPError` during the round trip. This was originally reported in
sagemath#32141, and a workaround was
added at some point.

The workaround is a bit ugly though, and it does not make a lot of sense
to test something that in fact does not work. I have tried all 2024 of
them, and the same problem does not arise with subgroups of
`SymmetricGroup(4)`, so this commit reverts the workaround and then
changes the 6 to a 4. (Five has the same problem that six does.)

Closes: sagemath#32141
    
URL: sagemath#40441
Reported by: Michael Orlitzky
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Jul 27, 2025
sagemathgh-40441: src/sage/groups/perm_gps/permgroup.py: bullet-proof a test
    
There's a test in this file that finds a random subgroup of
`SymmetricGroup(6)` with three generators, round-trips it through a
finite presentation, and then checks the result for isomorphism with
itself. If you are unlucky, however, GAP will get confused, and raise a
`GAPError` during the round trip. This was originally reported in
sagemath#32141, and a workaround was
added at some point.

The workaround is a bit ugly though, and it does not make a lot of sense
to test something that in fact does not work. I have tried all 2024 of
them, and the same problem does not arise with subgroups of
`SymmetricGroup(4)`, so this commit reverts the workaround and then
changes the 6 to a 4. (Five has the same problem that six does.)

Closes: sagemath#32141
    
URL: sagemath#40441
Reported by: Michael Orlitzky
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Jul 28, 2025
sagemathgh-40441: src/sage/groups/perm_gps/permgroup.py: bullet-proof a test
    
There's a test in this file that finds a random subgroup of
`SymmetricGroup(6)` with three generators, round-trips it through a
finite presentation, and then checks the result for isomorphism with
itself. If you are unlucky, however, GAP will get confused, and raise a
`GAPError` during the round trip. This was originally reported in
sagemath#32141, and a workaround was
added at some point.

The workaround is a bit ugly though, and it does not make a lot of sense
to test something that in fact does not work. I have tried all 2024 of
them, and the same problem does not arise with subgroups of
`SymmetricGroup(4)`, so this commit reverts the workaround and then
changes the 6 to a 4. (Five has the same problem that six does.)

Closes: sagemath#32141
    
URL: sagemath#40441
Reported by: Michael Orlitzky
Reviewer(s):
vbraun pushed a commit to vbraun/sage that referenced this pull request Jul 29, 2025
sagemathgh-40441: src/sage/groups/perm_gps/permgroup.py: bullet-proof a test
    
There's a test in this file that finds a random subgroup of
`SymmetricGroup(6)` with three generators, round-trips it through a
finite presentation, and then checks the result for isomorphism with
itself. If you are unlucky, however, GAP will get confused, and raise a
`GAPError` during the round trip. This was originally reported in
sagemath#32141, and a workaround was
added at some point.

The workaround is a bit ugly though, and it does not make a lot of sense
to test something that in fact does not work. I have tried all 2024 of
them, and the same problem does not arise with subgroups of
`SymmetricGroup(4)`, so this commit reverts the workaround and then
changes the 6 to a 4. (Five has the same problem that six does.)

Closes: sagemath#32141
    
URL: sagemath#40441
Reported by: Michael Orlitzky
Reviewer(s):
@vbraun vbraun merged commit 2d2b52f into sagemath:develop Aug 2, 2025
21 of 23 checks passed
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.

Unstable doctest involving permutation groups
3 participants