``Low Index Subgroups`` algorithm implementation in SymPy #11231

Merged
merged 18 commits into from Jun 22, 2016

Projects

None yet

2 participants

@gxyd
Member
gxyd commented Jun 12, 2016 edited

Implements the "Low Index Subgroups" algorithm.

@gxyd gxyd changed the title from [WIP] Implementation low index subgroups to [WIP] ``Low Index Subgroups`` algorithm implementation in SymPy Jun 12, 2016
@jksuom jksuom commented on an outdated diff Jun 13, 2016
sympy/combinatorics/fp_groups.py
+ # 2. alpha in omega (set of live cosets), w in R (relators)
+ A_dict = self.A_dict
+ A_dict_inv = self.A_dict_inv
+ f = alpha
+ i = 0
+ r = len(word)
+ b = alpha
+ j = r - 1
+ # list of union of generators and their inverses
+ while i <= j and self.table[f][A_dict[word[i]]] is not None:
+ f = self.table[f][A_dict[word[i]]]
+ i += 1
+ if i > j:
+ if f != b:
+ return False
+ return True
@jksuom
jksuom Jun 13, 2016 Member

Maybe return f == b.

@jksuom jksuom commented on an outdated diff Jun 13, 2016
sympy/combinatorics/fp_groups.py
+ # Pg. 166
+ def process_deductions_check(self, R_c_x, R_c_x_inv):
+ """
+ A variation of "process_deductions", this calls "scan_check" wherever
+ "process_deductions" calls "scan".
+ """
+ p = self.p
+ while len(self.deduction_stack) > 0:
+ if len(self.deduction_stack) >= max_stack_size:
+ self.lookahead()
+ del self.deduction_stack[:]
+ else:
+ alpha, x = self.deduction_stack.pop()
+ if p[alpha] == alpha:
+ for w in R_c_x:
+ self.scan_check(alpha, w)
@jksuom
jksuom Jun 13, 2016 Member

We should probably have something like if not self.scan_check(alpha, w): return False.
It also seems that the next line will not be needed since scan_check does not modify p.

@jksuom jksuom commented on an outdated diff Jun 13, 2016
sympy/combinatorics/fp_groups.py
+ self.lookahead()
+ del self.deduction_stack[:]
+ else:
+ alpha, x = self.deduction_stack.pop()
+ if p[alpha] == alpha:
+ for w in R_c_x:
+ self.scan_check(alpha, w)
+ if p[alpha] < alpha:
+ break
+ beta = self.table[alpha][self.A_dict[x]]
+ if beta is not None and p[beta] == beta:
+ for w in R_c_x_inv:
+ self.scan_check(beta, w)
+ if p[beta] < beta:
+ break
+
@jksuom
jksuom Jun 13, 2016 Member

If the loop ends successfully with empty stack, we should return True.

@jksuom jksuom commented on an outdated diff Jun 16, 2016
sympy/combinatorics/fp_groups.py
def is_complete(self):
- return not None in flatten(self.table)
+ """
+ The coset table is called complete if it has no undefined entries
+ on the live cosets; that is, whether α^x is defined for all
@jksuom
jksuom Jun 16, 2016 Member

'whether' could be omitted.

@jksuom jksuom commented on an outdated diff Jun 16, 2016
sympy/combinatorics/fp_groups.py
+ r = len(word)
+ b = alpha
+ j = r - 1
+ while i <= j and self.table[f][A_dict[word[i]]] is not None:
+ f = self.table[f][A_dict[word[i]]]
+ i += 1
+ if i > j:
+ return f == b
+ while j >= i and self.table[b][A_dict_inv[word[j]]] is not None:
+ b = self.table[b][A_dict_inv[word[j]]]
+ j -= 1
+ if j < i:
+ # we have an incorrect completed scan with coincidence f ~ b
+ # return False, instead of calling coincidence routine
+ return False
+ else:
@jksuom
jksuom Jun 16, 2016 Member

This is the place where we can profitably extend the table by processing a deduction if i == j. That will then effectively complete the scan by 'joining' the forwards and backwards scans.

@jksuom jksuom commented on an outdated diff Jun 16, 2016
sympy/combinatorics/fp_groups.py
+ [[1, 1, 2, 3],
+ [0, 0, None, None],
+ [None, None, None, 0],
+ [None, None, 0, None]]
+ >>> C232.scan(2, y**3)
+ >>> C232.table
+ [[1, 1, 2, 3], [0, 0, None, None], [None, None, 3, 0], [None, None, 0, 2]]
+ >>> C232.table[1][2] = 1; C232.table[1][3] = 1
+ >>> C232.table
+ [[1, 1, 2, 3], [0, 0, 1, 1], [None, None, 3, 0], [None, None, 0, 2]]
+ >>> first_in_class(C232)
+ False
+
+ """
+ n = C.n
+ lamda = -1
@jksuom
jksuom Jun 16, 2016 Member

This probably could be 0. Then we could have range(lambda). And lambda += 1 should be moved a bit later.

@jksuom jksuom commented on an outdated diff Jun 16, 2016
sympy/combinatorics/fp_groups.py
+ [None, None, None, 0],
+ [None, None, 0, None]]
+ >>> C232.scan(2, y**3)
+ >>> C232.table
+ [[1, 1, 2, 3], [0, 0, None, None], [None, None, 3, 0], [None, None, 0, 2]]
+ >>> C232.table[1][2] = 1; C232.table[1][3] = 1
+ >>> C232.table
+ [[1, 1, 2, 3], [0, 0, 1, 1], [None, None, 3, 0], [None, None, 0, 2]]
+ >>> first_in_class(C232)
+ False
+
+ """
+ n = C.n
+ lamda = -1
+ nu = [None]*n
+ mu = []
@jksuom
jksuom Jun 16, 2016 Member

This could probably also be [None]*n. Then there would be no need to try and append.

@jksuom jksuom commented on an outdated diff Jun 16, 2016
sympy/combinatorics/fp_groups.py
+ mu = []
+ breaker = False
+ for alpha in range(1, n):
+ # reset ν to "None" after previous value of α
+ for beta in range(lamda+1):
+ nu[mu[beta]] = None
+ # try α as the new point 0 in Ω_C_α
+ try:
+ mu[0] = alpha
+ except IndexError:
+ mu.append(alpha)
+ nu[alpha] = 0
+ # compare corresponding entries in C and C_α
+ lamda = 0
+ for beta in range(C.n):
+ if breaker:
@jksuom
jksuom Jun 16, 2016 Member

Perhaps this could be after the following loop. And probably breaker should be reset. (Something like next_alpha might be more descriptive from a mathematicians point of view.)

@jksuom jksuom commented on an outdated diff Jun 17, 2016
sympy/combinatorics/fp_groups.py
+ f = self.table[f][A_dict[word[i]]]
+ i += 1
+ if i > j:
+ return f == b
+ while j >= i and self.table[b][A_dict_inv[word[j]]] is not None:
+ b = self.table[b][A_dict_inv[word[j]]]
+ j -= 1
+ if j < i:
+ # we have an incorrect completed scan with coincidence f ~ b
+ # return False, instead of calling coincidence routine
+ return False
+ elif i == j:
+ # return True otherwise
+ self.table[f][A_dict[word[i]]] = b
+ self.table[b][A_dict_inv[word[i]]] = f
+ else:
@jksuom
jksuom Jun 17, 2016 Member

Here we should probably have only return True (also in case `i == j``).

@jksuom jksuom and 1 other commented on an outdated diff Jun 17, 2016
sympy/combinatorics/fp_groups.py
+ # continue with α
+ next_alpha = True
+ break
+ if nu[delta] is None:
+ # delta becomes the next point in Ω_C_α
+ lamda += 1
+ nu[delta] = lamda
+ mu[lamda] = delta
+ if nu[delta] < gamma:
+ return False
+ if nu[delta] > gamma:
+ # continue with α
+ next_alpha = True
+ break
+ if next_alpha:
+ break
@jksuom
jksuom Jun 17, 2016 Member

It seems that next_alpha should be reset before break (next_alpha = False).

@gxyd
gxyd Jun 17, 2016 Member

Yes, definitely.

On 06/17/2016 03:57 PM, Kalevi Suominen wrote:

In sympy/combinatorics/fp_groups.py
#11231 (comment):

  •                # continue with α
    
  •                next_alpha = True
    
  •                break
    
  •            if nu[delta] is None:
    
  •                # delta becomes the next point in Ω_C_α
    
  •                lamda += 1
    
  •                nu[delta] = lamda
    
  •                mu[lamda] = delta
    
  •            if nu[delta] < gamma:
    
  •                return False
    
  •            if nu[delta] > gamma:
    
  •                # continue with α
    
  •                next_alpha = True
    
  •                break
    
  •        if next_alpha:
    
  •            break
    

It seems that |next_alpha| should be reset before |break| (|next_alpha
= False|).


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
https://github.com/sympy/sympy/pull/11231/files/09134253eb3492d3304a8f0ebd223dbecddcdda1#r67490420,
or mute the thread
https://github.com/notifications/unsubscribe/AHWt8Snol98x9yBiE-VxN7bMn3-YSwPqks5qMnaugaJpZM4IzwKC.

@gxyd
Member
gxyd commented Jun 17, 2016

I have now basic understanding of first_in_class, added the doctest for confirmation of correct working of first_in_class routine, probably now it is working fine (apart from lamda comment, though that is not an issue result wise). Example has been taken from Derek Holt's handbook. Will now see for other routines, few comments to be addressed regarding scan_check and process_deductions_check routines.

@gxyd gxyd added the GSoC label Jun 17, 2016
@jksuom
Member
jksuom commented Jun 17, 2016

first_in_class looks good.

@jksuom jksuom commented on an outdated diff Jun 18, 2016
sympy/combinatorics/fp_groups.py
+def first_in_class(C):
+ """
+ Checks whether the subgroup H=G1 corresponding to the Coset Table could
+ possibly be the canonical representative of its conjugacy class.
+
+ Parameters
+ ==========
+
+ C: CosetTable
+
+ Returns
+ =======
+
+ bool: True/False
+
+ If this returns False, then no descendant of D can have that property, and
@jksuom
jksuom Jun 18, 2016 Member

The coset table in this paragraph should probably be the argument C, not D.

@jksuom jksuom and 1 other commented on an outdated diff Jun 18, 2016
sympy/combinatorics/fp_groups.py
@@ -378,9 +464,9 @@ def scan_and_fill_f(self, alpha, word):
def look_ahead(self):
R = self.fp_group.relators()
p = self.p
- for beta in self.omega:
- # complete scan all relators under all cosets(obviously live)
- # without making new definitions
+ # complete scan all relators under all cosets(obviously live)
+ # without making new definitions
+ for beta, w in self.omega:
@jksuom
jksuom Jun 18, 2016 Member

Should w really be here?

@gxyd
gxyd Jun 18, 2016 Member

No that shouldn't be here. Its a wrong change. I was trying to use
product here, and realized that there is to be a use of continue with β.

On 06/18/2016 04:06 PM, Kalevi Suominen wrote:

In sympy/combinatorics/fp_groups.py
#11231 (comment):

@@ -378,9 +464,9 @@ def scan_and_fill_f(self, alpha, word):
def look_ahead(self):
R = self.fp_group.relators()
p = self.p

  •    for beta in self.omega:
    
  •        # complete scan all relators under all cosets(obviously live)
    
  •        # without making new definitions
    
  •    # complete scan all relators under all cosets(obviously live)
    
  •    # without making new definitions
    
  •    for beta, w in self.omega:
    

Should |w| really be here?


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/sympy/sympy/pull/11231/files/cf9ce8dfab3b29154db79c917b8e733148045e2c#r67599158,
or mute the thread
https://github.com/notifications/unsubscribe/AHWt8UbGvW6Uzbh3uwF_xPRe3RI0epZSks5qM8oagaJpZM4IzwKC.

@jksuom jksuom and 1 other commented on an outdated diff Jun 18, 2016
sympy/combinatorics/fp_groups.py
+ for w, alpha in product(R2, C.omega):
+ if not C.scan_check(alpha, w):
+ return
+ # relators in R2 are satisfied, append the table to list
+ S.append(C)
+ else:
+ for alpha, x in product(range(len(C.table)), C.A):
+ if C.table[alpha][A_dict[x]] is None:
+ # this is "x" in pseudo-code (using "y" makes it clear)
+ undefined_coset, undefined_gen = alpha, x
+ break
+ reach = C.omega + [C.n]
+ for beta in reach:
+ if beta == C.n:
+ C.table.append([None]*len(C.A))
+ C.p.append(len(C.A) - 1)
@jksuom
jksuom Jun 18, 2016 Member

Should this be len(C.p)?

@gxyd
gxyd Jun 18, 2016 Member

Yes, this should be.

@jksuom jksuom and 1 other commented on an outdated diff Jun 18, 2016
sympy/combinatorics/fp_groups.py
+
+
+def try_descendant(S, C, R1_c_list, R2, N, alpha, x, beta):
+ """
+ It solves the problem of trying out each individual possibility for α^x.
+ """
+ D = C.copy()
+ A_dict = D.A_dict
+ if beta == D.n:
+ D.table.append([None]*len(D.A))
+ D.p.append(beta)
+ D.table[alpha][D.A_dict[x]] = beta
+ D.table[beta][D.A_dict_inv[x]] = alpha
+ D.deduction_stack.append((alpha, x))
+ D.process_deductions_check(R1_c_list[D.A_dict[x]], R1_c_list[D.A_dict_inv[x]])
+ if D.n == 0:
@jksuom
jksuom Jun 18, 2016 Member

Alternatively, we could have process_deductions_check return a boolean. Then there should be no need to modify the property n.

@gxyd
gxyd Jun 18, 2016 Member

Yes, that will be good from the implementation point of view. (though not from mathematician point of view, I believe). I agree this should be changed. :)

