Commutator subgroup of permutation group ignores the domain

[DaveWitteMorris]

Steps To Reproduce

As pointed out in this sage-devel thread, if a permutation group has a nonstandard domain, then the commutator method can return an incorrect result:

sage: G = PermutationGroup(gens=[(1,2), (2,4)], domain={1, 2, 4})
sage: G.commutator()
Permutation Group with generators [(1,2,3)]

Expected Behavior

The commutator subgroup should be generated by the cycle (1,2,4).

Actual Behavior

Sagemath reports that the commutator subgroup is generated by the cycle (1,2,3), which is not even in the group, because 3 is not in the domain.

Additional Information

I think the problem is that the commutator method ignores the domain. Line 3348 of src/sage/groups/perm_gps/permgroup.py is

            return PermutationGroup(gap_group=libgap.DerivedSubgroup(self))

but I think it should be

            return PermutationGroup(gap_group=libgap.DerivedSubgroup(self), domain=self.domain())

Environment

any

Checklist

• I have searched the existing issues for a bug report that matches the one I want to file, without success.
• I have read the documentation and troubleshoot guide

[AdityaK1729]

I applied the solution you mentioned and its working fine now so would you mind if i open the PR

[ypfmde]

Before opening a PR, one probably should check that other subgroup methods aren't affected by a similar issue. I'm not familiar enough with the interaction between Sage groups and Gap's libgap groups. In fact, I do not understand why for the same example G.sylow_subgroup(3) returns the correct group: In src/sage/groups/perm_gps/permgroup.py, we have
return self.subgroup(gap_group=self._libgap_().SylowSubgroup(p)). But

G = PermutationGroup(gens=[(1,2), (2,4)], domain={1, 2, 4})
gg = G._libgap_().SylowSubgroup(3)
print(gen_group)

returns the wrong group Group([ (1,3,2) ]), however G.subgroup(gap_group=gg) eventually yields the correct group, even though gg isn't a subgroup of G.

[DaveWitteMorris]

_libgap_ is a private method, so you would need to dig into the source code if you want to know what it's doing, but here's an explanation of why sylow_subgroup is doing the right thing.

gg isn't supposed to be a subgroup of G -- it's a subgroup of G._libgap_(), which is a different permutation group, because you specified a domain for the sage group. When converting G to a _libgap_ group, your custom domain needs to be converted to [1,2,3] (because GAP only works with permutations of integers but elements of a custom domain could be anything). So elements of G._libgap_() are permutations of [1,2,3]. GAP calculates the correct Sylow subgroup of G._libgap_(), and then this is converted back to a subgroup of G.

The problem with the commutator method is that it calculates the commutator subgroup as a libgap group and uses PermutationGroup to construct a sage group directly from that, without doing a conversion back to the domain that was specified for the sage group. In your Sylow subgroup example, the subgroup method does the conversion because it is a method of G and therefore knows what conversion is needed: it calls the Subgroup method, which executes domain = ambient.domain() to set the domain of the subgroup equal to the domain of the ambient group G.

[DaveWitteMorris]

@AdityaK1729, it's fine with me if you want to do the pull request. However, there also needs to be a doctest, such as:

        Verify that :issue:`39416` is fixed::

            sage: G = PermutationGroup(gens=[("a", "b"), ("b","c")])
            sage: G.commutator()
            Permutation Group with generators [('a','b','c')]

In addition, as ypfmde mentioned, other similar problems in the file should also be fixed. Are you going to do those, too, or just the one above?

I looked at the other occurrences of PermutationGroup in src/sage/groups/perm_gps/permgroup.py, and I think these are the other problems of this type:

====

In intersection and the last part of commutator, need to add something like this:

	# Will get wrong results unless `self.domain_to_gap` is compatible
	# with `other.domain_to_gap`. This requires one of the domains to
	# be an initial segment of the other.
	D1 = self.domain()
	D2 = other.domain()
	if len(D1) > len(D2):
		D1, D2 = D2, D1
	if D1 != D2[:len(D1)]
		raise NotImplementedError("domain of one group must be an initial segment of the domain of the other group")

Then it should be enough to let domain=D2 in the call to PermutationGroup.

====

semidirect_product raises an error when the domain of either of the groups is not (positive) integers, because it tries to apply libgap functions without first converting to permutations that libgap can understand. For now, I think this limitation should just be documented. (I think this code was written before we started using libgap, so it would benefit from refactoring, but that would be a separate ticket.)

====

conjugate can raise an error or give unexpected results when the domain of the group is not [1,2,...,n]. It needs to be documented that this method should not be applied with custom domains. It might be good to raise a NotImplementedError, but that might not be part of this ticket.

[DaveWitteMorris]

Also, instead of my original suggestion, I think a clearer way to fix the original issue for G.commutator() might be

            return self.subgroup(gap_group=libgap.DerivedSubgroup(self))

[DaveWitteMorris]

@AdityaK1729: Are you going to do the pull request for the original issue (and add a doctest)? Or should I have somone else do it?

I will get the other 3 issues fixed.

[AdityaK1729]

I was gonna do it but its currently my exams going on, so if you can give me 1 week I believe I can complete it

[DaveWitteMorris]

Yes, there is no rush. I will leave that one to you, but will try to get the others done.

[AdityaK1729]

Hi, a silly question but are doctests supposed to be added in the same document, I am not quite able to get it
Thanks

[DaveWitteMorris]

You are fixing a bug in the commutator method, so the doctest belongs in the docstring of that method. I think the docstring does not currently have a TESTS: block (only an EXAMPLES: block), so I think you will need to add TESTS: at the end of the docstring, then a blank line, and then your doctest.

[AdityaK1729]

I have made a PR #39624 and added the doctests, please check and lmk if some changes are needed