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 implementation of the modified coset enumeration #14830

Merged
merged 13 commits into from Jul 23, 2018

Conversation

Projects
None yet
5 participants
@RavicharanN
Contributor

RavicharanN commented Jun 24, 2018

Brief description of what is fixed or changed

Add modified methods for coset enumeration. The implementation is similar to that of the methods described in Section 5.3.2 of the Handbook

To-do:

  • Documentation hasn't been done yet.
  • Test cases need to be added.

@RavicharanN RavicharanN changed the title from Add modified methods to Add implementation of the modified coset enumeration Jun 24, 2018

@Abdullahjavednesar Abdullahjavednesar requested a review from valglad Jun 25, 2018

return C
raise e
alpha += 1
return C

This comment has been minimized.

@valglad

valglad Jun 26, 2018

Contributor

Is the above code essentially the same as coset_enumeration_r but with all relevant methods replaced by modified versions? If so, perhaps, we should just have a keyword modified in coset_enumeration_r and pick the right methods at the start, e.g. _define = C.define or _define = C.modified_define and use _define where necessary.

Same goes for any other methods that repeat a lot of the code that is already used during regular coset enumeration.

This comment has been minimized.

@valglad

valglad Jun 29, 2018

Contributor

What about this?

This comment has been minimized.

@RavicharanN

RavicharanN Jun 29, 2018

Contributor

I'll push it with the unit tests.

table[nu][C.A_dict_inv[x]] = mu
v = p_p[gamma]**-1*C.P[gamma][C.A_dict[x]]*p_p[delta]
C.P[mu][C.A_dict[x]] = v
C.P[nu][C.A_dict_inv[x]] = v**-1

This comment has been minimized.

@valglad

valglad Jun 26, 2018

Contributor

All of the above functions should be methods of CosetTable, for consistency.

This comment has been minimized.

@RavicharanN

RavicharanN Jun 26, 2018

Contributor

Okay 👍

This comment has been minimized.

@valglad

valglad Jun 29, 2018

Contributor

These are still not methods of CosetTable even though you pushed a couple of commits since.

This comment has been minimized.

@RavicharanN

RavicharanN Jun 29, 2018

Contributor

I've updated that now.

@@ -1,7 +1,7 @@
# -*- coding: utf-8 -*-
from sympy.combinatorics.fp_groups import FpGroup
from sympy.combinatorics.coset_table import (CosetTable,
coset_enumeration_r, coset_enumeration_c)
coset_enumeration_r, coset_enumeration_c, modifed_scan_and_fill, modified_coincidence, modified_define, modified_merge, modified_rep, modified_scan)

This comment has been minimized.

@valglad

valglad Jun 27, 2018

Contributor

This should be split into multiple lines for ease of reading from a standard-size terminal window. I think the limit is about 80 symbols per line.

RavicharanN added some commits Jun 28, 2018

@@ -1,7 +1,8 @@
# -*- coding: utf-8 -*-
from sympy.combinatorics.fp_groups import FpGroup
from sympy.combinatorics.coset_table import (CosetTable,
coset_enumeration_r, coset_enumeration_c)
coset_enumeration_r, coset_enumeration_c, modified_scan_and_fill,
modified_coincidence, modified_define, modified_merge, modified_rep, modified_scan)

This comment has been minimized.

@valglad

valglad Jun 29, 2018

Contributor

The last line seems very long, and I'm not sure if the one before it is the right length. Are they <=72 symbols per line (including whitespace at the start)?

This comment has been minimized.

@RavicharanN

RavicharanN Jun 29, 2018

Contributor

This is removed anyway, 'coz the modified methods need not be imported now.

@valglad

This comment has been minimized.

Contributor

valglad commented Jun 29, 2018

The tests should be part of the main coset_table file - it makes sense to have them separately for local testing but this should be changed for the final version.

