New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implemented Group Homomorphisms #12827

Merged
merged 13 commits into from Jul 17, 2017

Conversation

Projects
None yet
2 participants
@valglad
Contributor

valglad commented Jun 28, 2017

This PR implements group homomorphisms from FpGroups to PermutationGroups (with plans to extend it too all group combinations in the future).

GroupHomomorphism is a new class to be instantiated with the function homomorphism(domain, codomain, gens, images) which creates a homomorphism from domain to codomain defined by sending the elements of gens (generators of domain) to the elements of images (elements of codomain). The unspecified generators are mapped to the identity and if no images are given, a trivial homomorphism is created. If the definition doesn't extend to a homomorphism on the whole of domain, an exception is raised.

A new class FpSubgroup in sympy.combinatorics.fp_groups is implemented for handling subgroups of FpGroups in a way that would allow the use of words in the same generators as the original group. This is mainly for testing if an element of the parent group is contained in the subgroup and also the order can be calculated. The method to_FpGroup() returns an isomorphic FpGroup on different generators (same as would be returned by FpGroup's subgroup() method).

Also there is a new method generator_product() for permutation groups which, given an element g of the group, returns a list of strong generators [s_1 ... s_n] s.t. g = s_1*...*s_n. This is possible by keeping track of the generators making up the elements of basic_transversals and storing their indices in a new attribute _transversal_slp.

Show outdated Hide outdated sympy/combinatorics/grouphomomorphism.py
'''
image = self.image()
vals = [v for v in self.images.values() if not v.is_identity]
keys = self.images.keys()

This comment has been minimized.

@jksuom

jksuom Jun 29, 2017

Member

This is not a list in Python 3 but a 'view'. It should probably be explicitly converted to a list.

@jksuom

jksuom Jun 29, 2017

Member

This is not a list in Python 3 but a 'view'. It should probably be explicitly converted to a list.

Show outdated Hide outdated sympy/combinatorics/fp_groups.py
def to_FpGroup(self):
return self.parent.subgroup(C=self.C)

This comment has been minimized.

@jksuom

jksuom Jun 29, 2017

Member

Apparently there is trailing whitespace. (It is useful to configure the editor to show those. It is easy to forget git diff --check or bin/test quality.)

@jksuom

jksuom Jun 29, 2017

Member

Apparently there is trailing whitespace. (It is useful to configure the editor to show those. It is easy to forget git diff --check or bin/test quality.)

Show outdated Hide outdated sympy/combinatorics/fp_groups.py
def __contains__(self, g):
if not g in self.parent:
raise ValueError

This comment has been minimized.

@jksuom

jksuom Jun 30, 2017

Member

Is this better than returning False? (BTW, Python prefers the form g not in. More readable, I guess.)

@jksuom

jksuom Jun 30, 2017

Member

Is this better than returning False? (BTW, Python prefers the form g not in. More readable, I guess.)

Show outdated Hide outdated sympy/combinatorics/fp_groups.py
if not g in self.parent:
raise ValueError
gens = g.contains_generators()
if any([g not in self.generators for g in gens]):

This comment has been minimized.

@jksuom

jksuom Jun 30, 2017

Member

Is it safe to use g here?

@jksuom

jksuom Jun 30, 2017

Member

Is it safe to use g here?

This comment has been minimized.

@jksuom

jksuom Jun 30, 2017

Member

I assume the elements of self.generators are words, in general. So there need not be any generators of the parent group.

@jksuom

jksuom Jun 30, 2017

Member

I assume the elements of self.generators are words, in general. So there need not be any generators of the parent group.

This comment has been minimized.

@valglad

valglad Jun 30, 2017

Contributor

No, it isn't! The original g would be rewritten in this case, and I guess didn't notice only because I tried this on subgroups generated only by one element. I'll correct this.

@valglad

valglad Jun 30, 2017

Contributor

No, it isn't! The original g would be rewritten in this case, and I guess didn't notice only because I tried this on subgroups generated only by one element. I'll correct this.

This comment has been minimized.

@valglad

valglad Jun 30, 2017

Contributor

So there need not be any generators of the parent group.

True, I thought this when I was writing it and decided to come back later to change this to a set of parent generators involved in the words of self.generators, but I never did come back. Looking at it now, I'm not sure this check would do much for performance anyway so, maybe, it's not necessary at all.

@valglad

valglad Jun 30, 2017

Contributor

So there need not be any generators of the parent group.

True, I thought this when I was writing it and decided to come back later to change this to a set of parent generators involved in the words of self.generators, but I never did come back. Looking at it now, I'm not sure this check would do much for performance anyway so, maybe, it's not necessary at all.

Show outdated Hide outdated sympy/combinatorics/grouphomomorphism.py
'''
if isinstance(domain, PermutationGroup):
raise NotImplementedError("Homomorphisms from permutation groups are not currently implemented")
elif not isinstance(domain, FpGroup):

This comment has been minimized.

@jksuom

jksuom Jul 1, 2017

Member

Should FreeGroup be included?

@jksuom

jksuom Jul 1, 2017

Member

Should FreeGroup be included?

Show outdated Hide outdated sympy/combinatorics/grouphomomorphism.py
raise NotImplementedError("Homomorphisms from permutation groups are not currently implemented")
elif not isinstance(domain, FpGroup):
raise TypeError("The domain must be a group")
if not(isinstance(codomain, PermutationGroup) or isinstance(codomain, FpGroup)):

This comment has been minimized.

@jksuom

jksuom Jul 1, 2017

Member

A single isinstance with classinfo tuple will suffice.

@jksuom

jksuom Jul 1, 2017

Member

A single isinstance with classinfo tuple will suffice.

@jksuom

This comment has been minimized.

Show comment
Hide comment
@jksuom

jksuom Jul 1, 2017

Member

The file name could also be just homomorphisms.py since it will probably never be in the same directory with module homomorphisms. (I guess that can be handled with git mv.)

Member

jksuom commented Jul 1, 2017

The file name could also be just homomorphisms.py since it will probably never be in the same directory with module homomorphisms. (I guess that can be handled with git mv.)

Show outdated Hide outdated sympy/combinatorics/grouphomomorphism.py
if r.is_identity:
return identity
elif len(r) == 1:
return images[r]

This comment has been minimized.

@jksuom

jksuom Jul 1, 2017

Member

Can we assume that r is never the inverse of a single generator?

@jksuom

jksuom Jul 1, 2017

Member

Can we assume that r is never the inverse of a single generator?

This comment has been minimized.

@valglad

valglad Jul 1, 2017

Contributor

No, I should check if it is and do images[r**-1].

@valglad

valglad Jul 1, 2017

Contributor

No, I should check if it is and do images[r**-1].

Show outdated Hide outdated sympy/combinatorics/grouphomomorphism.py
return images[r]
else:
power = r.array_form[0][1]
return images[r[0]]**power*_image(r.subword(abs(power),len(r)))

This comment has been minimized.

@jksuom

jksuom Jul 1, 2017

Member

This recursion should probably be eventually replaced by repetition since Python's execution stack size is limited.

@jksuom

jksuom Jul 1, 2017

Member

This recursion should probably be eventually replaced by repetition since Python's execution stack size is limited.

Show outdated Hide outdated sympy/combinatorics/grouphomomorphism.py
def homomorphism(domain, codomain, gens, images=[]):
'''
Create (if possible) a group homomorphism from the group `domain` to the group

This comment has been minimized.

@jksuom

jksuom Jul 10, 2017

Member

These lines are too long for a 80 character display.

@jksuom

jksuom Jul 10, 2017

Member

These lines are too long for a 80 character display.

Show outdated Hide outdated sympy/combinatorics/grouphomomorphism.py
keys = list(self.images.keys())
inverses = {}
for i, v in enumerate(vals):
inverses[v] = keys[i]

This comment has been minimized.

@jksuom

jksuom Jul 13, 2017

Member

It looks like this could be wrong, if vals is shorter than keys. keys[i] could be mapped to identity.

@jksuom

jksuom Jul 13, 2017

Member

It looks like this could be wrong, if vals is shorter than keys. keys[i] could be mapped to identity.

@@ -246,7 +246,7 @@ def _handle_precomputed_bsgs(base, strong_gens, transversals=None,
def _orbits_transversals_from_bsgs(base, strong_gens_distr,
transversals_only=False):
transversals_only=False, slp=False):

This comment has been minimized.

@jksuom

jksuom Jul 13, 2017

Member

slp should be documented.

@jksuom

jksuom Jul 13, 2017

Member

slp should be documented.

Show outdated Hide outdated sympy/combinatorics/perm_groups.py
If `slp` is `True`, a dictionary `{beta: slp_beta}` is returned
for `\beta \in Orb` where `slp_beta` is a list of indices of the
generators in `generators` s.t. if `slp = [i_1 ... i_n]`
`g_\beta = generators[i_1]*...*generators[i_n]`.

This comment has been minimized.

@jksuom

jksuom Jul 13, 2017

Member

The order should be checked. In the* product, the leftmost permutation is applied first.

@jksuom

jksuom Jul 13, 2017

Member

The order should be checked. In the* product, the leftmost permutation is applied first.

This comment has been minimized.

@valglad

valglad Jul 13, 2017

Contributor

True, should be the other way around.

@valglad

valglad Jul 13, 2017

Contributor

True, should be the other way around.

Show outdated Hide outdated sympy/combinatorics/fp_groups.py
self.C = None
def __contains__(self, g):
gens = [s for s in g.contains_generators()]

This comment has been minimized.

@jksuom

jksuom Jul 14, 2017

Member

Could this be list(g.contains_generators())?

@jksuom

jksuom Jul 14, 2017

Member

Could this be list(g.contains_generators())?

This comment has been minimized.

@jksuom

jksuom Jul 14, 2017

Member

Or perhaps simply g.contains_generators()?

@jksuom

jksuom Jul 14, 2017

Member

Or perhaps simply g.contains_generators()?

Show outdated Hide outdated sympy/combinatorics/fp_groups.py
def __contains__(self, g):
gens = [s for s in g.contains_generators()]
if any([s not in self.generators for s in gens]):

This comment has been minimized.

@jksuom

jksuom Jul 14, 2017

Member

This looks like a strong condition. Is it necessary that each one of the generators appearing in g should be among the subgroup generators?

@jksuom

jksuom Jul 14, 2017

Member

This looks like a strong condition. Is it necessary that each one of the generators appearing in g should be among the subgroup generators?

Show outdated Hide outdated sympy/combinatorics/fp_groups.py
if any([s not in self.generators for s in gens]):
return False
elif isinstance(self.parent, FreeGroup):
return True

This comment has been minimized.

@jksuom

jksuom Jul 14, 2017

Member

This doesn't look right. The subgroup could be a proper subgroup of a free group.

@jksuom

jksuom Jul 14, 2017

Member

This doesn't look right. The subgroup could be a proper subgroup of a free group.

This comment has been minimized.

@valglad

valglad Jul 14, 2017

Contributor

self.generators contains the generators of the subgroup. So that, say, if the parent group is a free group F on a, b and we create an FpSubgroup(F, [a]), self.generators == [a]. Then the first if checks if all of the letters of the word are in the subgroup alphabet, and if they are, then it belongs to the subgroup.

Though I've just realised that we've discussed this before and that there is a problem with it because the subgroup generators could be arbitrary words, like a*b. Sorry about that. Will need to think more carefully about how to check the membership for FreeGroup subgroups.

@valglad

valglad Jul 14, 2017

Contributor

self.generators contains the generators of the subgroup. So that, say, if the parent group is a free group F on a, b and we create an FpSubgroup(F, [a]), self.generators == [a]. Then the first if checks if all of the letters of the word are in the subgroup alphabet, and if they are, then it belongs to the subgroup.

Though I've just realised that we've discussed this before and that there is a problem with it because the subgroup generators could be arbitrary words, like a*b. Sorry about that. Will need to think more carefully about how to check the membership for FreeGroup subgroups.

Show outdated Hide outdated sympy/combinatorics/fp_groups.py
def order(self):
# This is valid because `len(self.C.table)` (the index of the subgroup)
# will always be finite - otherwise coset enumeration doesn't terminate

This comment has been minimized.

@jksuom

jksuom Jul 14, 2017

Member

The index of a subgroup need not be finite, but a subgroup of a free group is also a free group, hence infinite or trivial.

@jksuom

jksuom Jul 14, 2017

Member

The index of a subgroup need not be finite, but a subgroup of a free group is also a free group, hence infinite or trivial.

This comment has been minimized.

@valglad

valglad Jul 14, 2017

Contributor

Oh, that comment was written a while ago, before FreeGroups were added and at the time when I thought the coset table should be set at construction. I'll change it.

@valglad

valglad Jul 14, 2017

Contributor

Oh, that comment was written a while ago, before FreeGroups were added and at the time when I thought the coset table should be set at construction. I'll change it.

Show outdated Hide outdated sympy/combinatorics/free_groups.py
@@ -150,6 +150,7 @@ def __new__(cls, symbols):
obj.dtype = type("FreeGroupElement", (FreeGroupElement,), {"group": obj})
obj.symbols = symbols
obj.generators = obj._generators()
obj.relators = tuple()

This comment has been minimized.

@jksuom

jksuom Jul 14, 2017

Member

This could rather be a class attribute, a common property of all free groups instead of an instance attribute in every groups dictionary.

@jksuom

jksuom Jul 14, 2017

Member

This could rather be a class attribute, a common property of all free groups instead of an instance attribute in every groups dictionary.

Show outdated Hide outdated sympy/combinatorics/homomorphisms.py
w = identity
i = 0
while i < len(r):
power = r.array_form[i][1]

This comment has been minimized.

@jksuom

jksuom Jul 14, 2017

Member

Both forms of r are used alternately in the loop, array form on this line and letter form for r[i] on the next line. It might be possible to avoid the repeated transformations by using a loop like this: for gen, pow in r: with r in array form.

@jksuom

jksuom Jul 14, 2017

Member

Both forms of r are used alternately in the loop, array form on this line and letter form for r[i] on the next line. It might be possible to avoid the repeated transformations by using a loop like this: for gen, pow in r: with r in array form.

This comment has been minimized.

@valglad

valglad Jul 15, 2017

Contributor

gen would be a symbol, not a FreeGroupElement in such a loop. Something like this could work if I do:

r_arr = r.array_form
for sym, power in r_arr:
    gen = r.group.dtype(sym)

Would that be better than what it currently is?

@valglad

valglad Jul 15, 2017

Contributor

gen would be a symbol, not a FreeGroupElement in such a loop. Something like this could work if I do:

r_arr = r.array_form
for sym, power in r_arr:
    gen = r.group.dtype(sym)

Would that be better than what it currently is?

This comment has been minimized.

@jksuom

jksuom Jul 15, 2017

Member

Probably not. I forgot that symbols are not keys of images. But r_arr could be a local variable and used on this line instead of r.array_form.

@jksuom

jksuom Jul 15, 2017

Member

Probably not. I forgot that symbols are not keys of images. But r_arr could be a local variable and used on this line instead of r.array_form.

Show outdated Hide outdated sympy/combinatorics/homomorphisms.py
gens = []
K = FpSubgroup(G, gens)
i = self.image().order()
while K.order()*i != G.order():

This comment has been minimized.

@jksuom

jksuom Jul 14, 2017

Member

G.order() could also be saved in a local variable.

@jksuom

jksuom Jul 14, 2017

Member

G.order() could also be saved in a local variable.

Show outdated Hide outdated sympy/combinatorics/fp_groups.py
if w2**-1 == w1:
continue
if w2[len(w2)-1]**-1 == w1[0] and w2*w1 not in gens:
gens.append(w2*w1)

This comment has been minimized.

@jksuom

jksuom Jul 16, 2017

Member

Appending to gens will probably not work as the generator enumerate(gens) is set up in the beginning. I would use another list for the tail part gens[i+1:], then loop while that is non-empty, remove one of its elements w1 and append it to min_words before continuing with the for w2 ... loop where new products would be appended to the tail. A set could also be used.

@jksuom

jksuom Jul 16, 2017

Member

Appending to gens will probably not work as the generator enumerate(gens) is set up in the beginning. I would use another list for the tail part gens[i+1:], then loop while that is non-empty, remove one of its elements w1 and append it to min_words before continuing with the for w2 ... loop where new products would be appended to the tail. A set could also be used.

This comment has been minimized.

@valglad

valglad Jul 16, 2017

Contributor

I didn't use a set because looping over it consistently would be hard. I'm not quite sure what you are suggesting (the remove one of its elements w1 and append it to min_words before continuing part) but I understand what the potential problem is. I'll try to fix it.

@valglad

valglad Jul 16, 2017

Contributor

I didn't use a set because looping over it consistently would be hard. I'm not quite sure what you are suggesting (the remove one of its elements w1 and append it to min_words before continuing part) but I understand what the potential problem is. I'll try to fix it.

This comment has been minimized.

@jksuom

jksuom Jul 16, 2017

Member

I think that it should be possible to implement "looping over a set tail" along the following lines:

while tail:
    w1 = tail.pop()
    min_words.append(w1)
    for w2 in tail:
        ...

But a list tail could also be used.

@jksuom

jksuom Jul 16, 2017

Member

I think that it should be possible to implement "looping over a set tail" along the following lines:

while tail:
    w1 = tail.pop()
    min_words.append(w1)
    for w2 in tail:
        ...

But a list tail could also be used.

This comment has been minimized.

@valglad

valglad Jul 16, 2017

Contributor

Oh I think I see. I've checked in terminal and appending to the list while looping over it with enumerate does work properly, say:

>>> a = [1,2,3]
>>> for i, e in enumerate(a):
...    print(e)
...    if i == 2:
...        a.append(4)
1
2
3
4

There is potentially a different problem: the words appended in the loop won't be checked against the preceding words. E.g. if gens == [b, b**-1, b*a*b**2] and I add b*a*b and a*b**2 to the end of the list, they won't be compared with b or b**-1 once we get to them.

@valglad

valglad Jul 16, 2017

Contributor

Oh I think I see. I've checked in terminal and appending to the list while looping over it with enumerate does work properly, say:

>>> a = [1,2,3]
>>> for i, e in enumerate(a):
...    print(e)
...    if i == 2:
...        a.append(4)
1
2
3
4

There is potentially a different problem: the words appended in the loop won't be checked against the preceding words. E.g. if gens == [b, b**-1, b*a*b**2] and I add b*a*b and a*b**2 to the end of the list, they won't be compared with b or b**-1 once we get to them.

This comment has been minimized.

@jksuom

jksuom Jul 16, 2017

Member

Maybe w2 should also run through the head part: for w2 in head + list(tail):.

@jksuom

jksuom Jul 16, 2017

Member

Maybe w2 should also run through the head part: for w2 in head + list(tail):.

This comment has been minimized.

@valglad

valglad Jul 16, 2017

Contributor

Yes, I was thinking that, though that'll cause unnecessary repetitions in some cases. I guess it doesn't matter too much. In that case I could just say for w2 in gens: and not actually create tail.

@valglad

valglad Jul 16, 2017

Contributor

Yes, I was thinking that, though that'll cause unnecessary repetitions in some cases. I guess it doesn't matter too much. In that case I could just say for w2 in gens: and not actually create tail.

This comment has been minimized.

@jksuom

jksuom Jul 16, 2017

Member

How would you find the end of looping?

@jksuom

jksuom Jul 16, 2017

Member

How would you find the end of looping?

This comment has been minimized.

@valglad

valglad Jul 16, 2017

Contributor

The end of looping? Is there a possibility for it to not finish looping by itself?

@valglad

valglad Jul 16, 2017

Contributor

The end of looping? Is there a possibility for it to not finish looping by itself?

This comment has been minimized.

@jksuom

jksuom Jul 16, 2017

Member

If you do not create an explicit tail, how do you detect when it has been depleted? (How to replace while tail:?)

@jksuom

jksuom Jul 16, 2017

Member

If you do not create an explicit tail, how do you detect when it has been depleted? (How to replace while tail:?)

This comment has been minimized.

@valglad

valglad Jul 16, 2017

Contributor

I thought that loop wouldn't be necessary considering that appending words while using enumerate doesn't actually cause problems

@valglad

valglad Jul 16, 2017

Contributor

I thought that loop wouldn't be necessary considering that appending words while using enumerate doesn't actually cause problems

Show outdated Hide outdated sympy/combinatorics/perm_groups.py
slp_dict[temp] = [gens.index(gen)] + px_slp
w = list(range(degree))
for s in slp_dict[temp]:
w = _af_rmul(w,gens[s])

This comment has been minimized.

@jksuom

jksuom Jul 16, 2017

Member

Should w be used somewhere?

@jksuom

jksuom Jul 16, 2017

Member

Should w be used somewhere?

This comment has been minimized.

@valglad

valglad Jul 16, 2017

Contributor

No, I think this is left over from when I was making sure things worked properly. I'll remove it.

@valglad

valglad Jul 16, 2017

Contributor

No, I think this is left over from when I was making sure things worked properly. I'll remove it.

Show outdated Hide outdated sympy/combinatorics/fp_groups.py
if self._min_words is None:
gens = self.generators[:]
gens.extend([e**-1 for e in gens])
for i, w1 in enumerate(gens):

This comment has been minimized.

@jksuom

jksuom Jul 17, 2017

Member

The index is not needed in this version.

@jksuom

jksuom Jul 17, 2017

Member

The index is not needed in this version.

@jksuom jksuom merged commit 68838fb into sympy:master Jul 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@valglad valglad deleted the valglad:homomorphism branch Jul 17, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment