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 methods for isomorphism computation #14861
Conversation
sympy/combinatorics/homomorphisms.py
Outdated
gens.append(g_isomorphism.invert(g)) | ||
else: | ||
gens = G.generators | ||
T = homomorphism(G, H, gens, images, check=False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be neater to just call homomorphism
directly with _H
as the co-domain and check=True
and catching the error if it's not well-defined, then checking if that is an isomorphism, and if it is, defining another one in case H
is actually an FpGroup
.
sympy/combinatorics/homomorphisms.py
Outdated
else: | ||
gens = G.generators | ||
T = homomorphism(G, H, gens, images, check=False) | ||
if T.is_injective() and T.is_surjective(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
T.is_isomorphism()
does this.
sympy/combinatorics/homomorphisms.py
Outdated
if (g_order in sieve) and (h_order in sieve): | ||
if g_order == h_order: | ||
return True | ||
return False |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are still not using the condition with Euler's totient function. Is it tricky to implement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will update that
sympy/combinatorics/homomorphisms.py
Outdated
# Two infinite FpGroups with the same generators are isomorphic | ||
# when the relators are same but are ordered differently. | ||
if G.generators == H.generators and (G.relators).sort() == (H.relators).sort(): | ||
return homomorphism(G, H, G.generators, H.generators) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This output is not of a valid form for either value of compute
, this should be preceded by a boolean.
sympy/combinatorics/homomorphisms.py
Outdated
if not isomorphism: | ||
n = g_order | ||
if (g_order == h_order) and (igcd(n, totient(n))) == 1: | ||
return True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be useful to leave a comment explaining that in this case the group is cyclic.
sympy/combinatorics/homomorphisms.py
Outdated
gens = [] | ||
if isinstance(H, FpGroup): | ||
for s in subset: | ||
images.append(h_isomorphism.invert(s)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
images = h_isomorphism.invert(subset)
should do it, provided subset
is a list.
sympy/combinatorics/homomorphisms.py
Outdated
|
||
def group_isomorphism(G, H, isomorphism=False): | ||
''' | ||
Compute an isomorphism between 2 given groups. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is how we describe it, then isomorphism
should be True
by default. Possibly worth actually having a little function like
def is_isomorphic(G, H):
return group_isomorphism(G, H, isomorphism=False)
just because that would be shorter to use.
sympy/combinatorics/homomorphisms.py
Outdated
if not isinstance(G, (PermutationGroup, FpGroup)): | ||
raise TypeError("The group must be a PermutationGroup or a finite FpGroup") | ||
if not isinstance(H, (PermutationGroup, FpGroup)): | ||
raise TypeError("The group must be a PermutationGroup or a finite FpGroup") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't have to be finite anymore because we can do some things with infinite groups!
sympy/combinatorics/homomorphisms.py
Outdated
return True | ||
return (True, homomorphism(G, H, G.generators, H.generators)) | ||
|
||
# `_H` is the permutation groups isomorphic to `H`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
group
sympy/combinatorics/homomorphisms.py
Outdated
raise NotImplementedError("Isomorphism methods are not implemented for infinite groups.") | ||
_H, h_isomorphism = H._to_perm_group() | ||
|
||
if (g_order != h_order) and (G.is_abelian != H.is_abelian): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be an or
condition.
sympy/combinatorics/homomorphisms.py
Outdated
|
||
if not isomorphism: | ||
# Two groups of same cyclic numbered order | ||
# are ismorphic to each other. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the same" and "isomorphic"
sympy/combinatorics/homomorphisms.py
Outdated
if (g_order == h_order) and (igcd(n, totient(n))) == 1: | ||
return True | ||
|
||
# Match the generators of `_G` with the subsets of `_H` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
G
and just subsets
, without the article.
sympy/combinatorics/homomorphisms.py
Outdated
# Match the generators of `_G` with the subsets of `_H` | ||
gens = G.generators | ||
for subset in itertools.permutations(_H, len(gens)): | ||
gens = list(gens) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do this outside the loop when you first define gens
sympy/combinatorics/homomorphisms.py
Outdated
gens = list(gens) | ||
images = list(subset) | ||
images.extend([_H.identity]*(len(G.generators)-len(images))) | ||
gens.extend([g for g in G.generators if g not in gens]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gens == list(G.generators)
so surely the list comprehension will always be empty.
sympy/combinatorics/homomorphisms.py
Outdated
gens.extend([g for g in G.generators if g not in gens]) | ||
_images = dict(zip(gens,images)) | ||
if _check_homomorphism(G, _H, _images): | ||
images = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to define this.
sympy/combinatorics/homomorphisms.py
Outdated
|
||
# `_H` is the permutation groups isomorphic to `H`. | ||
_H = H | ||
h_isomorphism = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need to define h_isomorphism
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think static scoping might cause problems if this is not defined. If h_isomorphism
is initialized inside that if
block, I think that variable would be local to that scope.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An if
statement doesn't usually count as a new scope - you'd need something major for that, like a function inside a function. You could try it with a simple script but I'm certain it works in Python.
sympy/combinatorics/homomorphisms.py
Outdated
if _check_homomorphism(G, _H, _images): | ||
images = [] | ||
if isinstance(H, FpGroup): | ||
images = h_isomorphism.invert(list(subset)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be .invert(images)
.
sympy/combinatorics/homomorphisms.py
Outdated
if isinstance(H, FpGroup): | ||
images = h_isomorphism.invert(list(subset)) | ||
else: | ||
images = subset |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You don't need this line at all if you don't rewrite images
with []
as you do before the if
.
sympy/combinatorics/homomorphisms.py
Outdated
# It is a valid isomorphism | ||
if not isomorphism: | ||
return True | ||
else: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for this else
.
P = PermutationGroup(p) | ||
assert not is_isomorphic(D, P) | ||
|
||
# Cyclic Groups of different prime order are not isomorphic to each other. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No groups of different order are isomorphic to each other - you don't really need the comment there, I think the test is clear as it is.
B = CyclicGroup(7) | ||
assert not is_isomorphic(A, B) | ||
|
||
# Two groups of same prime order are isomorphic to each other. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"the same"
This is mostly good now - there are some typos and redundant lines but once that's fixed, it should be good to merge. |
I think this is ready |
Brief description of what is fixed or changed
A
group_isomorphism
method has been added to compute the isomorphism between 2 groups. Corresponding tests have also been added.Other comments
The isomorphism methods are currently implemented for finite groups and only a few special cases of infinite groups.
To-do