gxyd added some commits Jun 18, 2016
@gxyd gxyd undo the try of product use in "lookahead" routine f55bda2
@gxyd gxyd Fix the wrong code in "descendant_subgroups" routine
Add tests for "low_index_subgroup" algorithm and also
fix the use of len(C.A)-1 -> len(C.p) in descendant_subgroups
68f9f05
@gxyd
Member
gxyd commented Jun 19, 2016

Seems like Python2 doesn't have attribute copy for a list instance. Will fix that.

@gxyd
Member
gxyd commented Jun 19, 2016

Also regarding the previous comment on removing the deductions from the scan_check routine. I saw in the solved example the use of "making deductions", and apart from scan_check there can be no other routine which can "make deductions", so thought of restoring it, and it seems to be working fine.

@gxyd
Member
gxyd commented Jun 19, 2016

Will it be useful to have a routine is_valid for CosetTable? which will check for whether the instance of CosetTable is indeed possible considering its attribute value of p, table, since .table, .p can be changed. For example [[0, 0, 0, 0], [1, 1, 1, 1]] is an in-valid CosetTable since it doesn't obey the property you described yesterday.

@jksuom
Member
jksuom commented Jun 19, 2016

The actual table is probably not valid all the time since not all rows are in omega. Otherwise I believe the tables created by the algorithms are valid. Do you think there would be need for is_valid somewhere?

@gxyd
Member
gxyd commented Jun 19, 2016

No, not as in such. But while a was testing the procedures, I found the
need to use it while implementing the "coset enumeration" in PR #11140.
#11140

But I don't think it would be too much necessary. Since that wouldn't be
from the user point of view.

On 06/19/2016 02:55 PM, Kalevi Suominen wrote:

The actual |table| is probably not valid all the time since not all
rows are in omega. Otherwise I believe the tables created by the
algorithms are valid. Do you think there would be need for |is_valid|
somewhere?


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#11231 (comment), or
mute the thread
https://github.com/notifications/unsubscribe/AHWt8VG4IVDjnDkSb0wyan-3ofrBkyItks5qNQshgaJpZM4IzwKC.

@gxyd
Member
gxyd commented Jun 19, 2016

I have added the variations, which are mentioned in the section 5.4.3 of Handbook.

@gxyd gxyd changed the title from [WIP] ``Low Index Subgroups`` algorithm implementation in SymPy to ``Low Index Subgroups`` algorithm implementation in SymPy Jun 20, 2016
@jksuom jksuom commented on an outdated diff Jun 20, 2016
sympy/combinatorics/fp_groups.py
+ mu = [None]*n
+ # mutually ν and μ are the mutually-inverse equivalence maps between
+ # Ω_c_α and Ω_c
+ next_alpha = False
+ # For each 0≠α ∈ [0 .. nc-1], we start by constructing the equivalent
+ # standardized coset table C_α corresponding to H_α
+ for alpha in range(1, n):
+ # reset ν to "None" after previous value of α
+ for beta in range(lamda+1):
+ nu[mu[beta]] = None
+ # we only want to reject our current table in favour of a preceding
+ # table in the ordering in which 1 is replaced by α, if the subgroup
+ # G_α corresponding to this preceding table definitely contains the
+ # given subgroup
+ for w in Y:
+ if C.table[alpha][C.A_dict[w]] != alpha:
@jksuom
jksuom Jun 20, 2016 Member

It seems that we have to assume that the subgroup generators in Y (if there are some) are also among the generators of the whole group (or their inverses). Otherwise it is necessary to run something like scan.

@gxyd
Member
gxyd commented Jun 21, 2016

I have added the comment.

@jksuom
Member
jksuom commented Jun 21, 2016

I think this is ready to be merged.

@gxyd
Member
gxyd commented Jun 21, 2016

Yes, I think so too.

On 06/21/2016 08:17 PM, Kalevi Suominen wrote:

I think this is ready to be merged.


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#11231 (comment), or
mute the thread
https://github.com/notifications/unsubscribe/AHWt8aqd7UR3EeOIj3N6-RpMibafzl1Dks5qN_megaJpZM4IzwKC.

@jksuom jksuom merged commit 5ba8b51 into sympy:master Jun 22, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gxyd gxyd deleted the gxyd:implementation_low_index_subgroups branch Jul 1, 2016
@gxyd gxyd restored the gxyd:implementation_low_index_subgroups branch Aug 16, 2016
@gxyd gxyd deleted the gxyd:implementation_low_index_subgroups branch Aug 24, 2016
@skirpichev skirpichev referenced this pull request in diofant/diofant Dec 7, 2016
Open

[wip] Backport some sympy fixes #390

81 of 99 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment