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

[WIP] Add methods for Polycyclic Groups #14879

Open
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@RavicharanN
Copy link
Contributor

RavicharanN commented Jul 7, 2018

References to other Issues or PRs

Brief description of what is fixed or changed

Currently, a few methods are defined to compute the polycyclic series for a solvable FpGroup.

To-Do

  • Compute pc-presentation.
  • Add tests and improve documentation.

Release Notes

  • combinatorics
    • Methods have been implemented for the Polycyclic groups and presentations

@RavicharanN RavicharanN changed the title Add methods for Polycyclic Groups [WIP] Add methods for Polycyclic Groups Jul 7, 2018

@Abdullahjavednesar Abdullahjavednesar requested a review from valglad Jul 8, 2018

_next_order = _next.order()
subnormal_series.append(_next)

return set(subnormal_series)

This comment has been minimized.

@valglad

valglad Jul 8, 2018

Contributor

Why return a set? The order in which subgroups appear in the series is useful information.

"implemented for FpGroups")
# Compute the derived series.
derived_series = self.derived_series()
derived_series.insert(0, self)

This comment has been minimized.

@valglad

valglad Jul 8, 2018

Contributor

You could just do derived_series = [self] + self.derived_series(). Though, the rest of derived_series doesn't contain FpGroups so it would make more sense to say derived_series = [self.generators] + self.derived_series()

This comment has been minimized.

@RavicharanN

RavicharanN Jul 9, 2018

Author Contributor

so it would make more sense to say derived_series = [self.generators] + self.derived_series()

Wouldn't it be better to leave a few groups as FpGroup in the subnormal series wherever possible? Because that way, we could avoid the computation of presentation at least for some groups when we try to find the relative orders of the polycyclic group.

This comment has been minimized.

@valglad

valglad Jul 10, 2018

Contributor

I mean, the original group will have a trivial coset table so probably wouldn't take long anyway, but I see your point. Could you perhaps find all the necessary subgroup presentations here, before adding the original group to them?

'''
if not self.is_polycyclic:
raise NotImplementedError("Polycyclic methods are only"
"implemented for FpGroups")

This comment has been minimized.

@valglad

valglad Jul 8, 2018

Contributor

That error message doesn't match the reason it is being raised. The reason is that the group is not known to be polycyclic. So a better message, perhaps, would be No methods currently implemented to check if the given group is polycyclic or something along those lines.

def subgroup_quotient(G, H):
'''
Compute the quotient group G/H
where, H is the subgroup of G.

This comment has been minimized.

@valglad

valglad Jul 8, 2018

Contributor

There shouldn't be a comma after where.

This comment has been minimized.

@valglad

valglad Jul 14, 2018

Contributor

There still shouldn't be a comma. Also, the docstring should explain what the arguments are for. I think that G and H should come as the first 2 arguments and the other things should be keyword arguments. Is free_group ever different from fp_grp.free_group?

