sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.42k stars 476 forks source link

IntegerVectorsModPermutationGroup confuses groups whose domains are different #36527

Open jukkakohonen opened 1 year ago

jukkakohonen commented 1 year ago

Steps To Reproduce

In SageMath version 9.0, running this:

A = PermutationGroup([(1,2)], domain=[1,2])
print("with domain ", A.domain())
print(IntegerVectorsModPermutationGroup(A, 3).list())

B = PermutationGroup([(1,2)], domain=[1,2,3])
print("with domain ", B.domain())
print(IntegerVectorsModPermutationGroup(B, 3).list())

Expected Behavior

In A , the domain has two elements, so I am expecting lists of two elements that add up to 3:

with domain  {1, 2}
[[3, 0], [2, 1]]

In B, the domain has three elements, so I am expecting lists of three elements that add up to 3:

with domain  {1, 2, 3}
[[3, 0, 0], [2, 1, 0], [2, 0, 1], [1, 1, 1], [1, 0, 2], [0, 0, 3]]

Actual Behavior

From both groups we get the same listing:

with domain  {1, 2}
[[3, 0], [2, 1]]
with domain  {1, 2, 3}
[[3, 0], [2, 1]]

Additional Information

If the order of execution is reversed, so that we do B first and A second, then B gives the correct three-element lists, and A gives incorrectly the three-element lists.

So, the first one gets computed correctly, and the second one incorrectly. It looks like IntegerVectorsModPermutationGroup is remembering that it has already seen this group, not noticing that is in fact a different permutation group with different domain.

Environment

- **OS**: Ubuntu 20.04
- **Sage Version**: 9.0

Checklist

mantepse commented 10 months ago

Doesn't this mean that for domains other than {1,...n} we are only able to test equality up to conjugacy. I admit I'm not sure. With 'Set' I am quite sure that we cannot guarantee a fixed order, can we guarantee it with 'set'?