def modified_define(self, alpha, x):
'''
Input: ∈ , x∈A, with x undefined.

This comment has been minimized.

@asmeurer

asmeurer Jun 29, 2018

Member

Are there missing characters here?

In general, I would use LaTeX math instead of Unicode characters. That will render better in the Sphinx documentation anyway.

This comment has been minimized.

@RavicharanN

RavicharanN Jun 29, 2018

Contributor

Yes, I will change the docstrings. They are yet to be updated, for all the modified methods.

This comment has been minimized.

@valglad

valglad Jul 7, 2018

Contributor

This still needs sorting.

This comment has been minimized.

@valglad

valglad Jul 12, 2018

Contributor

Still needs fixing.

raise ValueError("the coset enumeration has defined more than %s cosets. Try with a greater value max number of cosets "%C.coset_table_max_limit)
self.table.append([None]*len(self.A))
H = self.subgroup
self._grp = (list(free_group(', ' .join(["a_%d" % i for i in range(len(H))])))).pop(0)

This comment has been minimized.

@asmeurer

asmeurer Jun 29, 2018

Member

Does free_group support indexing. Would free_group(...)[0] work instead?

This comment has been minimized.

@RavicharanN

RavicharanN Jun 29, 2018

Contributor

self._grp = list(free_group(...))[0] should work here. Do you want me to update that?

This comment has been minimized.

@valglad

valglad Jun 29, 2018

Contributor

Simply free_group(...)[0] will work too, and it is shorter and looks clearer so better change it.

This comment has been minimized.

@valglad

valglad Jul 5, 2018

Contributor

The two lines above looks like it should be done before coset enumeration starts.

rho = s[mu]
p[rho] = lambda_
p_p[rho] = p_p[rho]*p_p[mu]
return lambda_

This comment has been minimized.

@valglad

valglad Jul 3, 2018

Contributor

How different is this from the regular rep method? You have to update p_p but other than that it looks almost exactly the same.Would it be worth just letting rep have a modified keyword? Same for the other methods that overlap with the regular ones a lot.

This comment has been minimized.

@valglad

valglad Jul 7, 2018

Contributor

And this should be merged with rep.

self.table[beta][self.A_dict_inv[x]] = alpha
# P[alpha][x] = epsilon, P[beta][x**-1] = epsilon
self.P[alpha][self.A_dict[x]] = self._grp.identity
self.P[beta][self.A_dict_inv[x]] = self._grp.identity

This comment has been minimized.

@valglad

valglad Jul 5, 2018

Contributor

This looks sufficiently like define that I'd give define a modified keyword. You can still define

def modified_define:
    define(..., modified=True)

if you think using modified_define will be more convenient.

This comment has been minimized.

@RavicharanN

RavicharanN Jul 5, 2018

Contributor

I was actually thinking of doing this. Will check it for the other methods too 👍

This comment has been minimized.

@valglad

valglad Jul 7, 2018

Contributor

This can still be merged with define.

r = len(w)
f = alpha
i = 0
f_p = self._grp.identity

This comment has been minimized.

@valglad

valglad Jul 5, 2018

Contributor

These 4 lines should go outside the loop

if j < i:
self.modified_coincidence(f, b, f_p**-1*b_p)
elif j == i:
# Dedcution

This comment has been minimized.

@valglad

valglad Jul 5, 2018

Contributor

typo

self.P[f][self.A_dict[w[i]]] = f_p**-1*b_p
self.P[b][self.A_dict_inv[w[i]]] = b_p**-1*f_p
else:
if not fill:

This comment has been minimized.

@valglad

valglad Jul 5, 2018

Contributor

This can be elif, and should it not be fill rather than not fill?

# For scan an not filling.
if not fill:
break

This comment has been minimized.

@valglad

valglad Jul 5, 2018

Contributor

Would it work if you said while fill or i == 0 (i==0 so that the loop is entered at least one when fill=False) instead of while True at the start? Then you won't need this break statement.

This comment has been minimized.

@valglad

valglad Jul 5, 2018

Contributor

And same comment as before - this repeats a lot of code from scan_and_fill so they could be just one method. Feel free to use the way you wrote it out as the basis for the common function - I quite like the idea of having all code in scan and defining scan_and_fill as scan(..., fill=True) 👍

mu = rho
rho = s[mu]
p[rho] = lambda_
p_p[rho] = p_p[rho]*p_p[mu]

This comment has been minimized.

@valglad

valglad Jul 5, 2018

Contributor

The Handbook defines an identity entry of p_p in modified_define but your version doesn't. I don't think you are doing that anywhere else and this could cause problems

self.p[phi] = psi
p_p[phi] = p_p[k]**-1*w*p_p[lamda]
l += 1
q[l] = psi

This comment has been minimized.

@valglad

valglad Jul 5, 2018

Contributor

Should be q[l] = phi. But also, you are not using l in modified_coincidence so you don't need to use it at all, and this can be q.append(phi).

self.p[psi] = phi
p_p[psi] = p_p[lamda]**-1*w**-1*p_p[k]
l += 1
q[l] = psi

This comment has been minimized.

@valglad

valglad Jul 5, 2018

Contributor

Same here, q.append(psi)

v = p_p[delta]**-1*self.P[gamma][self.A_dict[x]]**-1*p_p[gamma]*self.P[mu][self.A_dict[x]]
self.modified_merge(nu, table[mu][self.A_dict[x]], v, p_p, q, l)
elif table[nu][self.A_dict_inv[x]]:
v = p_p[gamma]**-1*self.P[gamma][self.A_dict[x]]*p_p[delta]*self.P[mu][self.A_dict_inv[x]]

This comment has been minimized.

@valglad

valglad Jul 5, 2018

Contributor

The lines above defining v look like they may be over 72 symbols. If so, then better shorten them

@valglad

This comment has been minimized.

Contributor

valglad commented Jul 5, 2018

The code seems generally good, apart from a few minor things, so what remains is to integrate regular and modified methods into one to avoid repeating lots of code, sort out docstring formatting and move the tests to the main coset_table test file.

@@ -458,7 +489,7 @@ def rep(self, k):
# α, β coincide, i.e. α, β represent the pair of cosets
# where coincidence occurs
def coincidence(self, alpha, beta):
def coincidence(self, alpha, beta, w=None,modified=False):

This comment has been minimized.

@valglad

valglad Jul 7, 2018

Contributor

There should be a space before modified, and the docstring should include a note on what modified=True does.

p_p = []
if modified:
p_p[beta] = self._grp.identity
l = 0

This comment has been minimized.

@valglad

valglad Jul 7, 2018

Contributor

You deleted l above but now it's here again. I assume you copied it by accident?

@@ -783,7 +810,7 @@ def modified_define(self, alpha, x):
raise ValueError("the coset enumeration has defined more than %s cosets. Try with a greater value max number of cosets "%C.coset_table_max_limit)
self.table.append([None]*len(self.A))
H = self.subgroup
self._grp = (list(free_group(', ' .join(["a_%d" % i for i in range(len(H))])))).pop(0)
self._grp = list(free_group(', ' .join(["a_%d" % i for i in range(len(H))])))[0]

This comment has been minimized.

@valglad

valglad Jul 7, 2018

Contributor

This can just be free_group(', ' .join(["a_%d" % i for i in range(len(H))])))[0] - no need for list.

self.table[beta][self.A_dict_inv[x]] = alpha
# P[alpha][x] = epsilon, P[beta][x**-1] = epsilon
self.P[alpha][self.A_dict[x]] = self._grp.identity
self.P[beta][self.A_dict_inv[x]] = self._grp.identity

This comment has been minimized.

@valglad

valglad Jul 7, 2018

Contributor

This can still be merged with define.

rho = s[mu]
p[rho] = lambda_
p_p[rho] = p_p[rho]*p_p[mu]
return lambda_

This comment has been minimized.

@valglad

valglad Jul 7, 2018

Contributor

And this should be merged with rep.

p_p[beta] = self._grp.identity
l = 0
if modified:
self.modified_merge(alpha, beta, w, p_p, q)

This comment has been minimized.

@valglad

valglad Jul 7, 2018

Contributor

I think p_p should just be an attribute of CosetTable, like P. Then you wouldn't need to keep passing it to all these methods and some methods will look more like each other, e.g. modified_rep takes two arguments right now, but it could be taking just one, exactly like rep.

self.modified_define(f, word[i])
else:
self.define(f, word[i])
# otherwise scan is incomplete and yields no information

This comment has been minimized.

@valglad

valglad Jul 7, 2018

Contributor

This could be self.define(f, word[i], modified=modified) once you introduce the keyword argument.

self.merge(alpha, beta, q)
p_p = []
if modified:
p_p[beta] = self._grp.identity

This comment has been minimized.

@valglad

valglad Jul 7, 2018

Contributor

Will this always be the same as what you'd set as identity in modified_define? If you make p_p a CosetTable attribute, it'll be easy to do this in modified_define and probably safer.

nu = self.modified_rep(delta, p_p)
else:
mu = self.rep(gamma)
nu = self.rep(delta)

This comment has been minimized.

@valglad

valglad Jul 7, 2018

Contributor

This is one example where having p_p as a CosetTable attribute would be useful: then you could just use self.rep(gamma, modified=modified).

def modified_define(self, alpha, x):
'''
Input: ∈ , x∈A, with x undefined.