'''
if not (isinstance(G, FpGroup)
and isinstance(H, FpGroup)):
raise ValueError("The group must be an instance of FpGroup")

This comment has been minimized.

@valglad

valglad Jul 8, 2018

Contributor

But when you use this method in compute_polycyclic_series, you don't currently make H into an instane of FpGroup and you make G an instance of FpSubgroup.

# Return a subnormal series.
_subnormal_series = _subnormal_series(q_grp)
# The images of every subgroup in the quotient map
# can be lifted to the orginal group

This comment has been minimized.

@valglad

valglad Jul 8, 2018

Contributor

Should be Every subgroup in _subnormal_series and original.

h_rels = _homomorphism(h_rels)
else:
h_gens = H.generators
h_rels = H.relators

This comment has been minimized.

@valglad

valglad Jul 15, 2018

Contributor

These two if statements are almost identical - they can be a subroutine that you first call on H and then on G

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

Still relevant

"of FpGroup")

if isinstance(G, list):
g_gens, g_rels, _gens = reidemeister_presentation(fp_group, G, rel=True)

This comment has been minimized.

@valglad

valglad Jul 15, 2018

Contributor

I think it would make more sense if this returned _homomorphism as a third parameter instead of _gens.

_next = _parent.subgroup(elem)
# Injective homomorphism between `_next` which is defined
# on another free_group and the parent group.
_homomorphism = homomorphism(_next, _parent, _next.generators[0], _parent.generaotrs[0], check=False)

This comment has been minimized.

@valglad

valglad Jul 15, 2018

Contributor

typo: generators. But beside that, I don't think this defines the injective homomorphism we want, in general.

This comment has been minimized.

@RavicharanN

RavicharanN Jul 15, 2018

Author Contributor

Since we are computing the presentation using one element of the generators, will that be changed? I meant, for example, if x, y are the generators of G and we compute the presentation of subgroup generated by x, will x1 not correspond to x here? Or is it still gonna change?

This comment has been minimized.

@valglad

valglad Jul 15, 2018

Contributor

Oh right, it's a cyclic group and we know what it's generated by. In that case this should be fine.

# on another free_group and the parent group.
_homomorphism = homomorphism(_next, _parent, _next.generators[0], _parent.generaotrs[0], check=False)
new_relators = _homomorphism(list(_next.relators))
_next = FpGroup(free_group, new_relators)

This comment has been minimized.

@valglad

valglad Jul 15, 2018

Contributor

free_group isn't defined in this function yet

def __init__(fp_group, pc_series=None, pcgs=None):
self.fp_group = fp_group
self.pc_series = pc_series
self.pcgs = pcgs

This comment has been minimized.

@valglad

valglad Jul 17, 2018

Contributor

That's not a very enlightening name. pc_sequence or pc_gens would be better.

pcgs = []
for i in range(0, len(self.pc_series) - 1):
quotient = subgroup_quotient(self.fp_grp.free_group, self.fp_grp,self.pc_series[i], self.pc_series[i+1])
pcgs.append(quotient.random())

This comment has been minimized.

@valglad

valglad Jul 17, 2018

Contributor

Careful: a random element of a cyclic group is not necessarily its generator. For a group of prime order, everything except the identity is in fact a generator but even then you'd need to check that it's not the identity.

In addition to this, will quotient.random() return an element of the main group that corresponds to the right generator?

This comment has been minimized.

@RavicharanN

RavicharanN Jul 17, 2018

Author Contributor

In addition to this, will quotient.random() return an element of the main group that corresponds to the right generator?

The subgroup_quotient method returns an FpGroup which is defined on the generators(or the subset of the generators) of the main group. So, I guess it should.

This comment has been minimized.

@valglad

valglad Jul 17, 2018

Contributor

Simply being defined on the same generators isn't enough. For example,

F, a, b = free_group("a b")
G = FpGroup(F, [a**2, b**3])
H = FpGroup(F, [b**2, a**3])

G and H are defined on the same generators and they are even isomorphic but a had order 2 in G and order 3 in H so they represent different elements.

rel_orders[i] = first_grp.order()/sec_grp.order()
self.rel_orders = rel_orders

def is_prime_order(self):

This comment has been minimized.

@valglad

valglad Jul 17, 2018

Contributor

There should be a function is_prime somewhere in the number theory module.

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

This is still relevant.

To-do : Docstring completion.
Add error and exception handling
'''
def __init__(fp_group, pc_series=None, pcgs=None):

This comment has been minimized.

@valglad

valglad Jul 17, 2018

Contributor

I think that the definition should be different. I would expect something like a method pc_presentation in the FpGroup class that checks if the group is polycyclic first, then computes pc_series and only then returns a PcGroup presentation. And a polycyclic presentation should have different generators and relators from the original group anyway, so perhaps __init__ could just take in pc_series as an argument, compute the polycyclic sequence to use as generators, compute the right relators, and only then return an instance of PcGroup.

Another possibility is doing what's described in the Handbook in Definition 8.7, when a polycyclic presentation is first defined. You could take in S as an argument and build up a polycyclic group from that. This would also mean that you could test your polycyclic methods before you sort out obtaining a polycyclic series for an FpGroup.

This comment has been minimized.

@RavicharanN

RavicharanN Jul 17, 2018

Author Contributor

perhaps init could just take in pc_series as an argument, compute the polycyclic sequence to use as generators, compute the right relators, and only then return an instance of PcGroup.

Well, I haven't pushed it yet, but I've changed the definition a bit. Currently, I've written it as __init__(self, free_group, pc_series=None, pcgs=None, relations=None). I've set pc_series, pcgs, relations as default arguments because the user could either choose to specify the pc_series directly or he could define a presenation. And the free_group argument would be needed anyway.

This comment has been minimized.

@valglad

valglad Jul 17, 2018

Contributor

Why would you need free_group as an argument? You can just build one that has the same length as the given pc_series, and this is what you'd have to do if free_group is the wrong length anyway. And what would happen if the user just specified a free_group and none of the other arguments? It seems like there could be problems with it.

Another issue with letting a user specify a presentation is that they could specify something that isn't actually polycyclic. While if we enforced that presentations have to be specified with S, then we never run into that problem

first_grp = self.pc_series[i]
sec_grp = self.pc_series[i+1]
if isinstance(self.pc_series[i], list):
first_grp = fp_group.subgroup(self.pc_series[i])

This comment has been minimized.

@valglad

valglad Jul 17, 2018

Contributor

I would expect pc_series to contain groups, not lists. And it does contain lists, then at least there should another attribute containing the relevant groups - they are computed during compute_polycyclic_series and it would be very inefficient to compute them again.

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

Any thoughts about this?

This comment has been minimized.

@RavicharanN

RavicharanN Jul 20, 2018

Author Contributor

Maybe we could store it as a dictionary with values as the presentation of the subgroup on new generators. I mean, the keys would be the elements of the current pc_gens and values could be the FpGroups on a new set of generators.

This comment has been minimized.

@valglad

valglad Jul 21, 2018

Contributor

That could work. I think it will depend on what's going to be most convenient to use, and you might need to write a bit more code to work out the best thing.

first_grp = fp_group.subgroup(self.pc_series[i])
if isinstance(self.pc_series[i+1], list):
sec_grp = fp_group.subgroup(self.pc_series[i+1])
rel_orders[i] = first_grp.order()/sec_grp.order()

This comment has been minimized.

@valglad

valglad Jul 17, 2018

Contributor

This should be rel_orders.append(first_grp.order()/sec_grp.order())

# 1. Documentation
# 2. Following methods.

def compute_presentation(self):

This comment has been minimized.

@valglad

valglad Jul 17, 2018

Contributor

A presentation would need to be computed at creation, and you could just use two methods, one for generators, one for relators there, so not much need for this method. A more useful method to have would be refined_presentation.

'''
return

def collection_algo(self):

This comment has been minimized.

@valglad

valglad Jul 17, 2018

Contributor

It should be called something like collected_word - though I guess this is just a draft name rather than one you'd actually propose.

gens, rels = simplify_presentation(gens, rels, change_gens=True)

C.schreier_generators = tuple(gens)
C.reidemeister_relators = tuple(rels)
if rel:

This comment has been minimized.

@valglad

valglad Jul 17, 2018

Contributor

As I said somewhere above, it would make more sense to return a homomorphism rather than a list of generators. And in any case, rel looks to much like short for relator which is confusing. This can be homomorphism if you return one.

lh_suffix = C.coset_representative(beta)
for i in range(1, len(rh)):
if rh.subword(i, len(rh)) == lh_suffix:
_gens.append(rh.subword(0, i))

This comment has been minimized.

@valglad

valglad Jul 17, 2018

Contributor

You could just define the transversal as you go in some dictionary tau, much like what's done in the description of DefineScreierGenerators in the Handbook. Initially you set tau[1] = f.identity, then tau[beta] = tau[alpha]*x when beta=gamma and at this stage you can just do _gens.append(tau(alpha)*x*tau(beta)**-1)

This comment has been minimized.

@RavicharanN

RavicharanN Jul 17, 2018

Author Contributor

Yeah, I've changed it. I just haven't pushed that yet. Also, can we reduce the word after tau(alpha)*x*tau(beta)**-1 is computed?

This comment has been minimized.

@valglad

valglad Jul 17, 2018

Contributor

Yes, of course, it'd be the same word as far as our group is concerned. Just check that the reduction time doesn't outweigh the benefits of having a shorter image.

@RavicharanN RavicharanN force-pushed the RavicharanN:polycyclic branch from ba8d2f2 to 0cf08ba Jul 19, 2018

# compute the schreier Traversal
traversal = []
for alpha in C.omega:
traversal.append(C.coset_representative(alpha))

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

This does compute some coset representatives but do we know that they are the right ones? Sticking with defining traversal as the function runs, as I explained in my last comment on this, should be much more reliable.

C.schreier_gen_elem = tuple(C._schreier_gen_elem)
return C.schreier_generators, C.reidemeister_relators, C.schreier_gen_elem
if homomorphism:
G = C.fp_group

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

This is already defined as fp_grp

if homomorphism:
G = C.fp_group
if gens:
_G = FpGroup(g[0].group, rels)

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

This should be the same as what's returned by fp_grp.subgroup(H). Hmm. Perhaps, it would make sense to postpone homomorphism construction until then, and here just return the dictionary {gen: _schreier_gen_elem[str(gen)]}. Then you'd need to add a homomorphism keyword to subgroups, and also add some examples there (btw, this should be possible to test now; does it return a valid isomorphism in all cases?).

first_grp = self.pc_series[i]
sec_grp = self.pc_series[i+1]
if isinstance(self.pc_series[i], list):
first_grp = fp_group.subgroup(self.pc_series[i])

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

Any thoughts about this?

rel_orders[i] = first_grp.order()/sec_grp.order()
self.rel_orders = rel_orders

def is_prime_order(self):

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

This is still relevant.

@@ -544,6 +544,12 @@ def compute_polycyclic_series(self):

return pc_series

def pc_group(self):
'''Returns a the corresponding Pc-Group'''

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

"Return a polycyclic presentation of the group, if it exists" - remember the docstring convention: it's always return or compute, not returns or computes.

rel -- When set to True, this returns a relation between subgroup generators
and the elements of the parent group.
homomorphism -- When set to True, this returns a relation between subgroup generators
and the elements of the parent group.

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

"A relation" is too ambiguous. What sort of a relation? Also, saying "return" rather than "this returns" would be more consistent with docstring conventions (even if this isn't the first line).

So maybe "Return a dictionary containing the images of the presentation generators in the original group"

to which the shreier generators corresponf to.
homomorphism: When set to True, this computes the elements in the `fp_grp`
to which the shreier generators correspond to and returns a
homomorphism between the presentation and the parent group.

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

Same comment as for define_schreier_generators - in fact the same docstring would work if we are returning a dictionary.

@@ -494,6 +496,120 @@ def elements(self):
P, T = self._to_perm_group()
return T.invert(P._elements)

@property
def is_polycyclic(self):
if self.is_solvable:

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

Not all solvable groups are polycyclic - finite solvable groups are though.


def is_subgroup(self, other):
"""
Return ``True`` if all elements of ``self`` belong to ``other``.

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

This is a misleading docstring, because for two FpGroups on differently-sized FreeGroups this is never true. A more correct statement would be "Check if self contains an isormorphic copy of other". But the method you are using won't necessarily be able to determine that. other._to_perm_group() could theoretically return a group of permutations that are not part of self._to_perm_group()

_other = other._to_perm_group()[0]
return _grp1.is_subgroup(_other, strict=False)

def compute_polycyclic_series(self):

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

Simply polycylic_series would be more consistent with how other similar methods are named.

q_group = FpGroup(free_group, q_relators)

# Return the quotient group with presentation
# <G.generators|q_realtors>

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

typo: relators. And it would make more sense for this comment to go before the preceding two lines of code


def _subnormal_series(G):
'''
Computes the subnormal series for an abelian group.

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

"Compute" and "a subnormal series". But then, this isn't just any subnormal series - it's a subnormal series with cyclic factor groups. Maybe the whole method should be called _cyclic_subnormal_series

def subgroup_quotient(free_group, fp_grp, G, H):
'''
Compute the quotient group G/H
where, H is the subgroup of G.

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

There is no need for the comma after where

Compute the quotient group G/H
where, H is the subgroup of G.
'''
if not isinstance(fp_group, FpGroup):

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

fp_group isn't defined - your argument is called fp_grp.

# Consistent by default
return PcGroup(polycyclic_series)

def subgroup_quotient(free_group, fp_grp, G, H):

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

You don't need to pass free_group - it can be extracted from fp_grp.

if isinstance(G, list):
g_gens, g_rels, _gens = reidemeister_presentation(fp_group, G, rel=True)
_homomorphism = homomorphism(G, fp_group, g_gens, _gens, check=False)
g_gens = _gens

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

This needs to be changed in accordance with what reidemeister_presentation returns now

raise ValueError("Subword not found")

def _find_relation(w, type):
'''Find the the approproate relation'''

This comment has been minimized.

@valglad

valglad Jul 20, 2018

Contributor

"appropriate"

@sympy-bot

This comment has been minimized.

Copy link

sympy-bot commented Jul 29, 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. -->

#### References to other Issues or PRs
<!-- 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
Currently, a few methods are defined to compute the polycyclic series for a solvable `FpGroup`.

