Save and load finitely presented groups coming from libgap groups

[enriqueartal]

At this point it is not possible to load a saved finitely presented group that comes from a libgap group, see an example in #37061

One possible cause is the use of a general __reduce__ method for free groups. At least, adding such a method allows to load free groups or finitely presented groups obtained from a libgap group using wrapFreeGroup or wrapFpGroup. It fixes #37061 and it would simplify some code in #36768

With these changes, free and finitely presented groups, included libgap groups, can be pickled.

📝 Checklist

• The title is concise, informative, and self-explanatory.
• 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.

[tscrim]

A bit more detail about what is going wrong here. The problem is that when using the wrap function for a free group, it already knows what its libgap group is, so it passes that along as part of its construction data. This means it is becomes part of its pickling data as it is part of its input (which defines itself as a unique representation). This also causes two free groups to be constructed on the same generators that are not equal (as they are not identical).

IMO, we should remove having the (lib)GAP group as an input option for free groups. Likewise, I think we should remove the wrap_FreeGroup and wrap_FpGroup functions. The former is only called by the latter, which is only called in one place (schemes/curves/zariski_vankampen.py) that doesn't even return anything related to the constructed object! Everything is being done in GAP, so just convert the data from GAP over. Not only will this solve the issue at hand, it will prevent other subtleties from happening if this function gets used elsewhere.

[enriqueartal]

A bit more detail about what is going wrong here. The problem is that when using the wrap function for a free group, it already knows what its libgap group is, so it passes that along as part of its construction data. This means it is becomes part of its pickling data as it is part of its input (which defines itself as a unique representation). This also causes two free groups to be constructed on the same generators that are not equal (as they are not identical).

IMO, we should remove having the (lib)GAP group as an input option for free groups.

I agree with this, may the opinion of @miguelmarco who created it would be interesting.

Likewise, I think we should remove the wrap_FreeGroup and wrap_FpGroup functions. The former is only called by the latter, which is only called in one place (schemes/curves/zariski_vankampen.py) that doesn't even return anything related to the constructed object! Everything is being done in GAP, so just convert the data from GAP over. Not only will this solve the issue at hand, it will prevent other subtleties from happening if this function gets used elsewhere.

I am not sure this is a good idea to eliminate wrap_FpGroup, it appears also in other functions in groups/finitely_presented.py and obtaining presentation for new groups is better down by gap and something that converts a finitely presented group in libgap into such a group in sage is necessary. One case that may appear, give the presentation of the kernel of a homomorphism into a finite group. Of course, may be another code for this function may also help.

[miguelmarco]

IIRC, my idea when I wrote that code was that the corresponding sage classes would be wrappers around gap groups, so it would make sense to have a way to wrap an already existing gap group.

It can be useful if some user constructs a group using some group using lower level gap functionalities (not directly exposed by sage), and then wants to handle it as a sage group.

For example, let's say we want to compute the lower central series of a group. Sage does not have a function for that, but gap does. So we can do:

sage: from sage.groups.finitely_presented import wrap_FpGroup
sage: G = groups.presentation.Symmetric(4)
sage: LCS = G._libgap_().LowerCentralSeriesOfGroup()
sage: LCS0 = LCS[0]
sage: LCS0
Group([ a, b ])
sage: LCS0P = LCS0.IsomorphismFpGroup().Image()
sage: LCS0P
<fp group on the generators [ F1, F2 ]>
sage: wrap_FpGroup(LCS0P)
Finitely presented group < F1, F2 | F2^2, F1^4, (F2*F1)^3 >

That is, because we can create sage groups directly from gap groups, we have a lower level interface to get the underlying gap group, work in it within gap to obtain a new gap FpGroup, and then wrap it to get a sage group. If we drop the option of wrapping them directly, we loose this functionality.

It wouldn't be a problem if we had methods that expose all (or at least, a vast majority) ways that you can construct finitely presented groups in gap... but there are a lot of them (this lower central series case is just an example). It would be nice to add them, but we are far from there now.

So until we have an alternative for this kind of constructions, I would be against dropping this possibility.

[enriqueartal]

Basically I had the same kind of examples, e.g, subgroups of finite index of a f.p. group, using also IsomorphismFpGroup().Range(). Anyway, I have only used wrap_FpGroup, never wrap_FreeGroup directly.

For the results of the change of code, I do not know why _reduction and __dict__ change so much. With develop:

sage: F = FreeGroup(2)
sage: F._reduction
(<class 'sage.groups.free_group.FreeGroup_class'>, (('x0', 'x1'),), {})
sage: F.__reduce__()
(<function unreduce at 0x7fb8c53c1800>,
 (<class 'sage.groups.free_group.FreeGroup_class'>, (('x0', 'x1'),), {}))
sage: F.__dict__
{'_libgap': <free group on the generators [ x0, x1 ]>,
 '_ambient': None,
 '_reduction': (<class 'sage.groups.free_group.FreeGroup_class'>,
  (('x0', 'x1'),),
  {})}

With this branch:

sage: F = FreeGroup(2)
sage: F._reduction
(<function dynamic_class at 0x7fea1562dbc0>,
 ('FreeGroup_class_with_category',
  (<class 'sage.groups.free_group.FreeGroup_class'>,
   <class 'sage.categories.category.JoinCategory.parent_class'>),
  None,
  None,
  <class 'sage.groups.free_group.FreeGroup_class'>))
sage: F.__reduce__()
(<function unreduce at 0x7fea1562e020>,
 (<class 'sage.groups.free_group.FreeGroup_class'>, (('x0', 'x1'),), {}))
sage: F.__dict__
{'_libgap': <free group on the generators [ x0, x1 ]>, '_ambient': None}

[tscrim]

We should not have as part of the input data of a free group the libgap group. It creates needless complexity with breaking the unique representation. However, we can add a __classcall_private__ that takes the libgap group input, extracts the necessary data, and sets the _libgap to the passed in group after the object is created. Although I don't think there is any benefit for free groups to this and it could subtly modify the unique object in (Sage's) memory; GAP doesn't do this uniqueness.

The _reduction and __dict__ change as part of the UniqueRepresentation as I mentioned above.

For FP groups, this could be more useful. Although I don't know how sensitive GAP is to using different (but really equivalent) free groups for the FP groups. However, the methods wrap_* is not a good approach; it should be handled by the class's construction (either __init__ or more likely __classcall_private__). However, we need to take care of the uniqueness (possibly downgrading to CachedRepresentation, but that feels like a step backwards to me). We might need to think carefully about how to do this if we really want to support this functionality.

[enriqueartal]

I made a couple of commits:

• __init__ of FreeGroup_class does not have anymore the keyword libgap_free_group; the reconstruction of the libgap free group is straightforward.
• wrap_FreeGroup is also gone
• __reduce__ of FreeGroup_class is gone, the former issues were caused by libgap_free_group
• The function wrap_FpGroup has been accordingly changed. Now G and wrap_FreeGroup(libgap(G)) are equal, and save-load works. I only see one option to eliminate this function: to create a sage method for libgap finitely presented groups, if it is possible. There are many places where the only option to create some f.p. groups is passing through libgap.
  If there is a better solution, please make a concrete one.

[miguelmarco]

We should not have as part of the input data of a free group the libgap group. It creates needless complexity with breaking the unique representation. However, we can add a __classcall_private__ that takes the libgap group input, extracts the necessary data, and sets the _libgap to the passed in group after the object is created. Although I don't think there is any benefit for free groups to this and it could subtly modify the unique object in (Sage's) memory; GAP doesn't do this uniqueness.

That sounds interesting, but I didn't know about the ' classcall_private` method until now. How would be the interface in this approach?

[enriqueartal]

While there are still some issues, with the last changes, there is no more wrap_FpGroup, its functionality is done with a sage method. After discussions with @miguelmarco, I think there is at least one issue (which basically exists in the develop branch): if a sage f.p. group is created from a libgap one, another libgap group is created in the process. The same happens in this version for free groups, but I think this is not an issue, since a gap free group does not contain a lot of extra information, while the original libgap f.p. group may have information which will be lost, and the gestion of the memory is not optimal.
I do not know if there are solutions for these issues starting from here, or a new approach is needed. Anyway, the original problem does not happen any more.

[enriqueartal]

Should the class GroupMorphismWithGensImages be based on hom in sage/structure/parent.pyx? The information is basically the same but much more methods are available, e.g., kernel from groups/libgap_morphism.py.

[enriqueartal]

I tried some tricks for homomorphisms and it seems that it is related to the problem pointed out here by @tscrim, for G a libgap group, G1 = G.sage() and G2 = G1.gap(), we do not have equality between G and G2. I have no idea how to do it.

[enriqueartal]

@tscrim can you compare the change of ##38097 with the one in this PR for CachedRepresentation?

[enriqueartal]

This PR has been silent for a while. The only big issue was the failing test for something related with sage<4.0 and I eliminated the test. I wonder if it is worth to try to get to 9.4, and if not, which changes are needed.

[tscrim]

Sorry, I haven't had much time to get to this until now.

I am still really bothered by the change breaking the Cartan type unpickling. I really do not understand why that doesn't work. Hence, I am very worried we could unknowingly be breaking something that isn't tested otherwise. It's probably okay since it is for very old pickles. I just wish I knew what was breaking there...

Maybe for better transparency, we should split the UniqueRepresentation change off as a separate PR. Would you be okay doing this? Sorry for the extra work.

[enriqueartal]

I am going to prepare a branch with the change in the class. Nevertheless, as far as some more experts take a look it will be difficult to test if some problems appear, they do not in the tests.

[tscrim]

I am going to prepare a branch with the change in the class. Nevertheless, as far as some more experts take a look it will be difficult to test if some problems appear, they do not in the tests.

Thank you. Well, there is one test that is failing, but we are removing it. So there is at least something that is breaking in a subtle way. At least with a separate PR, there is a clear point where people can see and comment. We can also clearly explain the reasoning (I can add this into the PR if necessary).