Do not canonicalize generators when building a permutation group from a libGAP group

[tscrim]

When creating a permutation group in Sage from a libGAP group, the default is to canonicalize the generators. However, when setting the libGAP group to the Sage permutation group, it still knows about its original generators, which can then make the objects out of sync with each other. In particular, if one tries to construct a group homorphism:

sage: A = AlternatingGroup(6)
sage: h = A.hom(codomain=A, im_gens=A.gens())  # the identity map

sage: lgg = libgap.eval("Group((1,2,3,4,5),(4,5,6))")
sage: P = PermutationGroup(gap_group=lgg)
sage: h = P.hom(codomain=P, im_gens=P.gens())

We should not canonicalize the group generators when setting the libgap group of the Sage group to the inputted group.

📝 Checklist

• The title is concise and informative.
• The description explains in detail what this PR is about.
• I have linked a relevant issue or discussion.
• I have created tests covering the changes.
• I have updated the documentation accordingly.

⌛ Dependencies

[dimpase]

Shouldn't this be also done for pexpect GAP groups?
I get the same error if I instead do

sage: lgg = gap.eval("Group((1,2,3,4,5),(4,5,6))")
...

here the type of lgg is just str. So this is the case dealt with in

if isinstance(gap_group, str):

[enriqueartal]

It seems right for me. Including canonicalize=False for pexpect GAP groups only creates test errors due to the new order. There are some more files with failing doctests; once they are changed, for me it is correct.

[tscrim]

@dimpase I added a test for that case showing that it works with the change. Is that what you meant?

[dimpase]

hmm, OK, it's all good.

[dimpase]

the CI run shows this error:

File "src/sage/groups/class_function.py", line 719, in sage.groups.class_function.ClassFunction_gap.restrict
Failed example:
    chi.restrict(H)
Expected:
    Character of Subgroup generated by [(4,5), (1,2), (1,2,3)] of
     (Symmetric group of order 5! as a permutation group)
Got:
    Character of Subgroup generated by [(1,2,3), (1,2), (4,5)] of (Symmetric group of order 5! as a permutation group)

[tscrim]

I thought I got all the tests, but it seems I missed one. Fixed.

[github-actions]

Documentation preview for this PR (built with commit 2a51c3e; changes) is ready! 🎉

[tscrim]

Thank you.

