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

Add quotient methods #14981

Open
wants to merge 11 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@RavicharanN
Copy link
Contributor

RavicharanN commented Jul 27, 2018

Brief description of what is fixed or changed

Add quotient methods for FpGroups.

Release Notes

  • combinatorics
    • It is now possible to compute subgroup quotient and the maximal abelian quotients of an FpGroup.
@sympy-bot

This comment has been minimized.

Copy link

sympy-bot commented Jul 27, 2018

Hi, I am the SymPy bot (v128). I'm here to help you write a release notes entry. Please read the guide on how to write release notes.

Your release notes are in good order.

Here is what the release notes will look like:

  • combinatorics
    • It is now possible to compute subgroup quotient and the maximal abelian quotients of an FpGroup. (#14981 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.

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. -->

<!-- If this pull request fixes an issue, write "Fixes #NNNN" in that exact
format, e.g. "Fixes #1234". See
https://github.com/blog/1506-closing-issues-via-pull-requests .-->


#### Brief description of what is fixed or changed
Add quotient methods for `FpGroup`s. 



#### 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 compute subgroup quotient and the maximal abelian quotients of an `FpGroup`. 
<!-- END RELEASE NOTES -->

"of FpGroup")

if not (G.free_group == free_group
or H.free_group == free_group):

This comment has been minimized.

@valglad

valglad Jul 27, 2018

Contributor

If you pass G and H as lists, then they have no attribute free_group, and if you pass them as FpGroups, then in general, their presentations won't be on free_group. You could theoretically make G and H instances of FpSubgroup, in which case this check would work. However, it still wouldn't be write to say q_group = FpGroup(free_group, q_relators) at the end - it works when fp_group == G, but not otherwise.

This comment has been minimized.

@RavicharanN

RavicharanN Jul 27, 2018

Author Contributor

You could theoretically make G and H instances of FpSubgroup

We could probably make the user define the free group. Coz' this problem might only be during the polycyclic series computation in which we pass the lists, in all other cases the user would mostly pass them as the instances of the FpGroup, right? This could be avoided if we add an argument for the free_group. Or maybe we could add it as a default argument and check if free_group is defined when both G and H are lists (or maybe, only G), if it isn't we could raise a ValueError.


def maximal_abelian_quotient(G):
'''
Compute the Maximal abelian quitoent of an FpGroup

This comment has been minimized.

@valglad

valglad Jul 27, 2018

Contributor

Typo quotient and Maximal shouldn't be capitalised


# Compute the commutator subgroup
_com_grp = _G.derived_series()
com_grp_gens = T.invert(_com_grp.generators)

This comment has been minimized.

@valglad

valglad Jul 27, 2018

Contributor

_com_grp is either a tuple or a list, so it won't have the attribute generators. Don't you simply want to use G.derived_subgroup() here (which is an FpGroup method so no need to change to the permutation group manually)?

This comment has been minimized.

@RavicharanN

RavicharanN Jul 27, 2018

Author Contributor

_com_grp is either a tuple or a list, so it won't have the attribute generators.

This wasn't tested yet so I might've accidentally used derived_Series instead of derived_subgroup.

com_subgroup_rels = com_subgroup.relators
quotient_rels = homomorphism(com_subgroup_rels) + G.generators

return FpGroup(G.free_group, quotient_rels)

This comment has been minimized.

@valglad

valglad Jul 27, 2018

Contributor

Or you could simply say return subgroup_quotient(G, G.derived_subgroup()) and not have to do anything else


return subgroup_quotient(G, G.derived_subgroup())

def epimorphism_pgroup(F, G, n=None):

This comment has been minimized.

@valglad

valglad Jul 29, 2018

Contributor

Just epimorphism would make more sense as a name. Perhaps, this could also have a keyword all which is False by default. Sometimes all one needs is a single epimorphism - computing all of them takes a lot of time so could be selected as an option when needed. What is n?

Add conjgacy class implemnetation
'''
if not isinstance(F, FpGroup):
raise ValueError("The group must be an FpGroup")

This comment has been minimized.

@valglad

valglad Jul 29, 2018

Contributor

Why? This method would work just fine for other sorts of groups

_H, h_isomorphism = H._to_perm_group()

if g_order == S.infinity or h_order == S.infinity:
raise NotImplementedError("Epimorphism methods are not implemented for infinite groups.")

This comment has been minimized.

@valglad

valglad Jul 29, 2018

Contributor

This would work for infinite domains

raise ValueError("The group must be an FpGroup")

g_order = G.order()
h_order = H.order()

This comment has been minimized.

@valglad

valglad Jul 29, 2018

Contributor

You arguments are F and G, not G and H. I assume you just copied the code from that isomorphism method from earlier and forgot to change the names? In any case, this looks a lot like the stuff there so, as usual, it would be better to make that code more general and this - a special case of it. Maybe it could be homomorphism rather than isomorphism with keywords injective, surjective and all (for computing all homomorphisms). Then you can have epimorphism with surjective=True, monomorphism with injective=True and isomorphism with both of them True.

A separate point is that in the case of surjective homomorphisms, if h_order doesn't divide g_order, then there are no epimorphisms so you can immediately return [] (or None). In the case of a monomorphism, it's the other way around: g_order had to divide h_order

@valglad

This comment has been minimized.

Copy link
Contributor

valglad commented Jul 29, 2018

The last two commits were small changes. I think I've mentioned that it's preferable to push bigger chunks of changes. Small commits are fine if the PR is almost ready and only a little change is required to complete it or if the tests are failing for a silly reason (like a typo) and you didn't notice it until pushing (e.g. if you forgot to run tests in python3 as well as python2). Other than that, it's best if each commit is a self-contained completed chunk of work. For example, you could have pushed epimorphisms in its final form, with properly completed docstring and that conjugacy computation you mentioned (though I think that finding conjugacy classes could be hard to do, in general), and included the changes from the second commit there as well

@RavicharanN RavicharanN force-pushed the RavicharanN:quotient-methods branch 2 times, most recently from 1661e2a to 537446d Jul 31, 2018

@RavicharanN RavicharanN force-pushed the RavicharanN:quotient-methods branch from 537446d to 893d661 Aug 1, 2018

>>> F, x, y = free_group("x, y")
>>> f = FpGroup(F, [x**2, y**3, (x*y)**4])
>>> k = f.derived_series()

This comment has been minimized.

@valglad

valglad Aug 1, 2018

Contributor

k isn't used anywhere else

@@ -527,38 +560,25 @@ def subgroup_quotient(G, H, fp_group=None):
if isinstance(G, list) and not fp_group:
raise ValueError("The fp_group must be"
"defined when the group is a list")

This comment has been minimized.

@valglad

valglad Aug 1, 2018

Contributor

This error could go into the previous if statement. E.g.

if not fp_group:
   if isinstance(G, list):
      ...
   else:
       ...
if ((isinstance(H, list) and not all(elem in free_group for elem in H))
or (isinstance(H, FpGroup) and not (H.free_group == free_group))):
raise ValueError("The elements groups must belong to"
"to the same free group as that of the parent group")

This comment has been minimized.

@valglad

valglad Aug 1, 2018

Contributor

Could be worth writing a little function _check that does this and then running

_check(G)
_check(H)

I think it would look a bit neater.

I would also suggest making the error message something like "The subgroup generators don't belong to the parent group". The fact that in SymPy, to belong to an FpGroup means the same as to belong to a FreeGroup from which it was constructed is a design choice, and might confuse a regular user who is not familiar with the implementation details