This comment has been minimized.

@valglad

valglad Jul 7, 2018

Contributor

This still needs sorting.

@@ -123,7 +123,7 @@ def is_complete(self):
return not any(None in self.table[coset] for coset in self.omega)
# Pg. 153 [1]
def define(self, alpha, x):
def define(self, alpha, x, modified=True):

This comment has been minimized.

@valglad

valglad Jul 12, 2018

Contributor

This should probably be False by default

# P[alpha][x] = epsilon, P[beta][x**-1] = epsilon
if modified:
self.P[alpha][self.A_dict[x]] = self._grp.identity
self.P[beta][self.A_dict_inv[x]] = self._grp.identity

This comment has been minimized.

@valglad

valglad Jul 12, 2018

Contributor

Should also do self.p_p[beta]=self._grp.identity

H = self.subgroup
self._grp = free_group(', ' .join(["a_%d" % i for i in range(len(H))]))[0]
self.P.append([None]*len(self.A))
self.p_p = []

This comment has been minimized.

@valglad

valglad Jul 12, 2018

Contributor

All of this should still be done outside of this function, in the main coset_enumeration - it only needs doing once but define is called a lot of times...

@@ -323,7 +332,9 @@ def scan(self, alpha, word, y=None, fill=False, modified=False):
f_p = None
if modified:
f_p = self._grp.identity
while fill or i == 0:
itr = 0