I think I should clarify my statement: If we exclusively use gap to check for equality and we do not have the same total ordering of a set (we shouldn't use Set anyway) throughout a sage session, then we can test only up to conjugacy.

However, it may well be that python guarantees that list(set(X)) always produces the same result within one session, I don't know.

I think it doesn't matter that the ordering differs in different sessions.

jukkakohonen commented 10 months ago

Doesn't this mean that for domains other than {1,...n} we are only able to test equality up to conjugacy. I admit I'm not sure. With 'Set' I am quite sure that we cannot guarantee a fixed order, can we guarantee it with 'set'?

If we cannot even consistently test two finite S(s)ets for equality, it's pretty much end of the project...

Surely we can test two finite S(s)ets for equality. And if they are equal, presumably it means they contain the same objects. If you have X == Set("abcd") and Y == Set("abcd"), you also have X==Y and each element of X appears, just once, in Y.

Sure, if you list X and Y independently, in arbitrary orders, and map the elements to integers in that order, your "a" of X can map to a different integer than your "a" of Y. That is what happens now, and leads to inconsistency (really equal groups becoming inequal in GAP).

But I don't see anything preventing just choosing a common mapping when performing the equality test: If you list X and get ["b","d","a","c"], mapping them in that order to 1,2,3,4, surely you can use the same mapping for the elements of Y. Then the elements of your two groups are mapped consistently: if "a" from X maps to 3, then also "a" from Y maps to 3.

This, of course, relies on the two domains having the same elements.

If someone wants to define some kind of "consistent" mapping from two arbitrarily different sets into small integers, that is going to be a very different kind of a project.

jukkakohonen commented 10 months ago

However, it may well be that python guarantees that list(set(X)) always produces the same result within one session, I don't know.

It does not guarantee that two sets that are equal would have equal lists in one session. This is consecutive commands on one session:

sage: X = set("aceh")
....: Y = set("heca")
....: X==Y
....: list(X)
....: list(Y)
....: list(X)==list(Y)
True
['c', 'e', 'a', 'h']
['e', 'c', 'a', 'h']
False

If two groups have domains that are equal (==), and one wants to convert them to GAP in a consistent manner, the point is not to hope that they happen to be listed in the same order (they won't). The point is to map the same elements to the same integers.

mantepse commented 10 months ago

@jukkakohonen, excellent, then it is clear that the mapping must be done in PermutationGroup_generic.__richcmp__. That is, essentially, compute the permutation Permutation([self._domain_to_gap[self._domain_from_gap[i]] for i in range(1, n+1)]) and conjugate by that. Do I understand correctly?

As the other experiment (printing the domain if it is not {1,...,n}) shows special domains are rarely used, mostly for the automorphism group of a graph. I am guessing that especially there we actually would like to take the domain into account.

jukkakohonen commented 10 months ago

@jukkakohonen, excellent, then it is clear that the mapping must be done in PermutationGroup_generic.__richcmp__. That is, essentially, compute the permutation Permutation([self._domain_to_gap[self._domain_from_gap[i]] for i in range(1, n+1)]) and conjugate by that. Do I understand correctly?

I'm not well familiar with the call interface between Sage and GAP, but something like that. Indeed it seems it must be in __richcmp__, because that is the place where we have access to the two groups being compared, so we can simply match the elements between the domains.

If each group creates their own GAP mapping (from domain elements to integers) separately, then it seems much more difficult to ensure that each group will choose the same mapping. If one of them maps "a" to 17, then how does the other know that -- if their domains are just equal (==), and not same (is)? This is possible if the domain has a way to list the elements in a canonical order.

Perhaps this code snippet explains what I'm after (or perhaps it was clear already):

sage: G = PermutationGroup([("a","c")], domain=set("aceh"))
sage: H = PermutationGroup([("a","c")], domain=set("heca"))
sage: list(G.domain())
['c', 'a', 'e', 'h']
sage: list(H.domain())
['c', 'h', 'a', 'e']
sage: g = G.gap()
sage: g
Group([ (1,2) ])
sage: G._domain_to_gap
{'c': 1, 'a': 2, 'e': 3, 'h': 4}

We see that the GAP mapping from G happens to map 'c' to 1. So, simply make a GAP mapping from H that maps 'c' to 1. In fact map all elements of H to integers using G._domain_to_gap.

Then we have the two GAP objects which should have consistently the same integers for the same elements both in G and H.

Not sure what is the best low-level technique of doing it. Do we need to create a new GAP object just for the purpose of the richcmp, and then throw it away? H might already have a GAP object but it could be using different numbering than the object in G.

jukkakohonen commented 10 months ago

One point I should mention: Only one of the two "directions" of confusion is really harmful with respect to what this issue was originally about, namely IntegerVectorsModPermutationGroup thinking that two groups are the same when they aren't.

If a mistake happens the other way, that two groups compare inequal while they "should" be equal (by whatever reasoning), the result is that IntegerVectorsModPermutationGroup simply does the calculations separately (creating a new strong generating system and whatever). It will work correctly, just a bit slower than if it could use the cached instance.

Thus, although for many other reasons it might be nice to have a permutation group equality machinery in Sage that is consistent by all accounts, it is not necessary for fixing this particular bug. For this bug we just need something that certainly differentiates the groups that are not "really" equal. Either by fixing group equality at least in that direction, or even more easily, make a local change in IntegerVectorsModPermutationGroup.

Seeing what a can of worms it is to "really" fix permutation group equality, a local change might be the best option for now. Then open a new issue for discussing the general notion.

mantepse commented 10 months ago

I don't think it is a good idea to postpone the fix for equality, if there is consensus. I don't see a can of worms, just an oversight.

jukkakohonen commented 10 months ago

I have no informed opinion on that (when to fix equality and by what protocol). Certainly it is something that I would like to have, but I trust others will have a better hunch on just how intrusive it is going to be. Also, there could be performance issues in creating a new GAP object within __richcmp__ just for the sake of one comparison? I don't have data on how expensive it is to create them.

In any case the equality fix deserves to be an issue with a more prominent name than "IntegerVectorsModPermutationGroup confuses group whose domains are different", no?

dimpase commented 10 months ago

at the moment (and already for a while, cf #26902) Sage has 2 interfaces to GAP

  1. old and slow expect interface (.gap, _gap_, etc)
  2. fast interface via libgap (.libgap, _libgap_, etc)

The plan is to move to libgap, largely done, but not quite. Old interface should be avoided. I think that PermutationGroup already uses libgap, so the overhead using it is minimal if any

dimpase commented 10 months ago

note that _domain_to_gap = {key: i+1 for i, key in enumerate(self._domain)} - i.e. there is no GAP calls here, it's just Python iteration

mantepse commented 10 months ago

@dimpase I don't see how constructing a gap group can be avoided if pi = Permutation([self._domain_to_gap[self._domain_from_gap[i]] for i in range(1, n+1)]) is not the identity. If it's not we have to conjugate the other.gapj() with pi, if I'm not mistaken.

NB: PermutationGroup_generic.conjugate ignores the domain (and only allows PermutationGroupElement, i.e., permutations with domain 1,...,n as input). This should also be fixed.

jukkakohonen commented 10 months ago

If the consensus is to fix the equality (to respect domains, and also other bug fixes like the GAP encoding), a couple of questions:

mantepse commented 10 months ago
* How exactly is "the same domain" defined? Do they have to be equal objects or is it enough that they contain the same elements? Could be different types `[1,2,3]` and `set((1,2,3))`, or ordered lists in different orders. I guess we could again look how `Graph`s did it.

Shouldn't the domain be a set? I see that currently it is a FiniteEnumeratedSet, but that looks like a mistake to me.

jukkakohonen commented 10 months ago

Shouldn't the domain be a set? I see that currently it is a FiniteEnumeratedSet, but that looks like a mistake to me.

Oh I see, whatever the user is giving is always converted to the same type. That makes sense, so we don't need to worry about different types of domain objects. Now FiniteEnumeratedSet is ordered, so one needs to decide (and document) whether e.g. symmetric groups on [1,2,3] and on [3,1,2] are the same group or not.

dimpase commented 10 months ago

IMHO the domain in permutation groups theory is a set, mathematically speaking.

Also, I think I explained above that we cannot fix equality without fixing inequalities, as we want $B\leq A\leq B$ to be equivalent to $A=B$.

dimpase commented 10 months ago

It seems that strict containment $A\lt B$ might be left alone, as inequality of domains here doesn't lead to contradictions. But this is confusing to the user.

jukkakohonen commented 10 months ago

I have now added a warning of this issue in the docstring of IntegerVectorsModPermutationGroup.

mantepse commented 10 months ago

IMHO the domain in permutation groups theory is a set, mathematically speaking.

I just realized: the reason for the domain being a FiniteEnumeratedSet is that we allow one-line-notation for group elements:

    Permutation groups can work on any domain. In the following
    examples, the permutations are specified in list notation,
    according to the order of the elements of the domain::

        sage: list(PermutationGroup([['b','c','a']], domain=['a','b','c']))
        [(), ('a','b','c'), ('a','c','b')]
        sage: list(PermutationGroup([['b','c','a']], domain=['b','c','a']))
        [()]
        sage: list(PermutationGroup([['b','c','a']], domain=['a','c','b']))
        [(), ('a','b')]