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

Compute map from subgroup to parent group #14968

Merged
merged 6 commits into from Jul 28, 2018

Conversation

Projects
None yet
3 participants
@RavicharanN
Copy link
Contributor

RavicharanN commented Jul 24, 2018

Brief description of what is fixed or changed

  • Compute an injective homomorphism between the reidemester generators and the parent group
  • Add method to compute the coset representative of a given coset.

Release Notes

  • combinatorics
    • It is now possible to get the injection of the group returned by subgroup into the parent FpGroup
@sympy-bot

This comment has been minimized.

Copy link

sympy-bot commented Jul 24, 2018

Hi, I am the SymPy bot (v120). I'm here to make sure this pull request has a release notes entry. Please read the guide on how to write release notes.

Click here to see the pull request description that was parsed.

<!-- Your title above should be a short description of what
was changed. Do not include the issue number in the title. -->

#### Brief description of what is fixed or changed
* Compute an injective homomorphism between the reidemester generators and the parent group
* Add method to compute the coset representative of a given coset.

#### To-do
* Add release notes
* Add tests and improve documentation


#### Release Notes

<!-- Write the release notes for this release below. See
https://github.com/sympy/sympy/wiki/Writing-Release-Notes for more information
on how to write release notes. If there is no release notes entry for this PR,
write "NO ENTRY". The bot will check your release notes automatically to see
if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
* combinatorics 
   * It is now possible to get the injection of the group returned by subgroup into the parent `FpGroup`
<!-- END RELEASE NOTES -->

Your release notes are in good order.