'''
def _get_map(fp_group, F):

This comment has been minimized.

@valglad

valglad Aug 1, 2018

Contributor

_get_pres could be a better name because you are returning a presentation rather than a map

g_gens, g_rels = _get_map(fp_group, G)
h_gens, h_rels = _get_map(fp_group, H)

q_relators = list(g_rels) + list(h_gens)
q_group = FpGroup(free_group, q_relators)

This comment has been minimized.

@valglad

valglad Aug 1, 2018

Contributor

This doesn't work when both G and H are proper subgroups of fp_group. For example:

>>> F, a, b, c = free_group("a b c")
>>> G = [a,b]
>>> H = [a]

Then G/H should be isomorphic to a free group on a so it should be <a, b, c | b, c> but q_group will be <a, b, c | b> which is a free group on 2 generators.

What you should really do, is find a presentation of G along with an injection T_G into fp_group, translate the generators of H into elements of this new presentation (btw, this would also let you check if H is a subgroup of G which you should do), then return q_group on the free group of the presentation of G. Does this make sense?

This comment has been minimized.

@RavicharanN

RavicharanN Aug 1, 2018

Author Contributor

then return q_group on the free group of the presentation of G

Wouldn't that be a problem if the computed quotients don't belong to the same FreeGroup as that of the FpGroup?

return subgroup_quotient(G, G.derived_subgroup())
See Also
========
subgroup_quotient

This comment has been minimized.

@valglad

valglad Aug 1, 2018

Contributor

"Examples" usually come before "See Also" in docstrings

>>> from sympy.combinatorics.fp_groups import (FpGroup, low_index_subgroups,
... reidemeister_presentation, FpSubgroup)
>>> from sympy.combinatorics.free_groups import free_group
>>> from sympy.combinatorics.fp_groups import subgroup_quotient, maximal_abelian_quotient

This comment has been minimized.

@valglad

valglad Aug 1, 2018

Contributor

Again, don't need to import most of these

homomorphism_list.append(T)

return homomorphism_list
raise ValueError("The group must be a finitely presented group")

This comment has been minimized.

@valglad

valglad Aug 1, 2018

Contributor

Just "finitely presented" is enough

@@ -421,7 +421,7 @@ def block_homomorphism(group, blocks):
H = GroupHomomorphism(group, codomain, images)
return H

def group_isomorphism(G, H, isomorphism=True):
def group_isomorphism(G, H, isomorphism=True, epimorphism=False, all=False):

This comment has been minimized.

@valglad

valglad Aug 1, 2018

Contributor

It shouldn't be called "isomorphism" if we are allowing it to compute epimorphisms as well. That's why I suggested calling it "homomorphism" - though I guess we have that one. Perhaps find_homomorphism since we are not defining a specific one here, but asking for any one? And it would make sense to give that keywords injective and surjective because that would give you the most versatility. And what used to be isomorphism could actually become something like check (so that check=True means only check existence, don't actually compute the homomorphism)

if (g_order != h_order) or (G.is_abelian != H.is_abelian):
if not isomorphism:
return False
return (False, None)

This comment has been minimized.

@valglad

valglad Aug 1, 2018

Contributor

If you make your keywords injective and surjective, than you'll need an if statement for each of them and G.is_abelian != H.is_abelian could be included as well, though split into parts like this:

if injective:
    if (order stuff) or not G.is_abelian and H.is_abelian:
        #false
if surjective:
    if (order stuff) or G.is_abelian and not H.is_abelian:
        #false

That's because you can't inject a non-abelian group into an abelian one, or have a surjection from an abelian group to a non-abelian one

G -- An FpGroup.
H -- A subgroup of `G`.
parent_group -- The parent group of `G` and `H`. This is used to know
the free_group on which the quotient group has to be defined.

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

Not true anymore. parent_group is simply something that should be specified when G and H are given by lists of generators

Arguments
=========
G -- An FpGroup.
H -- A subgroup of `G`.

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

Both of these can be specified as lists of FreeGroupElements

'''
Compute the quotient group G/H.
The quotient group is computed on a new FreeGroup when the
`G` is a list and is generated by a proper subset of the
generators of the parent_group.

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

"the" before G is not necessary, and it is generated on a new FreeGroup when G is a list of any elements of parent_group (even if those elements generate the whole group - it often won't be clear until you actually compute a presentation of G and, once you've done it, you might as well use the free group of the new presentation)

=======
* Quotient group along with the homomorphism when G is
generated by a proper subset of the generators of the parent_group
* Only the quotient group in all other cases.

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

Should say "along with a homomorphism from the quotient to the parent group (which is parent_group if specified and G otherwise) whose image is isomorphic to G. As before, G can be generated by any set of elements, not just generators of parent_group. I also think the user should choose whether or not they want a homomorphism, like in subgroup

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

As before, G can be generated by any set of elements, not just generators of parent_group

This still stands. It shouldn't say when G is generated by a proper subset of the generators of the parent_group

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

G is always generated by some elements of the parent group, so there is no need to say anything about it at all. The homomorphism is returned when homomorphism=True, and no other conditions matter

This comment has been minimized.

@valglad

valglad Aug 10, 2018

Contributor

This is still relevant

if ((isinstance(F, list) and not all(elem in free_group for elem in F))
or (isinstance(F, FpGroup) and not (F.free_group == free_group))):
raise ValueError("The elements groups must belong to"
"to the same free group as that of the parent group")

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

Should be just "The group elements must belong to the parent group" - I think I explained last time that it's only a feature of SymPy's implementation that belonging to a free group or an FpGroup on it is the same thing; in general, in group theory it isn't


if not isinstance(fp_group, FpGroup):
if not isinstance(parent_group, FpGroup):
raise ValueError("The parent group must be an instance"
"of FpGroup")

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

or FreeGroup. I can't recall of the top of my head if FreeGroups count as FpGroups in SymPy


# If the generators of `G` is a proper subgroup of the
# generators of `parent_group` return the `FpGroup` on a new presentation.
if isinstance(G, list) and all(elem in parent_group.generators for elem in G):

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

all(elem in parent_group.generators for elem in G) checks that G is any subset of parent_group, not necessarily proper. But in any case, as I've said, I think we might as well compute the presentation on the free group of G, even if G turns out to be isomorphic to parent_group

This comment has been minimized.

@valglad

valglad Aug 6, 2018

Contributor

I think his should just be if isinstance(G, list)

h_gens = T(h_gens)
h_rels = T(h_rels)
free_group = G.free_group
q_realtors = list(G.relators) + list(h_gens)

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

q_relators

h_rels = T(h_rels)
free_group = G.free_group
q_realtors = list(G.relators) + list(h_gens)
return FpGroup(free_group, q_realtors), T

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

T will need to change slightly - you should set its domain to the quotient, because as it stands T.domain == G (you might not need to recreate the homomorphism for that - have a look at what should change)

g_gens, g_rels = _get_map(fp_group, G)
h_gens, h_rels = _get_map(fp_group, H)
g_gens, g_rels = _get_pres(parent_group, G)
h_gens, h_rels = _get_pres(parent_group, H)

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

I think this could be laid out better. h_gens, h_rels = ... could go outside any if statements because you are going to compute them anyway. G, T = parent_group.subgroup(G, homomorphism=True) could be done whenever G is a list, same for free_group = G.free_group. And after that the rest is the same for both cases

if (g_order != h_order) or (G.is_abelian != H.is_abelian):
if not isomorphism:
if injective:
if (g_order != h_order) or not G.is_abelian and H.is_abelian:

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

You can easily have an injective homomorphism where g_order != h_order - same for surjective homomorphisms

if T.is_isomorphism():
# It is a valid isomorphism
if ((injective and surjective and T.is_isomorphism()) or
(injective and T.is_injective()) or (surjective and T.is_surjective())):

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

Or just if T.is_injective == injective and T.is_surjective == surjective

surjective (boolean) -- When set to True, this computes all possible epimorphisms.
check (boolean) -- When set to False, This avoids the computation of
homomorphism and only checks if there exists
an isomorphism/epimorphism/monomorphism between the groups.

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

You called this argument compute rather than check. Also only check the existence of a homomorphism, avoiding computation where possible is shorter

isomorphism (boolean) -- This is used to avoid the computation of homomorphism
when the user only wants to check if there exists
an isomorphism between the groups.
injective (boolean) -- When set to True, it compute the possible monomorphism

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

"compute a monomorphism, if possible"

when the user only wants to check if there exists
an isomorphism between the groups.
injective (boolean) -- When set to True, it compute the possible monomorphism
surjective (boolean) -- When set to True, this computes all possible epimorphisms.

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

"compute an epimorphism, if possible" (you are not computing all, unless all=True) (I think I mentioned this: "compute" is often preferable to "this/it computes" in docstrings)

Summary:
Uses the approach suggested by Robert Tarjan to compute the isomorphism between two groups.
Uses the approach suggested by Robert Tarjan to compute the homomorphism between two groups.

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

"a homomorphism"

First, the generators of `G` are mapped to the elements of `H` and
we check if the mapping induces an isomorphism.
we check if the mapping induces an isomorphism/epimorphism/monomorphism.

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

I think "a homomorphism with required properties" would sound better

(False, None)
>>> F, a, b = free_group("a, b")
>>> G = FpGroup(F, [a**3, b**3, (a*b)**2])
>>> H = AlternatingGroup(4)
>>> (check, T) = group_isomorphism(G, H)
>>> (check, T) = find_homomorphism(G, H)
>>> check
True
>>> T(b*a*b**-1*a**-1*b**-1)
(0 2 3)

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

Some examples with other keywords would be good

@@ -421,27 +421,31 @@ def block_homomorphism(group, blocks):
H = GroupHomomorphism(group, codomain, images)
return H

def group_isomorphism(G, H, isomorphism=True, epimorphism=False, all=False):
def find_homomorphism(G, H, injective=True, surjective=True, compute=True, all=False):

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

I think injective and surjective should be False by default because it would be more natural to say find_homomorphism(..., injective=True) instead of find_homomorpism(..., surjective=False) is you were looking for an injection. You could define group_isomorphism to use as a shortcut though

list = group_isomorphism(G, H, epimorphism=True, all=True)
assert all((elem[0].is_isomorphism() and elem[1].is_surjective()) for elem in list)
# Find all possible epimorphisms.
list = find_homomorphism(G, H, injective=False, all=True)

This comment has been minimized.

@valglad

valglad Aug 5, 2018

Contributor

list is a reserved keyword - best to use something else

@valglad

This comment has been minimized.

Copy link
Contributor

valglad commented Aug 5, 2018

The test coverage should be improved - for example, there are no tests for quotient methods that use G and H as lists and specify a parent group and test if the homomorphism works properly. find_homomorphism could do with a test on finding injections.

@RavicharanN RavicharanN force-pushed the RavicharanN:quotient-methods branch from ec047e5 to b1ddd01 Aug 6, 2018