This comment has been minimized.

@valglad

valglad Jul 12, 2018

Contributor

Why introduce itr? What was wrong with using i?

This comment has been minimized.

@RavicharanN

RavicharanN Jul 12, 2018

Contributor

Using i causes problems with a few tests. It gets into an infinite loop. So, I had to use another iterator.

This comment has been minimized.

@valglad

valglad Jul 12, 2018

Contributor

This is somewhat worrying because regular scan either doesn't have this problem or the tests aren't detecting it. Are these tests that had failures the ones for modified methods or regular ones? Does regular coset enumeration fail in master on the same examples?

If regular coset enumeration doesn't have this issue, then the problem is with the modified code you added, in which case this solution seems like something that masks the underlying problem without actually trying to fix it - kind of like covering a hole in the wall with a picture instead of properly mending it. Maybe, itr is really the whole solution but we'd need more justification for it.

This comment has been minimized.

@RavicharanN

RavicharanN Jul 12, 2018

Contributor

I've tested it on the master branch where the modified methods aren't defined. It still goes into an infinite loop.

This comment has been minimized.

@valglad

valglad Jul 12, 2018

Contributor

You mean you tried it with one of the examples you are using here in modified tests? Then it's very lucky that we caught this bug! I'll think about whether there is more to this problem, but perhaps for now this solution will do.

This comment has been minimized.

@valglad

valglad Jul 13, 2018

Contributor

But coset table tests don't fail in master

This comment has been minimized.

@RavicharanN

RavicharanN Jul 13, 2018

Contributor

I meant they fail once both the methods are merged scan and scan_and_fill

This comment has been minimized.

@valglad

valglad Jul 13, 2018

Contributor

Okay, but that means that the issue is with the code you added. scan shouldn't behave any different to how it used to when modified=False, even without itr, so there's definitely something more to it.

This comment has been minimized.

@RavicharanN

RavicharanN Jul 13, 2018

Contributor

But there aren't many changes when both the methods are merged except for adding the while loop.

This comment has been minimized.

@valglad

valglad Jul 13, 2018

Contributor

There must be something though. scan_and_fill had the while loop and didn't go into an infinite loop, and for scan fill=False so the loop shouldn't happen infinitely many times anyway. I guess if i isn't incremented and none of the if statements are true (because then adding return would work). Perhaps, in that case we could exit differently? I guess, it just doesn't seem very elegant to increment a variable each time the loop is run for no better reason other than in one special case it's useful

if modified:
self.modified_merge(alpha, beta, w, p_p, q)
self.p_p[beta] = self._grp.identity

This comment has been minimized.

@valglad

valglad Jul 12, 2018

Contributor

This should be done in define

