Fix FinitelyPresentedGroupElement.__hash__

[user202729]

Replacement for #40563. Fix #40549

Note that FpElementNFFunction is what GAP uses internally for ==

#############################################################################
##
#M  \=( <elm1>, <elm2> )  . . . . . . . . .  for two elements of a f.p. group
##
InstallMethod( \=, "for two f.p. group elements", IsIdenticalObj,
    [ IsElementOfFpGroup, IsElementOfFpGroup ],0,
# this is the only method that may ever be called!
function( left, right )
  if UnderlyingElement(left)=UnderlyingElement(right) then
    return true;
  fi;
  return FpElmEqualityMethod(FamilyObj(left))(left,right);
end );


InstallMethod( FpElmEqualityMethod, "generic dispatcher",
true,[IsElementOfFpGroupFamily],0,MakeFpGroupCompMethod(\=));


BindGlobal( "MakeFpGroupCompMethod", function(CMP)
  return function(fam)
    local hom,f,com;
    # if a normal form method is known, and it is not known to be crummy
    if HasFpElementNFFunction(fam) and not IsBound(fam!.hascrudeFPENFF) then
      f:=FpElementNFFunction(fam);
      com:=x->f(UnderlyingElement(x));
    # if we know a faithful representation, use it
    elif HasFPFaithHom(fam) and
     FPFaithHom(fam)<>fail then
      hom:=FPFaithHom(fam);
      com:=x->Image(hom,x);
	  ...

Sometimes == works better than confluent_rewriting_system. (Although I don't dare to include this as a test, otherwise upgrade to GAP that selects a different algorithm may randomly timeout the test)

sage: F.<a,b,c,d,e> = FreeGroup()
sage: rules = [e*d*c^-1*d*a, b^-1*a, a*e*b*d^-1*c]
sage: G = F / rules
sage: G(a) == G(1)
False
sage: G.confluent_rewriting_system()  # takes forever

Sometimes not. But gap appears to use some sort of caching system, so it might work anyway.

sage: F.<a,b,c> = FreeGroup()
sage: rules = [a^-1*b*c*a*b, a*c^-1*a*c^-1*a, a^2*c]
sage: G = F / rules
sage: G(a) == G(1)	# takes forever

When one compute one confluent_rewriting_system and one hash before the ==, it finishes quickly.

sage: F.<a,b,c> = FreeGroup()
sage: rules = [a^-1*b*c*a*b, a*c^-1*a*c^-1*a, a^2*c]
sage: G = F / rules
sage: ignore = G.confluent_rewriting_system()
sage: G(a)._normal_form_by_gap_nffunction()
a
sage: G(a) == G(1)
False

📝 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 and checked the documentation preview.

⌛ Dependencies

[cxzhong]

It still cause timeout issues

src/bin/sage -t --warn-long 5.0 --random-seed=113808444557570614600856870960997183849 src/doc/en/thematic_tutorials/group_theory.rst  # Timed out
src/bin/sage -t --warn-long 5.0 --random-seed=113808444557570614600856870960997183849 src/sage/combinat/designs/incidence_structures.py  # Timed out
src/bin/sage -t --warn-long 5.0 --random-seed=113808444557570614600856870960997183849 src/sage/graphs/bliss.pyx  # Timed out
src/bin/sage -t --warn-long 5.0 --random-seed=113808444557570614600856870960997183849 src/sage/graphs/generic_graph.py  # Timed out
src/bin/sage -t --warn-long 5.0 --random-seed=113808444557570614600856870960997183849 src/sage/groups/finitely_presented_named.py  # Timed out
src/bin/sage -t --warn-long 5.0 --random-seed=113808444557570614600856870960997183849 src/sage/groups/libgap_wrapper.pyx  # Timed out
src/bin/sage -t --warn-long 5.0 --random-seed=113808444557570614600856870960997183849 src/sage/groups/perm_gps/permgroup.py  # Timed out
src/bin/sage -t --warn-long 5.0 --random-seed=113808444557570614600856870960997183849 src/sage/groups/perm_gps/permgroup_morphism.py  # Timed out
src/bin/sage -t --warn-long 5.0 --random-seed=113808444557570614600856870960997183849 src/sage/topology/simplicial_complex_examples.py  # Timed out
src/bin/sage -t --warn-long 5.0 --random-seed=113808444557570614600856870960997183849 src/sage/topology/simplicial_complex.py  # Timed out

[user202729]

#39244

[cxzhong]

Maybe the serve is down. We may skip the tests?

[cxzhong]

The ouput is correct

sage: F.<x,y> = FreeGroup()
sage: G = F / [x^4, y^13, x*y*x^-1*y^-5]
sage: a, b = G.gens()
sage: assert(G.order() == 52)
sage: assert(a.order() == 4)
sage: assert(b.order() == 13)
sage: assert(a*b*a^-1 == b^5)
sage: gr = G.cayley_graph(generators=[a,b]).to_undirected()
sage: print(gr.num_verts())
52

[tscrim]

Why is this a hidden method? It makes more sense to me for this to be public.

[user202729]

Do we guarantee the method is implemented by GAP?
Is NFFunction even publicly documented API in GAP, if so, what is its behavior?
Better be safe.

[tscrim]

You should add some actual examples.

[user202729]

same comment as above. It seems dangerous to promise behavior of this while the underlying gap function is undocumented.

[tscrim]

Suggested change

	Prefer to use ``libgap(k)`` for consistency.
	The returned object should not be mutated, otherwise with the current implementation,
	``self`` will be mutated as well.

I don't see the point of this; please remove.

[user202729]

remove what?

• the recommendation to use libgap()?
• the _libgap_ hook?
• the whole method?

it's true that the external user interface is really meant to be libgap(...) though (instead of x._libgap_() or x.gap(), note that the last one doesn't work on almost all types, while libgap(...) works on almost all types. See also #39905 #39909)

[tscrim]

Please explain this change. It seems like it is making the error message more understandable.

[user202729]

MakeConfluent is a GAP-wrapped function and can only ever raise GAPError or (if the user interrupt it) KeyboardInterrupt or AlarmInterrupt. ValueError is never raised.

[tscrim]

It will be very surprising to have elements in the free group instead of the FPG. This change needs much more justification, both here and in the documentation if deemed reasonable.

[user202729]

all_edges is a local variable, it's not visible outside. What matters is the simplicial set structure.

edit: thinking about it again maybe the result are visible somewhere in the simplicial set structure (need double check), but either way, what matters is the simplicial set structure.

[dimpase]

I really don't understand why the problem that was meant to be addressed by this PR is not fixed properly, by converting the group into a permutation group to begin with, as I explained on the predecessor to this PR.

Instead there is some fishy code calling undocumented GAP functions. Which might break, change behaviour, etc.

[dimpase]

Please address the original problem, and do not use undocumented 3rd party code.

[user202729]

@dimpase the surface level problem is Cayley graph construction is incorrect. The underlying problem is __hash__ of finitely presented group element is incorrect. Fixing the latter also fixes the former. And I can't find an good way to get a normalized form of a GAP group element.

What do you propose to do with __hash__, raise NotImplementedError?

[dimpase]

Construction of the Cayley graph of a permutation group is trivial. I really don't see why anyone would want to mess around with words.

[dimpase]

computing the canonical form of an element in an f.p. group is in general algorithmically unsolvable, and is solvable, but still rather tricky, for finite groups, and some classes of infinite groups. I don't mind if hash returns not implemented for f.p. groups.

[user202729]

The current situation is:

• Someone report in Cayley graphs of free groups have wrong number of vertices. #40549 that the function to compute Cayley graph is incorrect for certain finitely-presented groups.
• Investigation reveals that the problem is caused by elements of type FinitelyPresentedGroupElement violate Python's invariant: if x == y, then hash(x) == hash(y), which causes the issue. Fixing this problem would fix the original issue with no further change.
• Internally, GAP implements equality comparison of finitely presented group element by computing a normal form for each element, using the internal function FpElementNFFunction, and compare these.

The proposals are:

1. I propose to fix the problem by using FpElementNFFunction to implement __hash__.

   The advantage of this is that for group elements whose equality can be decided by GAP's default algorithm, __hash__ should works as well.

   The most common use case of __hash__ is dict/set, and in these cases equality need to be decided anyway, which means __hash__ works "as often as it could be".

2. @dimpase says calling FpElementNFFunction is "fishy", and propose to make __hash__ raise NotImplementedError, and in functions that previously used __hash__, rewrite them to use something else.

   The advantage of this is not using undocumented functions.

Am I summarizing the situation correctly? @dimpase

[dimpase]

I find it rather unconvincing that the undocumented GAP function does the job you claim it does. Looking at its description in the source code, and the fact it's merely a helper in the documented GAP function "47.3-4 SetReducedMultiplication", it seems it's just a heuristic to speed up multiplication of words in an f.p. group.

[dimpase]

I am speaking on this topic not as an amateur in the area, but as someone with a PhD in group theory, as someone who published ~20 papers where computations with f.p. groups played a significant role.

And the main message I am trying to get through is that if you want to do anything with an f.p. group which is not of a very special type, your best and the only bet is to convert it to a group where you can efficiently test words in generators for equality: a permutation group, a matrix group, etc.

It's meaningless to talk about hash properties for elements x,y of an arbitrary f.p. group, as the question of whether x==y is algorithmically unsolvable.