(Unfortunately this means we will be more susceptible to changes in GAP's output order when versions change, but so be it.)

[enriqueartal]

Thank you.

(Unfortunately this means we will be more susceptible to changes in GAP's output order when versions change, but so be it.)

Apparently soon, see issue #37616

[dimpase]

one just have to rewrite tests like this in an output-insensitive way.

[enriqueartal]

one just have to rewrite tests like this in an output-insensitive way.

Right. I think what is important is that the generators in sage and gap coincide since gap is going to do the hard work.

[tscrim]

one just have to rewrite tests like this in an output-insensitive way.

I think that is far easier said than done, and it is not always useful to a user looking at the examples. It might just be something we have to deal with when upgrading GAP…

[tornaria]

one just have to rewrite tests like this in an output-insensitive way.

I think that is far easier said than done, and it is not always useful to a user looking at the examples. It might just be something we have to deal with when upgrading GAP…

We must try to come up with some solution, because it's not feasible to assume everybody in every context is using the exact same version of gap.

In particular, what shall we do about gap 4.13 (#37616)?

[enriqueartal]

one just have to rewrite tests like this in an output-insensitive way.

I think that is far easier said than done, and it is not always useful to a user looking at the examples. It might just be something we have to deal with when upgrading GAP…

We must try to come up with some solution, because it's not feasible to assume everybody in every context is using the exact same version of gap.

In particular, what shall we do about gap 4.13 (#37616)?

If the failure of tests is only due to change of orders, it is not necessary a big issue

[tornaria]

one just have to rewrite tests like this in an output-insensitive way.

I think that is far easier said than done, and it is not always useful to a user looking at the examples. It might just be something we have to deal with when upgrading GAP…

We must try to come up with some solution, because it's not feasible to assume everybody in every context is using the exact same version of gap.
In particular, what shall we do about gap 4.13 (#37616)?

If the failure of tests is only due to change of orders, it is not necessary a big issue

If the tests give different answers for 4.12 than for 4.13, it means they will only pass with one particular version of gap.

The change has other implications, for instance conjugacy classes are in a different order, which makes character tables different, etc.

It's not an easy problem, but it's very critical to support system packages. We are facing a similar issue with singular (#37492).

[fingolfin]

Even with the exact same version of GAP, the orders of generators, conjugacy classes etc. can vary if you re-run the same code, due to the use of randomized algorithms in many places.

[dimpase]

Indeed - and it's alarming, as Sage does testing with different random seeds (quite a few real bugs got detected this way), but apparently these seeds don't reach GAP's settings.

[tscrim]

Indeed - and it's alarming, as Sage does testing with different random seeds (quite a few real bugs got detected this way), but apparently these seeds don't reach GAP's settings.

That could probably be fixed with how we set up our testing framework to pass along that seed to GAP.

[tscrim]

If the tests give different answers for 4.12 than for 4.13, it means they will only pass with one particular version of gap.

The change has other implications, for instance conjugacy classes are in a different order, which makes character tables different, etc.

It's not an easy problem, but it's very critical to support system packages. We are facing a similar issue with singular (#37492).

There are cases where this order is important because the documentation is demonstrating how to do a particular computation on a particular character/conjugacy class/etc. (Not to mention the cases where having a fixed order makes understanding the function and its outputs much easier.) We might need to have a more dynamic documentation that depends on different versions (we had similar sort of tags for py2 vs py3), or perhaps some general disclaimer that the order might change (via some tag that implicitly sorts output like for a dict output). However, making outputs agnostic to output order should not be used universally IMO (which I believe you need).

[dimpase]

for the purpose of demonstrating one can mark such EXAMPLES #random, and do real testing in TESTS:

[tscrim]

@dimpase That would be the easy way out, but it sends a bit of the wrong message. The doctests are not actually random, they just depend on the version of GAP. A user should know the output is the same across runs and sessions (as they will find it useful).

[mantepse]

@dimpase That would be the easy way out, but it sends a bit of the wrong message. The doctests are not actually random, they just depend on the version of GAP. A user should know the output is the same across runs and sessions (as they will find it useful).

Actually, I think it is - additionally - important for a user to know that the order of the output may change across versions.

[dimpase]

output may also change if a GAP package is installed or removed

[tscrim]

@dimpase That would be the easy way out, but it sends a bit of the wrong message. The doctests are not actually random, they just depend on the version of GAP. A user should know the output is the same across runs and sessions (as they will find it useful).

Actually, I think it is - additionally - important for a user to know that the order of the output may change across versions.

As a general principle, I agree with you (especially when they change based on the packages installed/used). Although strictly speaking that is impossible to know as we cannot control what GAP might change in the future. However, if a user is relying on a particular order, then a user should understand that this could change across versions (and generally should just construct the element directly, which is not always useful for pedagogical purposes in docstrings). We don’t make an explicit promise that iteration orders in Sage do not change from version to version.

[tornaria]

@tscrim:

There are cases where this order is important because the documentation is demonstrating how to do a particular computation on a particular character/conjugacy class/etc. (Not to mention the cases where having a fixed order makes understanding the function and its outputs much easier.) We might need to have a more dynamic documentation that depends on different versions (we had similar sort of tags for py2 vs py3), or perhaps some general disclaimer that the order might change (via some tag that implicitly sorts output like for a dict output). However, making outputs agnostic to output order should not be used universally IMO (which I believe you need).

@dimpase :

for the purpose of demonstrating one can mark such EXAMPLES #random, and do real testing in TESTS:

Maybe we could have a # random_order tag that somehow compares up to order? I don't think that solves everything (because, well... different order of a basis may end up having some impact that it's not so obvious) but at least it saves us from making the example artificially sort the output when there's no need except for checking. I think there are many tests that sort before printing to workaround this.

The general problem is how to have a framework for "testing examples" which allows to have alternate comparison functions somehow. I.e., we want to test that the character table printed out matches (in value and format) the one that is shown as part of the example, but we want to accept the variations that can change under different versions of the software.