H -- A subgroup of `G`.
parent_group -- The parent group of `G` and `H`. This is used to know
the free_group on which the quotient group has to be defined.
G -- A list of `FreeGroupElement`s.

This comment has been minimized.

@valglad

valglad Aug 6, 2018

Contributor

or an FpGroup

G -- A list of `FreeGroupElement`s.
H -- A list of `FreeGroupElement`s.
parent_group -- A group specified when `G` and `H` are given by a list of generators.
homomorphism -- Return a homomorphism when the quotient is computed on a new `FreeGroup`.

This comment has been minimized.

@valglad

valglad Aug 6, 2018

Contributor

Should return a homomorphism whenever homomorphism=True

generated by a proper subset of the generators of the parent_group
* Quotient group along with the homomorphism from the quotient to the
parent_group whose image is isomorphic to G when G is generated by
a proper subset of the generators of the parent_group

This comment has been minimized.

@valglad

valglad Aug 6, 2018

Contributor

This should be "when homomorphism=True"


# If the generators of `G` is a proper subgroup of the
# generators of `parent_group` return the `FpGroup` on a new presentation.
if isinstance(G, list) and all(elem in parent_group.generators for elem in G):

This comment has been minimized.

@valglad

valglad Aug 6, 2018

Contributor

I think his should just be if isinstance(G, list)

if injective:
if (g_order != h_order) or not G.is_abelian and H.is_abelian:
if injective and surjective:
if (g_order != h_order) or (G.is_abelian != H.is_abelian):

This comment has been minimized.

@valglad

valglad Aug 6, 2018

Contributor