#### To-Do 
* Compute pc-presentation.
* 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 
   * Methods have been implemented for the Polycyclic groups and presentations
<!-- END RELEASE NOTES -->

Your release notes are in good order.

Here is what the release notes will look like:

  • combinatorics
    • Methods have been implemented for the Polycyclic groups and presentations (#14879 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.

@@ -114,7 +142,7 @@ def _find_index_range(self, elem, w):
raise ValueError("Subword not found")

def _find_relation(w, type):
'''Find the the approproate relation'''
'''Find the approproate relation'''

This comment has been minimized.

@valglad

valglad Jul 31, 2018

Contributor

"appropriate"

This is useful in the implementation of the collection
algorithm where a word is converted to a form
where each element in the word is a distince generator.

This comment has been minimized.

@valglad

valglad Jul 31, 2018

Contributor

It would make more sense to do this sort of thing in compute_pcgs

The implementation for this methods will be similar to
the coin change problem in DP.
'''
def __init__(self, pc_series, power_exponents=None):

This comment has been minimized.

@valglad

valglad Jul 31, 2018

Contributor

A user should be submitting either pc_series or power_exponents, not both at the same time. It makes more sense to take some arg, work out if it specifies pc_series or power_exponents, then proceed based on that. I still think you should implement pc presentation from power_exponents as soon as possible because then you could be testing all polycyclic methods you are adding. Computing polycyclic series is hard and will take time - I'm actually wondering if it should be a separate PR. But computing a presentation given power exponents is easy

for i in range(0, len_pc_gens):
dict[x[i]] = self.pc_gens[i]

return F, x, dict

This comment has been minimized.

@valglad

valglad Jul 31, 2018

Contributor

dict is a reserved python keyword, best to rename it to something else

fr_grp_str = fr_grp_str[:-2]

# Define a few group on a new set of generators.
free_group = free_group(fr_grp_str)

This comment has been minimized.

@valglad

valglad Jul 31, 2018

Contributor

Or just free_group(",".join(["x_"+str(i) for i in range(3)]). Why only 3 generators? And I'm not entirely convinced this method is necessary at all. Why can't we do without?

This comment has been minimized.

@RavicharanN

RavicharanN Jul 31, 2018

Author Contributor

That shouldn't be 3. It should be equal to the length of pc_gens.

This method would be useful to map the pc_gens to the generators of another group. Basically, the pc_gens defined on a FreeGroup F could be of the form x**2 or x*y. This could be a problem during the implementation of the collection algorithm as the collection algorithm is implemented on a basis that the pc_gens are of the form x1, x2...etc(individual entities). So, we define a new FreeGroup on generators x1, x2...etc and map them to the pc_gens, in this case, x1 = x**2 and x2 = x*y.

Now the all the elements in the power presentation and the uncollected subword will be wriiten in the form of new generators. For, example a word x**5*y would be written as x1**2*x2.

This comment has been minimized.

@valglad

valglad Jul 31, 2018

Contributor

Yes, so you see, I think that a new FreeGroup should be created when you compute pc_gens. After you take in a polycyclic series, you should compute a group presentation on pc_gens, and once the group is created all words should be in terms of these new generators so it won't be a problem.

This new class is a subclass of FpGroup and an FpGroup must have generators and relators set at creation

def compute_pcgs(self):
pcgs = []
for i in range(0, len(self.pc_series) - 1):
quotient = subgroup_quotient(self.fp_grp.free_group, self.fp_grp,self.pc_series[i], self.pc_series[i+1])

This comment has been minimized.

@valglad

valglad Jul 31, 2018

Contributor

Computing the quotients takes time, and we already did this when computing the polycyclic series. We should store and use them if possible.

rel_orders.append(first_grp.order()/sec_grp.order())
self.rel_orders = rel_orders

def is_prime_order(self):

This comment has been minimized.

@valglad

valglad Jul 31, 2018

Contributor

is_prime is a standard method in the number theory module, I think I've mentioned this already


return subword_dict

def _find_index_range(self, elem, w):

This comment has been minimized.

@valglad

valglad Jul 31, 2018

Contributor

So, have you checked there isn't a method that does this?

rel = elem_2**(elem_2.arr_form[0][1])*elem_1*elem_2
if rel in self.conjugate_relations:
return rel
return None

This comment has been minimized.

@valglad

valglad Jul 31, 2018

Contributor

This return statement isn't necessary

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