if table[mu][A_dict[x]] is not None:
if modified:
v = p_p[delta]**-1*self.P[gamma][self.A_dict[x]]**-1*p_p[gamma]*self.P[mu][self.A_dict[x]]
self.modified_merge(nu, table[mu][self.A_dict[x]], v, p_p, q)
v = self.p_p[delta]**-1*self.P[gamma][self.A_dict[x]]**-1*self.p_p[gamma]*self.P[mu][self.A_dict[x]]

This comment has been minimized.

@valglad

valglad Jul 12, 2018

Contributor

This line is too long - it should be shortened to <= 72 symbols (generally, if you run git diff in terminal before committing the changes, the lines that are too long will wrap around - that's how you tell).

def modified_define(self, alpha, x):
'''
Input: ∈ , x∈A, with x undefined.

This comment has been minimized.

@valglad

valglad Jul 12, 2018

Contributor

Still needs fixing.

q.append(phi)
elif phi < psi:
self.p[psi] = phi
p_p[psi] = p_p[lamda]**-1*w**-1*p_p[k]
self.p_p[psi] = self.p_p[lamda]**-1*w**-1*self.p_p[k]
q.append(psi)

This comment has been minimized.

@valglad

valglad Jul 12, 2018

Contributor

Did you see how it's written out in regular merge? You can use min(phi, psi) and make this twice as short.

@@ -356,10 +367,7 @@ def scan(self, alpha, word, y=None, fill=False, modified=False):
self.P[f][self.A_dict[word[i]]] = f_p**-1*b_p
self.P[b][self.A_dict_inv[word[i]]] = b_p**-1*f_p

This comment has been minimized.

@valglad

valglad Jul 12, 2018

Contributor

There is a return statement here in the Handbook. Could you check if that stops the infinite loop without itr?

This comment has been minimized.

@RavicharanN

RavicharanN Jul 13, 2018

Contributor

I tried adding the return statement. It's still the same. The problem is when self.scan(beta, w) is called from look_ahead method which is called from process_deductions. This happens with coset_enumeration_c.

Summary
=======
Define a function p_p from from [1..n] to A* as
an additional component of the modifief coset table.

This comment has been minimized.

@valglad

valglad Jul 21, 2018

Contributor

typo: "modified". Also, I think this line starting with "Define" should be the first sentence of the docstring.

See Also
========
define
'''

This comment has been minimized.

@valglad

valglad Jul 21, 2018

Contributor

I can't see what exactly is going on from here, but Python 3.6 claims a SyntaxError due to an invalid escape sequence \e here. Is there an unusual character (rather than just \newline or something like that) after or before '''?

This comment has been minimized.

@jksuom

jksuom Jul 21, 2018

Member

There is a file that 3.6 cannot compile. It is hard to find as GitHub does not show the error.

This comment has been minimized.

@RavicharanN

RavicharanN Jul 21, 2018

Contributor

Not able to produce that error locally. The doctests with Python3 run fine on my machine.

This comment has been minimized.

@valglad

valglad Jul 21, 2018

Contributor

@RavicharanN Is your python3 the 3.6 version though?

This comment has been minimized.

@RavicharanN

RavicharanN Jul 22, 2018

Contributor

Yeah, it is. As @jksuom suggested, using raw strings did the trick. 👍

def modified_define(self, alpha, x):
"""
Input: \alpha \in \Omega, x \in A*

This comment has been minimized.

@jksuom

jksuom Jul 21, 2018

Member

I suspect that this is the issue: unescaped backslashes. There should be either \\alpha..., or r""" for raw string.

f_p = self._grp.identity
itr = 0
while fill or itr == 0:
itr += 1

This comment has been minimized.

@valglad

valglad Jul 22, 2018

Contributor

This could just be itr=1.

@valglad

This comment has been minimized.

Contributor

valglad commented Jul 22, 2018

This looks almost ready now but the tests should go in the main test_coset_table (+ itr can become a flag)

Example 5.7 from [1] Holt, D., Eick, B., O'Brien
"Handbook of Computational Group Theory".
'''

This comment has been minimized.

@valglad

valglad Jul 23, 2018

Contributor

I think the modified tests should have an actual coset enumeration test - something that would catch the error that you've just fixed (the one with the infinite loop)

@valglad

This comment has been minimized.

Contributor

valglad commented Jul 23, 2018

I think this is good now, let's merge it.

@valglad valglad merged commit ed58372 into sympy:master Jul 23, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment