Skip to content
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] Added Polycyclic Group Class #16991

Open
wants to merge 21 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@divyanshu132
Copy link
Member

commented Jun 8, 2019

Brief description of what is fixed or changed

Polycyclic group class has been added and few of the helper methods are also implemented.

Other comments

Release Notes

  • combinatorics
    • added polycyclic group class and methods
@sympy-bot

This comment has been minimized.

Copy link

commented Jun 8, 2019

Hi, I am the SymPy bot (v147). 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:

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

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 . Please also
write a comment on that issue linking back to this pull request once it is
open. -->


#### Brief description of what is fixed or changed
Polycyclic group class has been added and few of the helper methods are also implemented.

#### Other comments


#### 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. The bot will check your release notes
automatically to see if they are formatted correctly. -->

<!-- BEGIN RELEASE NOTES -->
*  combinatorics
   * added polycyclic group class and methods 
<!-- END RELEASE NOTES -->

from sympy import sieve
from sympy.combinatorics.perm_groups import PermutationGroup

class PcGroup(Basic):

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jun 8, 2019

Member

I think PolycyclicGroup sounds better. Why PcGroup? Can you give a short explanation? Just asking out of curiosity.

is_group = True
is_solvable = True

def __init__(self, _pcgs):

This comment has been minimized.

Copy link
@czgdp1807

czgdp1807 Jun 8, 2019

Member

Why not __new__?

@czgdp1807

This comment has been minimized.

Copy link
Member

commented Jun 8, 2019

I think you forgot to add test in test.args. Moreover where are the tests for PcGroup?

@divyanshu132 divyanshu132 changed the title Added Polycyclic Group Class [WIP] Added Polycyclic Group Class Jun 8, 2019

group = word.group
uncollected_subwords = minimal_uncollected_subwords(word, relative_order)
power_relators, conjugate_relators = _relations(pc_relators)
for w, case in uncollected_subwords.items():

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jun 15, 2019

Author Member

@jksuom here after computing with minimal_uncollected_subwords the type of words has been changed to core.mul.

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 15, 2019

Member

It looks like collected_subwords is making use of array form. Perhaps the results should be converted back to group elements.

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jun 15, 2019

Author Member

I've tried but it doesn't work!

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 15, 2019

Member

The rewritingsystem module works with similar transformations converting words to group elements.

for i in range(len(array)-1):
if array[i+1][1] > 0:
# case-1: v = x[i]**a*x[i+1]
uncollected_subwords[array[i][0]**array[i][1]*array[i+1][0]] = 0

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 15, 2019

Member

I think that x[i] and x[i+1] should be group elements.

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jun 15, 2019

Author Member

Do you mean converting them into group element using dtype()?

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 15, 2019

Member

I think that there is a method for the conversion.

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jun 16, 2019

Author Member

I'm not able to find any such method!

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 16, 2019

Member

It seems that I was thinking of letter_form_elm that is used in rewritingsystem. It calls dtype internally but apparently cannot be applied here, at least not easily. So it looks like dtype has to be used more explicitly for the conversion. Anyway, some new code has to be introduced to manage the ordering of the generators.

@codecov

This comment has been minimized.

Copy link

commented Jun 16, 2019

Codecov Report

Merging #16991 into master will increase coverage by 0.53%.
The diff coverage is 62.913%.

@@             Coverage Diff              @@
##            master    #16991      +/-   ##
============================================
+ Coverage   73.959%   74.489%   +0.53%     
============================================
  Files          620       624       +4     
  Lines       160517    161371     +854     
  Branches     37656     37875     +219     
============================================
+ Hits        118718    120205    +1487     
+ Misses       36317     35837     -480     
+ Partials      5482      5329     -153
# case-2: v = x[i]**a*x[i+1]*-1
uncollected_subwords[array[i][0]**array[i][1]*array[i+1][0]**-1] = 0

if all(array[i][1]!=exp for exp in range(relative_order[i])):

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 16, 2019

Member

Does this mean that array[i][1] < 0 or array[i][1] >= relative_order[i]?

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jun 16, 2019

Author Member

for collected word 0 < array[i][1] < relative_order[i] in other cases it will be uncollected.

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 16, 2019

Member

It is very inefficient to compare separately with each number in the range. It will suffice to compare with the endpoints.

for w, v in power_relators.items():
divisor = w.array_form[0][1]
if w.letter_form_elm[0] == word.letter_form_elm[0] and dividend % divisor < rem:
rem = dividend % divisor

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 16, 2019

Member

// and % should not be used with noncommutative group elements.

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jun 16, 2019

Author Member

Can't we use these for exponents of elements?

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 17, 2019

Member

Oh yes. I didn't notice that they were integers.

uncollected_subwords = {}
for i in range(len(array)-1):
if array[i+1][1] > 0:
# case-1: v = x[i]**a*x[i+1]

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 16, 2019

Member

This is uncollected only if the generator x[i+1] comes before x[i] in the polycyclic series. Otherwise it is collected.

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jun 16, 2019

Author Member

I think minimal_uncollected_subwords function is just working on the given word in first 2-cases even they are not using relative_order. From this the word is further reduced with the help of power and conjugate relators.

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 17, 2019

Member

Do you mean that minimal_uncollected_subwords should also return collected subwords? (Those where x[i] comes before x[i+1].)

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jun 17, 2019

Author Member

No it shouldn't, now I realized that it should be changed as you suggested.

divyanshu132 added some commits Jun 20, 2019

Section 8.1.3
"""

def __init__(self, pc_relators, word):

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 20, 2019

Member

It is not efficient to create a different Collector object for each word. It would probably suffice to construct single object for each PolycyclicGroup. That would contain the generators and relators in the power-conjugate form. Variable components, like words, can then be given as parameters like minimal_uncollected_subwords(self, word).

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jun 21, 2019

Author Member

To compute the index we should have group = word.group so, in that case we'll need word or may be group will not be an attribute to the self.

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 21, 2019

Member

The words that are collected are elements of a (fixed) free group. I think that it should be an attribute of Collector.

{((x1, 7),): 1, ((x2, 2), (x1, 1)): 0}
"""
group = self.word.group

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 20, 2019

Member

group could be an attribute of self.

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jun 20, 2019

Author Member

group is an attribute of self but, I think by mistake I've done that!

"""
group = self.word.group
gens = group.symbols
index = {}

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 20, 2019

Member

This index structure is used repeatedly. It is inefficient to create it time and again. It should be an attribute of self.

gens = group.symbols
index = {}
i = 1
for g in gens:

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 20, 2019

Member

This could probably be something like

for i, s in enumerate(group.symbols):
    index[s] = i  
i += 1
array = self.word.array_form
uncollected_subwords = {}
for i in range(len(array)-1):

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 20, 2019

Member

I would use abbreviated notations for the symbol-exponent pairs to make the code more readable. Something like this, for example

s1, e1 = array[i]
s2, e2 = array[i+1]

and then if e2 > 0 and index[s1] > index[s2]: etc.

group = collector.group
flag = 0
while True:
uncollected_subwords = collector.minimal_uncollected_subwords()

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 27, 2019

Member

It would suffice to get one minimal uncollected subword at a time. If there are several, those will be handled on the following rounds of the True loop. It would probably be enough to have the word in array form and its position. Its type and length can be easily found here.

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jun 27, 2019

Author Member

Do you mean just processing a single uncollected subword at a time that is, removing the loop below this?

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 27, 2019

Member

Yes. The True-loop alone should suffice.

@codecov

This comment has been minimized.

Copy link

commented Jun 28, 2019

Codecov Report

Merging #16991 into master will increase coverage by 0.59%.
The diff coverage is 81.623%.

@@             Coverage Diff             @@
##            master   #16991      +/-   ##
===========================================
+ Coverage   73.959%   74.55%   +0.59%     
===========================================
  Files          620      624       +4     
  Lines       160517   161779    +1262     
  Branches     37656    37956     +300     
===========================================
+ Hits        118718   120607    +1889     
+ Misses       36317    35829     -488     
+ Partials      5482     5343     -139
{((x1, 7),): 2, ((x2, 2), (x1, 1)): 0}
"""
group = word.group

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 28, 2019

Member

This could be self.group. (All words should belong to the same group.)

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jun 28, 2019

Author Member

Since now, word is not an attribute to self we can't define self.group = word.group!

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 28, 2019

Member

It should be an attribute of the collector, given as a parameter of __init__. That we are sure that all words belong to the same group.

for i in range(len(array)-1):
s1, e1 = array[i]
s2, e2 = array[i+1]
if e2 > 0 and index[s1] > index[s2]:

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 28, 2019

Member

I would start by checking that e1 is less than relative order and nonnegative.

# case-0: v = x[i]**a*x[i+1], where index[x[i]] > index[x[i+1]]
uncollected_subwords[((s1, e1), (s2, 1))] = 0

elif e2 < 0 and index[s1] > index[s2]:

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 29, 2019

Member

The index condition is the important part. It should be checked first. Then this line and the previous one could be handled together. For example,

if index[s1] > index[s2]:
    e = 1 if e2 > 0 else -1
    return ((s1, e1), (s2, e))

I don't think it would be necessary to created a dict uncollected_subwords when at most one word is returned at a time. Its case can be easily determined afterwards: If the length of the returned tuple is 1, we have a single ((s1, e1),). Otherwise we have ((s1, e1), (s2, e)), and the case can be seen from the sign of e.

@divyanshu132

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2019

Suppose I want to collect for S(3):
Below are the relators and relative_order, let me know if they are wrong!
pc_relators = {x0**2: (), x1**3: (), x0**-1*x1*x0: x1**2}
relative_order = {x0: 2, x1: 3} (I think this may be wrong)
word = x1**-2*x0
Now, the uncollected_word will be x1**-2.

@jksuom

This comment has been minimized.

Copy link
Member

commented Jun 29, 2019

The relative order of x1 is 3. We write -2 = -1*3 + 1 and replace x1**-2 by x1**1*(x1**3)**-1 = x1 since x1**3 = 1.

Example 8.7 Pg. 281 from [1]
>>> F, x1, x2 = free_group("x1, x2")
>>> pc_relators = {x1**2 : (), x1*x2*x1**-1 : x2**-1, x1**-1*x2*x1 : x2**-1}
>>> relative_order = {x1: 2, x2: 3}
>>> relative_order = {x1: 2, x2: S.Infinity}

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 30, 2019

Member

I think that we should avoid importing S.Infinity. It would suffice to just omit x2 from the relative_order dict.

# case-2: v = x[i]**a
uncollected_subwords[((s1, e1), )] = 2
continue
if (e1 < 1 or e1 > re[s]-1) and re[s] != S.Infinity:

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 30, 2019

Member

Is it possible that e1 == 0? If not, then I would write e1 < 0. So this could be

if s in re and (e1 < 0 or e1 >= re[s]):

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jun 30, 2019

Author Member

Is it possible that e1 == 0?

No!

uncollected_subwords[((s1, e1), )] = 2

return uncollected_subwords
if (e1 < 1 or e1 > re[s]-1) and re[s] != S.Infinity:

This comment has been minimized.

Copy link
@jksuom

jksuom Jun 30, 2019

Member

Here as well: if s in re and ....

return None

group = self.group
index = {s: i+1 for i, s in enumerate(group.symbols)}

This comment has been minimized.

Copy link
@jksuom

jksuom Jul 1, 2019

Member

Why i+1 instead of i?

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jul 1, 2019

Author Member

Initially, we thought of treating index as relative order that's why I kept this, Now this can be changed!

group = self.group
index = {s: i+1 for i, s in enumerate(group.symbols)}
array = word.array_form
re = self.relative_order

This comment has been minimized.

Copy link
@jksuom

jksuom Jul 1, 2019