The condition on order can be split up and each part put under either if injective or if surjective. I mentioned this the first time I suggested these keywords: you can't have an injective homomorphism if g_order doesn't divide h_order (just by Lagrange's theorem), and you can't have a surjective one if h_order doesn't divide g_order. When you have both injective and surjective set to True, these two conditions will automatically give you g_order != h_order

@@ -533,8 +535,7 @@ def find_homomorphism(G, H, injective=True, surjective=True, compute=True, all=F
if isinstance(H, FpGroup):
images = h_isomorphism.invert(images)
T = homomorphism(G, H, G.generators, images, check=False)
if ((injective and surjective and T.is_isomorphism()) or
(injective and T.is_injective()) or (surjective and T.is_surjective())):
if (injective == T.is_injective()) or (surjective == T.is_surjective()):

This comment has been minimized.

@valglad

valglad Aug 6, 2018

Contributor

Shouldn't this be and?

This comment has been minimized.

@RavicharanN

RavicharanN Aug 6, 2018

Author Contributor

Well, if we use an and we might miss out the cases where it is both epimorphism (or monomorphism) and also an isomorphism whenever only one of injectiveor surjective is True, right?

This comment has been minimized.

@valglad

valglad Aug 6, 2018

Contributor

An isomorphism is an epimorphism (and also a monomorphism). Using or would actually make us find more homomorphisms than needed: for example, if injective=True, this condition will be true for any injective homomorphism, surjective or not, - and they'd all be returned, even if the user specified surjective=True

@@ -548,6 +549,34 @@ def find_homomorphism(G, H, injective=True, surjective=True, compute=True, all=F
return False
return (False, None)

def group_isomorphism(G, H, all=False):
'''
Compute an isomorphism(if possible) between 2 groups.

This comment has been minimized.

@valglad

valglad Aug 6, 2018

Contributor

missing space

Arguments:
G (a finite `FpGroup` or a `PermutationGroup`) -- First group
H (a finite `FpGroup` or a `PermutationGroup`) -- Second group
all (boolean) -- compute all possible isomorphism when set to True.

This comment has been minimized.

@valglad

valglad Aug 6, 2018

Contributor

"isomorphisms"

@@ -1,6 +1,6 @@
from sympy.combinatorics import Permutation
from sympy.combinatorics.perm_groups import PermutationGroup
from sympy.combinatorics.homomorphisms import homomorphism, find_homomorphism, is_isomorphic
from sympy.combinatorics.homomorphisms import homomorphism, find_homomorphism, is_isomorphic, group_isomorphism

This comment has been minimized.

@valglad

valglad Aug 6, 2018

Contributor

This line is looking long - it's probably time to split it

=======
* Quotient group along with the homomorphism when G is
generated by a proper subset of the generators of the parent_group
* Only the quotient group in all other cases.

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

As before, G can be generated by any set of elements, not just generators of parent_group

This still stands. It shouldn't say when G is generated by a proper subset of the generators of the parent_group

if injective and surjective:
if (g_order != h_order) or (G.is_abelian != H.is_abelian):
if injective:
if (h_order%g_order != 0) or not G.is_abelian and H.is_abelian:

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

There should be spaces on either side of "%" - same for the surjective case

assert T.order() == f.order()/H.order()

F, x, y = free_group("x, y")
f = FpGroup(F, [x**2, y**3, (x*y)**4])

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

You are using the same f here in all examples, and the same free group F so there is no need to redefine them every time. Also, it would be good to have some tests here with parent_group keyword (preferably one in which G is a proper subgroup of parent_group), and something that returns a homomorphism. Doctests are one thing but they more examples, rather than actual tests so you should still include tests for all possible features even if you have some in doctests

assert T(K.generators) == list(f.generators)
G = f.subgroup(G)
H = f.subgroup(H)
assert K.order() == G.order()/H.order()

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

Since this is test_order, I think it would more appropriate to just check the order here, with no need for parent_group and homomorphism - that sort of thing should be tested in test_subgroup_quotient

This comment has been minimized.

@valglad

valglad Aug 14, 2018

Contributor

We still have no test where G is a proper subgroup of parent_group. Maybe the test above could be changed.

from sympy.combinatorics.fp_groups import (FpGroup, low_index_subgroups,
reidemeister_presentation, FpSubgroup)
from sympy.combinatorics.fp_groups import (FpGroup, low_index_subgroups, reidemeister_presentation,
FpSubgroup, subgroup_quotient, maximal_abelian_quotient)

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

Is this longer than 72 symbols? It needs to be shortened if it is

@@ -448,25 +453,27 @@ def group_isomorphism(G, H, isomorphism=True):
>>> from sympy.combinatorics.perm_groups import PermutationGroup
>>> from sympy.combinatorics.free_groups import free_group
>>> from sympy.combinatorics.fp_groups import FpGroup
>>> from sympy.combinatorics.homomorphisms import homomorphism, group_isomorphism
>>> from sympy.combinatorics.homomorphisms import homomorphism, find_homomorphism, group_isomorphism

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

You are not using homomorphism or PermutationGroup in the examples

@valglad

This comment has been minimized.

Copy link
Contributor

valglad commented Aug 8, 2018

I'm still reviewing

=======
* Quotient group along with the homomorphism when G is
generated by a proper subset of the generators of the parent_group
* Only the quotient group in all other cases.

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

G is always generated by some elements of the parent group, so there is no need to say anything about it at all. The homomorphism is returned when homomorphism=True, and no other conditions matter

# generators of `parent_group` return the `FpGroup` on a new presentation.
if isinstance(G, list) and all(elem in parent_group.generators for elem in G):
# If `G` is generated by any set of elements of the
# `parent_group` return the `FpGroup` on a new presentation.

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

Again, G is always generated by any set of elements so no need to say anything about it. Also, I think you meant on a new free group

@@ -83,6 +84,9 @@ def test_isomorphisms():
assert check
assert T(b*a*b**-1*a**-1*b**-1) == Permutation(0, 2, 3)
assert T(b*a*b*a**-1*b**-1) == Permutation(0, 3, 2)
# Find all possible epimorphisms.
list_hom = find_homomorphism(G, H, surjective=True, all=True)
assert all(elem.is_surjective() for elem in list_hom)

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

Would be good to have a couple more find_homomorphism tests that don't just look for isomorphisms. Maybe something injective, and you should also try compute=False. The more features you cover the better

If compute = False -- Returns a boolean.
If compute = True -- Returns a boolean and a homomorphism with required properties
between `G` and `H`.
If all = True -- Returns all possible specified homomorphisms as a list.

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

I think saying "Return" everywhere here would be slightly better

========
>>> from sympy.combinatorics.fp_groups import FpGroup
>>> from sympy.combinatorics.free_groups import free_group
>>> from sympy.combinatorics.fp_groups import subgroup_quotient, maximal_abelian_quotient

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

You are not using subgroup_quotient here

'''
Compute the quotient group G/H.
The quotient group is computed on a new FreeGroup when
`G` list of the elements of parent_group.

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

"is a list" - typo

Returns
=======
* When `homomorphism` = True, quotient group along with the homomorphism

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

homomorphism = True ("`" closed too early)

>>> G = f.subgroup(G)
>>> H = f.subgroup(H)
>>> T.order() == G.order()/H.order()
True

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

You are specifically choosing G to be the same a f here so you can just not compute G as a subgroup and simply use f.order()/H.order(). But generally, as I've said, we should have a test where G is a proper subgroup of the parent group - though it's enough to just do it in the tests. However, it would be good to have an actual example with homomorphism=True, to demonstrate how that feature works

This comment has been minimized.

@valglad

valglad Aug 10, 2018

Contributor

You still don't have an example with homomorphism=True

h_rels = T.invert(h_rels)
free_group = G.free_group

g_gens, g_rels = _get_pres(parent_group, G)

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

Whatever G is at the start, by this point it's already an FpGroup because of the previous if statement so there is no need for _get_pres to take parent_group as an argument

'''
def _get_pres(parent_group, F):
from sympy.combinatorics.homomorphisms import homomorphism

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

Not using homomorphism here

"defined when the group is a list")
parent_group = G

free_group = parent_group.free_group

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

We don't need this statement - we always use the free group of G

This comment has been minimized.

@valglad

valglad Aug 14, 2018

Contributor

Still the case

This comment has been minimized.

@RavicharanN

RavicharanN Aug 19, 2018

Author Contributor

But, it has to be defined before the _check function is called.

This comment has been minimized.

@valglad

valglad Aug 19, 2018

Contributor

I see. Then, perhaps, this line should move into the definition of _check since it's the only reason we have it?


# Return the quotient group with presentation
# <G.generators|q_realtors>
if homomorphism and T:

This comment has been minimized.

@valglad

valglad Aug 8, 2018

Contributor

We should really return a homomorphism whenever homomorphism is true, whether or not G is specified as a list but you can define it easily in that case, perhaps by adding an else to if isinstance(G, list) above

This comment has been minimized.

@valglad

valglad Aug 19, 2018

Contributor

This is still relevant. The only condition for returning a homomorphism should be homomorphism == True - whether or not T is defined shouldn't matter

This comment has been minimized.

@RavicharanN

RavicharanN Aug 19, 2018

Author Contributor

But, T is defined only when G is a list and we need a homomorphism only when the quotient group is defined on another free_group, right?

This comment has been minimized.

@valglad

valglad Aug 19, 2018

Contributor

I think that would lead to some strange behaviour. It's true that the homomorphism will be trivial when G == parent_group, but if someone said subgroup_quotient(..., homomorphism=True), then they'd expect to get back a homomorphism. I think it would be strange to be able to say homomorphism=True but not get anything back. We might as well return an identity homomorphism when T isn't defined

@valglad

This comment has been minimized.

Copy link
Contributor

valglad commented Aug 8, 2018

Now I've reviewed everything. This is coming along well, we might be able to get it merged by the end of the week.

=======
* Quotient group along with the homomorphism when G is
generated by a proper subset of the generators of the parent_group
* Only the quotient group in all other cases.

This comment has been minimized.

@valglad

valglad Aug 10, 2018

Contributor

This is still relevant

>>> G = f.subgroup(G)
>>> H = f.subgroup(H)
>>> T.order() == G.order()/H.order()
True

This comment has been minimized.

@valglad

valglad Aug 10, 2018

Contributor

You still don't have an example with homomorphism=True

@@ -101,5 +103,7 @@ def test_find_homomorphism():
# Two groups of the same prime order are isomorphic to each other.
G = FpGroup(F, [a, b**5])
H = CyclicGroup(5)
list_hom = find_homomorphism(G, H, surjective=True, all=True)
assert all(elem.is_injective() for elem in list_hom)

This comment has been minimized.

@valglad

valglad Aug 10, 2018

Contributor

It would be better if G and H weren't isomorphic so that we could test just the surjections.

@@ -87,6 +87,8 @@ def test_find_homomorphism():
# Find all possible epimorphisms.
list_hom = find_homomorphism(G, H, surjective=True, all=True)
assert all(elem.is_surjective() for elem in list_hom)
check = find_homomorphism(G, H, injective=True, surjective=True, compute=False)

This comment has been minimized.

@valglad

valglad Aug 10, 2018

Contributor

This is the same as is_isomorphic which works differently from purely isomorphic cases. It would be better to have a test where G and H aren't isomorphic and we are looking for a proper injection

"defined when the group is a list")
parent_group = G

free_group = parent_group.free_group

This comment has been minimized.

@valglad

valglad Aug 14, 2018

Contributor

Still the case

@@ -537,4 +582,4 @@ def is_isomorphic(G, H):
Returns -- boolean
'''
return group_isomorphism(G, H, isomorphism=False)
return find_homomorphism(G, H, injective=True, surjective=True ,compute=False)

This comment has been minimized.

@valglad

valglad Aug 14, 2018

Contributor

Typo with the comma and space: True, compute=

assert T(K.generators) == list(f.generators)
G = f.subgroup(G)
H = f.subgroup(H)
assert K.order() == G.order()/H.order()

This comment has been minimized.

@valglad

valglad Aug 14, 2018

Contributor

We still have no test where G is a proper subgroup of parent_group. Maybe the test above could be changed.

H = f.subgroup(H)
assert K.order() == G.order()/H.order()

F, x, y = free_group("x, y")

This comment has been minimized.

@valglad

valglad Aug 14, 2018

Contributor

This is already defined

@@ -83,11 +84,18 @@ def test_isomorphisms():
assert check
assert T(b*a*b**-1*a**-1*b**-1) == Permutation(0, 2, 3)
assert T(b*a*b*a**-1*b**-1) == Permutation(0, 3, 2)
# Find all possible epimorphisms.

This comment has been minimized.

@valglad

valglad Aug 14, 2018

Contributor

This comment isn't right anymore

@@ -83,11 +84,18 @@ def test_isomorphisms():
assert check
assert T(b*a*b**-1*a**-1*b**-1) == Permutation(0, 2, 3)
assert T(b*a*b*a**-1*b**-1) == Permutation(0, 3, 2)
# Find all possible epimorphisms.
list_hom = find_homomorphism(G, H, injective=True, all=True)

This comment has been minimized.

@valglad

valglad Aug 14, 2018

Contributor

It would be better to do this test for G and H that are not isomorphic.
Additionally, a test of either injective or surjective (but not both at the same time) homomorphisms with compute=False could be good. For example, you could take G and H where G.order() doesn't divide H.order(), or where G is abelian and H isn't, and assert that there isn't an injective homomorphism.

Another thing I've noticed, is that you redefine free groups in the second and third tests when you could just reuse F and E

# Find all possible epimorphisms.
list_hom = find_homomorphism(G, H, injective=True, all=True)
assert all(elem.is_injective() for elem in list_hom)
check = find_homomorphism(G, H, injective=True, surjective=True, compute=False)

This comment has been minimized.

@valglad

valglad Aug 14, 2018

Contributor

There should be a blank line between this one

"defined when the group is a list")
parent_group = G

free_group = parent_group.free_group

This comment has been minimized.

@valglad

valglad Aug 19, 2018

Contributor

I see. Then, perhaps, this line should move into the definition of _check since it's the only reason we have it?


# Return the quotient group with presentation
# <G.generators|q_realtors>
if homomorphism and T:

This comment has been minimized.

@valglad

valglad Aug 19, 2018

Contributor

This is still relevant. The only condition for returning a homomorphism should be homomorphism == True - whether or not T is defined shouldn't matter

@@ -503,12 +505,12 @@ def find_homomorphism(G, H, injective=False, surjective=False, compute=True, all
_H, h_isomorphism = H._to_perm_group()

if injective:
if (h_order % g_order != 0) or not G.is_abelian and H.is_abelian:
if (h_order % g_order != 0) or (not G.is_abelian and H.is_abelian):

This comment has been minimized.

@valglad

valglad Aug 19, 2018

Contributor

Why introduce the brackets? If I'm not mistaken, and takes precedence over or and not applies only to the following term so that the earlier version was parsed as if (h_order % g_order != 0) or ((not G.is_abelian) and H.is_abelian) as it should. There is no harm in keeping the brackets, I just wonder if there was some specific reason for them

This comment has been minimized.

@RavicharanN

RavicharanN Aug 19, 2018

Author Contributor

Just put them there to make it more clear. Could be useful to someone reading the code.

This comment has been minimized.

@valglad

valglad Aug 19, 2018

Contributor

I thought that might be why. I guess that does make it clearer, good idea

H = [x*y**2*x*y, y**2*x*y*x, y**-1]
K, T = subgroup_quotient(G, H, parent_group=f, homomorphism=True)
assert T.domain == K
assert T(K.generators) == list(f.generators)
G = f.subgroup(G)
H = f.subgroup(H)
assert K.order() == G.order()/H.order()

This comment has been minimized.

@valglad

valglad Aug 19, 2018

Contributor

Mabe, it would be good to have some assert statements for T here, just to see that it is properly defined.

Returns
=======
* When `homomorphism = True`, quotient group along with the homomorphism
from the quotient to the parent_group whose image is isomorphic to G.

This comment has been minimized.

@valglad

valglad Aug 19, 2018

Contributor

I've just realised that this doesn't work. In general, there is no well-defined homomorphism from a quotient to the parent group, because each element of the quotient would have to be mapped to multiple elements (everything in the corresponding coset). We can only return a surjective homomorphism from G to the quotient. And if we were defining a homomorphism from parent_group to the quotient instead, it would have to be a partial one because there would be nowhere to send elements outside of G, and that's not possible with homomorphism. The best I can think of would be to return two homomorphisms at once when G is a list: one an injection from G into parent_group (what you get from subgroup()) and the other the surjection from G onto the quotient. What do you think of this?

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