fix hash of PermutationGroup_generic

[mantepse]

Currently, PermutationGroup_generic inherits its hash method from CategoryObject, which uses the string representation of the object. However, string representations of equal permutation groups may be different, which leads to a bug:

            sage: G = PermutationGroup([(1,2,3), (1,3)])
            sage: G == SymmetricGroup(3)
            True
            sage: G in set([SymmetricGroup(3)])

In current sage, this yields False.

We base the hash on the cardinality of the group. This is not ideal, because it may take some time to compute it, but I doubt that there is anything cheaper available.

I am hesitant to include the degree, because #36912 is not entirely settled.

[github-actions]

Documentation preview for this PR (built with commit b758a92; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

[tscrim]

This makes hashing the perm groups very slow as it has to compute the order every time it is being hashed. That can be mostly solved if we cache order() though, which is probably a good thing to do anyways...

[mantepse]

Done, yes that's a good idea, thank you!

[mantepse]

I must say that the handling of order and cardinality of groups is rather strange.

• categories.FiniteGroups.ParentMethods.cardinality checks whether order is available, otherwise resorts to counting. It is mentioned there that finite groups should implement cardinality.
• groups.group.Group implements order by calling cardinality, and if that's not available raises a NotImplementedError.
• some groups define cardinality and make order an alias, for example ArtinGroup, and BraidGroup, but also FinitelyPresentedGroup.
• some groups implement order and make cardinality an alias, for example AbelianGroup_class, CubicBraidGroup, HeisenbergGroup, BinaryDihedralGroup, PariGroup

[tscrim]

I would say it is done that way so you only need to implement one, but other groups can override both for speed (not that they are likely to be called often that though).

[tscrim]

I don't understand why you are removing the alias here.

Also, there are a number of slow doctests being reported with this change. Do you see speed regressions when testing this locally? (It is probably be unavoidable though.)

[tscrim]

This is no longer true with this change. Although I still don't see why you need to change how this is done with defining order and then making cardinality an alias. There is no harm in having it IMO.

[dimpase]

please rebase