Member

Could this be a list? For instance, [2, 3, 2, 2] for S(4).

This comment has been minimized.

Copy link
@jksuom

jksuom Jul 1, 2019

Member

Maybe 0 or None could be used to denote a missing power relation ("infinite relative order").

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jul 1, 2019

Author Member

Yes, we can do that!

"""
group = w.group
gens = list(sorted(w.contains_generators()))

This comment has been minimized.

Copy link
@jksuom

jksuom Jul 4, 2019

Member

Is list needed? sorted should return a list.

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jul 4, 2019

Author Member

Yes, I should remove this!

return low, high

def map_relation(self, w):
"""

This comment has been minimized.

Copy link
@jksuom

jksuom Jul 4, 2019

Member

Some kind of explanation is expected in the docstring.

self.pc_series = self._pc_series()
self.pcgs = self._compute_pcgs()
def __init__(self, pc_sequence, pc_series):
self.pc_series = pc_series

This comment has been minimized.

Copy link
@jksuom

jksuom Jul 5, 2019

Member

I think that the groups in the polycyclic series should be computed from the generator sequence: <x0, ..., xn>, <x1, ...xn>, ..., <xn>, 1. That would guarantee that generator the sequence and the polycyclic series are compatible. (There are many alternatives for both the sequence and the series.)

divyanshu132 added some commits Jul 5, 2019

# G = self.pc_series[i]
# H = self.pc_series[i+1]
# rel_orders.append(G.order()//H.order())
rel_orders = [g.order() for g in self.pcgs]

This comment has been minimized.

Copy link
@jksuom

jksuom Jul 5, 2019

Member

These are not relative orders.

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jul 5, 2019

Author Member

Yes, but I think for the current tests that we've added they will be equal to the relative order!

>>> pc_group = G.polycyclic_group()
>>> group = F
>>> pc_group.power_relations(group)
[x1**3, x0**2]

This comment has been minimized.

Copy link
@jksuom

jksuom Jul 10, 2019

Member

These are the lhs expressions of power relations. There should also be the rhs expressions in normal form.

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jul 10, 2019

Author Member

We are only computing lhs in both the methods(this one and below) which can be directly feed to the pc_presentation where rhs is computed may be we can make both the methods private with different method names something like conjugate_lhs and power_lhs.
This is just to reduce the number of lines in method pc_presentation!

>>> pc_group = G.polycyclic_group()
>>> group = F
>>> pc_group.conjugate_relations(group)
[x2**-1*x3*x2, x1**-1*x3*x1, x1**-1*x2*x1, x0**-1*x3*x0,

This comment has been minimized.

Copy link
@jksuom

jksuom Jul 10, 2019

Member

Also here we only have the lhs expressions of the relations, not the actual relations.

rel_orders.append(G.order() // H.order())
return rel_orders

def is_prime_order(self):

This comment has been minimized.

Copy link
@jksuom

jksuom Jul 10, 2019

Member

Where is this method needed?

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jul 10, 2019

Author Member

This is not used anywhere, this is a separate method for polycyclic groups.


def is_prime_order(self):
for order in self.relative_order():
if order not in sieve:

This comment has been minimized.

Copy link
@jksuom

jksuom Jul 10, 2019

Member

How does isprime compare with sieve (like return all(isprime(order) for order in self.relative_order()))?

This comment has been minimized.

Copy link
@divyanshu132

divyanshu132 Jul 11, 2019

Author Member

For, greater length of relative_order isprime seems a good option.

word = word*perm_to_free[gens]
collector = Collector(pc_relators, self.relative_order(), group)

word = collector.collected_word(word)

This comment has been minimized.

Copy link
@jksuom

jksuom Jul 11, 2019

Member

Conjugate relations are not yet available in pc_relators. There is no guarantee that the collected word is correct.

divyanshu132 added some commits Jul 13, 2019

@divyanshu132

This comment has been minimized.

Copy link
Member Author

commented Jul 16, 2019

ping @jksuom

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.