groups: implemented .subgroup() method #12658
Conversation
sympy/combinatorics/free_groups.py
Outdated
| all(isinstance(s, FreeGroupElement) for s in symbols): | ||
| return symbols | ||
| raise ValueError("") | ||
| raise ValueError("The type of `symbols` must be one of the following: "\ |
There was a problem hiding this comment.
I think, having \ at the end wouldn't be necessary (better not to have them).
sympy/combinatorics/free_groups.py
Outdated
| if isinstance(symbols, string_types): | ||
| return _symbols(symbols, seq=True) | ||
| elif isinstance(symbols, Expr): | ||
| elif isinstance(symbols, Expr) or isinstance(symbols, FreeGroupElement): |
There was a problem hiding this comment.
A single isinstance should suffice.
sympy/combinatorics/fp_groups.py
Outdated
| ''' | ||
| if not all([isinstance(g, FreeGroupElement) for g in gens]): | ||
| raise ValueError("Generators must be `FreeGroupElement`s") | ||
| letters = [g.array_form[0][0] for g in self.generators] |
There was a problem hiding this comment.
Is this the same as self.symbols?
There was a problem hiding this comment.
self is an FpGroup and it doesn't have .symbols. Though self.free_group.symbols is probably the same.
sympy/combinatorics/fp_groups.py
Outdated
| letters = self.free_group.symbols | ||
| for g in gens: | ||
| c = [t[0] in letters for t in g.array_form] | ||
| if not all(c): |
There was a problem hiding this comment.
Each FreeGroupElement has attribute group. Perhaps we should use the condition g.group == self.free_group.
There was a problem hiding this comment.
That would be good but that also makes me realise that we can't just make a new FreeGroup on already existing FreeGroupElements because then the generators end up with a wrong .group attribute. Also, the group attribute of the generators returned by reidemester_presentation() is a free group on more generators than necessary. Hmm, I'll have another look at it. Having reidemester_presentation() return generators with the right group attribute and matching relators would probably be better. Alternatively, go through the generators and relators changing their .group attribute when a new FreeGroup is created on them.
sympy/combinatorics/perm_groups.py
Outdated
| raise ValueError("The group doesn't contain the supplied generators") | ||
|
|
||
| G = PermutationGroup(gens) | ||
| G._parent = self |
There was a problem hiding this comment.
For what purpose is this defined?
There was a problem hiding this comment.
I suppose it's not as relevant to permutation groups. For FpGroups it allows to have is_subgroup method, and also this is a set-up GAP has (i.e. having .parent attribute for subgroups of groups). Theoretically having this might save time when someone tries to call is_subgroup on large permutation groups but mainly I was just mimicking what I did for FpGroups here. Should I remove it?
| assert str(p1) == "((y_1, y_2), (y_1**2, y_2**3, y_2*y_1*y_2*y_1*y_2*y_1))" | ||
|
|
||
| H = f.subgroup(H) | ||
| assert (H.generators == p1[0]) and (H.relators == p1[1]) |
There was a problem hiding this comment.
This could probably have (H.generators, H.relators) == p1.
sympy/combinatorics/fp_groups.py
Outdated
|
|
||
| @property | ||
| def relators(self): | ||
| return tuple(self._relators) |
There was a problem hiding this comment.
Would there be any reason why we couldn't have self.relators instead of self._relators as a plain attribute?
There was a problem hiding this comment.
I don't think so. And I don't think there is any reason to have self._free_group as well as self.free_group. Should I change both of them?
There was a problem hiding this comment.
I think you could do that. For relators in particular, that seems better than defining a property. But it would probably be preferable to do the changes in a separate PR.
| simplify_presentation(C) | ||
|
|
||
| syms = [g.array_form[0][0] for g in C._schreier_generators] | ||
| g = free_group(syms)[0] |
There was a problem hiding this comment.
How is this related to C._schreier_free_group?
There was a problem hiding this comment.
It's a free group on generators that have the same symbols as a subset of C._schreier_free_group's generators.
3 participants
This PR implements a method
.subgroup()for groups (both permutation andFpGroups) which takes the subgroup generators as the argument and returns another group whoseparentis set to be the original group.PermutationGroups: the method simply creates anotherPermutationGroupon the given generators. The use of.parentin the.is_subgroup()method allows for faster subgroup checking in some cases.FpGroups:reidemeister_presentation()method returns the generators and relators for a presentation of the subgroup. These are instances ofFreeGroupElement. To return anFpGroupbased on them, it is necessary to create aFreeGroupon the generators first and make sure that the generators of this free group match up with those used in the relators. For this purpose I made it possible to create aFreeGroupby passing instances ofFreeGroupElements to it.The other changes are relatively minor and not directly relevant to the new method:
Generators of
FpGroupare returned by the property.generatorswhile a method.relators()has to be called for the relators. It feels a bit inconsistent so I made.relatorsa property.Rearranged things in the file
fp_groups.pyto have properties all together near the top of the class definition.Corrected a couple of typos, added a missing error message and slightly changed the description for the arguments of the
FreeGroupclass.