Here is what the release notes will look like:

  • combinatorics
    • It is now possible to get the injection of the group returned by subgroup into the parent FpGroup (#14968 by @RavicharanN)

This will be added to https://github.com/sympy/sympy/wiki/Release-Notes-for-1.2.1.

Note: This comment will be updated with the latest check if you edit the pull request. You need to reload the page to see it.

Update

The release notes on the wiki have been updated.

@valglad

This comment has been minimized.

Copy link
Contributor

valglad commented Jul 24, 2018

Need some tests. You could check that the subgroup generators are mapped to elements of the subgroup in the parent group and also that the image is equal to the subgroup order, could also check injectivity.

"""
fp_group: A finitely presented group, an instance of FpGroup
H: A subgroup whose presentation is to be found, given as a list
of words in generators of `fp_grp`
homomorphism: When set to True, return a homomorphism from the subgroup
to the parent group
subgroup: Returns the subgroup defined on the presentation

This comment has been minimized.

@valglad

valglad Jul 24, 2018

Contributor

That's what subgroup() is for. As I've said in the other PR, I'd be inclined to just return C.schreier_gen_elem here when homomorphism is true and build the actual homomorphism in subgroup. If somebody wants to have a homomorphism, they will almost certainly want a subgroup as well - otherwise there's nothing to apply the homomorphism to.

if alpha == 0:
return x
return self.coset_representative(alpha)*x

This comment has been minimized.

@valglad

valglad Jul 24, 2018

Contributor

You are not using this anywhere else. I suppose it could be good to have this sort of a function but it can be more efficient: for example, you could start in row coset, find the first x s.t. gamma < coset where gamma = self.table[coset][self.A_dict[x]] and return coset_representative(gamma)*x**-1

@@ -118,7 +118,7 @@ def identity(self):
def __contains__(self, g):
return g in self.free_group

def subgroup(self, gens, C=None):
def subgroup(self, gens, C=None, homomorphism=False):
'''
Return the subgroup generated by `gens` using the
Reidemeister-Schreier algorithm

This comment has been minimized.

@valglad

valglad Jul 24, 2018

Contributor

This should have an explanation of homomorphism and actually an example of homomorphism=True could be useful (just to demonstrate that the homomorphism returns an image of an element of subgroup(...) in the parent group)

if not all([isinstance(g, FreeGroupElement) for g in gens]):
raise ValueError("Generators must be `FreeGroupElement`s")
if not all([g.group == self.free_group for g in gens]):
raise ValueError("Given generators are not members of the group")
g, rels = reidemeister_presentation(self, gens, C=C)
if homomorphism:
g, rels, _gens = reidemeister_presentation(self, gens, C=C, homomorphism=True)

This comment has been minimized.

@valglad

valglad Jul 26, 2018

Contributor

You end up running reidemeister_presentation twice if homomorphism=True, so better move the first one into an "else".

>>> T(K.generators[0]*K.generators[1])
x*y*x**-1*y**2*x**-1
>>> T(K.generators[0]*K.generators[1])
x*y*x**-1*y**2*x**-1

This comment has been minimized.

@valglad

valglad Jul 26, 2018

Contributor

This is repeated twice. Also, I think T(K.generators) is enough. We should still have more tests in the main tests file

>>> f = FpGroup(F, [x**3, y**3, x**-1*y**-1*x*y])
>>> C = coset_enumeration_r(f, [x])
>>> C.table
[[0, 0, 1, 2], [1, 1, 2, 0], [2, 2, 0, 1], [None, 1, None, None], [1, 3, None, None]]

This comment has been minimized.

@valglad

valglad Jul 26, 2018

Contributor

Best to run C.compress() first - then the table will look what it should, without all these Nones

@valglad

This comment has been minimized.

Copy link
Contributor

valglad commented Jul 26, 2018

I think that "It is now possible to get the injection of the group returned by subgroup into the parent FpGroup" or something along those lines would be clearer for the release notes. At the moment, it sounds like given a group H and the fact that it is a subgroup of G, we can compute the injection of H into G - but that's not what's being implemented.

@@ -171,10 +171,21 @@ def test_fp_subgroup():
f = FpGroup(F, [x**4, y**2, x*y*x**-1*y])
S = FpSubgroup(f, [x*y])
assert (x*y)**-3 in S
K, T = f.subgroup([x*y], homomorphism=True)
assert T(K.generators) == [y*x**-1]
assert T.is_injective()

This comment has been minimized.

@valglad

valglad Jul 27, 2018

Contributor

You should check that T.image().order() = K.order() to verify the image is indeed isomorphic to the subgroup and possibly T(K.generators) in S would be a better check for generators. I would actually write a unit test as a subfunction here to avoid repetition. Say:

def _test(K, T):
   assert T(K.generators) in S
   assert T.is_injective()
   assert T.image.order() == S.order()

and just run _test(f.subgroup([x**-1*y*x], homomorphism=True), etc after that.

for elem in _gens:
assert elem in S
assert T.is_injective()
assert T.image().order() == S.order()

This comment has been minimized.

@valglad

valglad Jul 28, 2018

Contributor

I believe this sort of thing usually goes inside the unit test for which it is relevant, because it itself isn't an independent test. So this should go after def test_fp_subgroup() - a purely stylistic thing but it seems that it's usually done that way.

def _test_subgroup(K, T, S):
_gens = T(K.generators)
for elem in _gens:
assert elem in S

This comment has been minimized.

@valglad

valglad Jul 28, 2018

Contributor

Or you could say assert all(elem in S for elem in _gens)

@@ -1209,11 +1248,13 @@ def elimination_technique_2(C):
C._schreier_generators = gens
return C._schreier_generators, C._reidemeister_relators

def reidemeister_presentation(fp_grp, H, C=None):
def reidemeister_presentation(fp_grp, H, C=None, homomorphism=False, subgroup=False):

This comment has been minimized.

@valglad

valglad Jul 28, 2018

Contributor

Just noticed that you didn't remove the subgroup keyword - it doesn't do anything anymore

C.schreier_gen_elem = {}
for gen in gens:
C.schreier_gen_elem[gen] = C._schreier_gen_elem[str(gen)]
_gens.append(C.schreier_gen_elem[gen])

This comment has been minimized.

@valglad

valglad Jul 28, 2018

Contributor

Does C.schreier_gen_elem actually serve a purpose? It seems enough to just compute _gens. You could even do _gens = [C._schreier_gen_elem[str(gen)] for gen in gens] in just one line

This comment has been minimized.

@RavicharanN

RavicharanN Jul 28, 2018

Author Contributor

That could be removed. Thought I might be useful elsewhere by making it an attribute, but, I don't think it's necessary.

@valglad

This comment has been minimized.

Copy link
Contributor

valglad commented Jul 28, 2018

This looks good now

@valglad valglad merged commit 2b4afb6 into sympy:master Jul 28, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
sympy-bot/release-notes The release notes look OK
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment