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

FpGroup: Implemented rewriting systems #12893

Merged
merged 20 commits into from Jul 31, 2017

Conversation

Projects
None yet
2 participants
@valglad
Contributor

valglad commented Jul 5, 2017

This PR implements rewriting systems for FpGroups by creating a new class RewritingSystem. This is added to an FpGroup instance as an attribute ._rewriting_system, and three new FpGroup methods are implemented:

  • make_confluent() attempts to make the group's rewriting system confluent by employing the Knuth-Bendix algorithm
  • reduce() takes a word as an argument and returns its reduced form under the rewriting system
  • equals() takes two words and checks if they are equal under the rewriting system. If the system is confluent, this checks for actual equality of two elements of FpGroup.

Additionally, eliminate_words() method from the FreeGroupElement class is extended to handle the keyword _all properly and eliminate_word() is given a keyword attribute inverse - when this is set to False, the method won't try to substitute for the inverse of the requested substitution.

Show outdated Hide outdated sympy/combinatorics/rewritingsystem.py
if w1 < w2:
s1 = w1
w1 = w2
w2 = s1

This comment has been minimized.

@jksuom

jksuom Jul 8, 2017

Member

The swap can be a single line.

@jksuom

jksuom Jul 8, 2017

Member

The swap can be a single line.

Show outdated Hide outdated sympy/combinatorics/rewritingsystem.py
len2 = len(r2)
result = []
j = 0
while j < len1 + len2 - 1:

This comment has been minimized.

@jksuom

jksuom Jul 8, 2017

Member

Could this be a for loop like for j in range(1, len1 + len2)?

@jksuom

jksuom Jul 8, 2017

Member

Could this be a for loop like for j in range(1, len1 + len2)?

Show outdated Hide outdated sympy/combinatorics/rewritingsystem.py
# preceding ones. there is probably a better way
# to handle this
for r2 in lhs:
overlaps = _overlaps(r1, r2)

This comment has been minimized.

@jksuom

jksuom Jul 9, 2017

Member

Is it necessary to do this also when r1 and r2 are the same? It seems that, in that case, the overlap w is also the same, and then _process_overlap(w, r1, r2) would do nothing.

@jksuom

jksuom Jul 9, 2017

Member

Is it necessary to do this also when r1 and r2 are the same? It seems that, in that case, the overlap w is also the same, and then _process_overlap(w, r1, r2) would do nothing.

Show outdated Hide outdated sympy/combinatorics/rewritingsystem.py
j = 0
while j < len1 + len2 - 1:
j += 1
for j in range(1, len1 + len2 - 1):

This comment has been minimized.

@jksuom

jksuom Jul 9, 2017

Member

Note that this will make the last j equal to len1 + len2 - 2. Will that suffice?

@jksuom

jksuom Jul 9, 2017

Member

Note that this will make the last j equal to len1 + len2 - 2. Will that suffice?

This comment has been minimized.

@valglad

valglad Jul 9, 2017

Contributor

Nope, should change that.

@valglad

valglad Jul 9, 2017

Contributor

Nope, should change that.

using the group's rewriting system. If the system is
confluent, the returned answer is necessarily correct.
(If it isn't, `False` could be returned in some cases
where in fact `word1 == word2`)

This comment has been minimized.

@jksuom

jksuom Jul 10, 2017

Member

Probably None should be returned instead of False if the system is not confluent.

@jksuom

jksuom Jul 10, 2017

Member

Probably None should be returned instead of False if the system is not confluent.

This comment has been minimized.

@valglad

valglad Jul 10, 2017

Contributor

That would make sense.

@valglad

valglad Jul 10, 2017

Contributor

That would make sense.

Show outdated Hide outdated sympy/combinatorics/rewritingsystem.py
return self._is_confluent
def _init_rules(self):
rels = self.group.relators[:]

This comment has been minimized.

@jksuom

jksuom Jul 18, 2017

Member

A local copy doesn't seem necessary.

@jksuom

jksuom Jul 18, 2017

Member

A local copy doesn't seem necessary.

Show outdated Hide outdated sympy/combinatorics/rewritingsystem.py
# the overlaps. See [1], Section 3 for details.
# overlaps on the right
while len(s1) - len(s2) > 0:

This comment has been minimized.

@jksuom

jksuom Jul 18, 2017

Member

If the difference is 1 and no cancelling occurs, then it may become -1. It seems that the rule will not be stored.

@jksuom

jksuom Jul 18, 2017

Member

If the difference is 1 and no cancelling occurs, then it may become -1. It seems that the rule will not be stored.

Show outdated Hide outdated sympy/combinatorics/rewritingsystem.py
# the overlaps. See [1], Section 3 for details.
# overlaps on the right
while len(s1) - len(s2) > 1:

This comment has been minimized.

@jksuom

jksuom Jul 19, 2017

Member

It looks like the rules where len(w1) - len(w2) is 0 or 1 are not stored at all. Should that be the first thing to do before these two loops?

@jksuom

jksuom Jul 19, 2017

Member

It looks like the rules where len(w1) - len(w2) is 0 or 1 are not stored at all. Should that be the first thing to do before these two loops?

Show outdated Hide outdated sympy/combinatorics/rewritingsystem.py
def add_rule(self, w1, w2):
new_keys = set()
if len(self.rules) + 1 > self.maxeqns:
self._is_confluent = self._check_confluence()

This comment has been minimized.

@jksuom

jksuom Jul 19, 2017

Member

_check_confidence may try to add new rules. It should be called at most once The following might work:

if not self._max_exceeded:
    self._max_exceeded = True
    self._is_confluent = = self._check_confluence()
return ValueError("Too many rules were defined.")
@jksuom

jksuom Jul 19, 2017

Member

_check_confidence may try to add new rules. It should be called at most once The following might work:

if not self._max_exceeded:
    self._max_exceeded = True
    self._is_confluent = = self._check_confluence()
return ValueError("Too many rules were defined.")

This comment has been minimized.

@valglad

valglad Jul 19, 2017

Contributor

Hmm, actually maybe ._check_confluence() shouldn't try to add rules at all. Otherwise if someone runs it, it could return False while in practice the system is already confluent because of the rules it added.

@valglad

valglad Jul 19, 2017

Contributor

Hmm, actually maybe ._check_confluence() shouldn't try to add rules at all. Otherwise if someone runs it, it could return False while in practice the system is already confluent because of the rules it added.

Show outdated Hide outdated sympy/combinatorics/fp_groups.py
@@ -58,10 +58,8 @@ class FpGroup(DefaultPrinting):
def __init__(self, fr_grp, relators):
relators = _parse_relators(relators)
# return the corresponding FreeGroup if no relators are specified

This comment has been minimized.

@jksuom

jksuom Jul 19, 2017

Member

This line is no longer valid.

@jksuom

jksuom Jul 19, 2017

Member

This line is no longer valid.

valglad added some commits Jul 19, 2017

fix iteration over rules in make_confluent
The list of rules is extended in the double loop in make_confluent
but the for loop ignored the new elements. This is fixed now.
Additionally, _reduce_redundacies was run too early in the loop
and would sometimes lead to a dictionary key error. This is also
taken care of now.
replace continue by break
if the first rule in make_confluent is made redundant, the second loop
should be broken, not continued
Show outdated Hide outdated sympy/combinatorics/rewritingsystem.py
if not check:
self.rules[s1] = s2
new_keys.add(s1)
elif s1 < s2:

This comment has been minimized.

@jksuom

jksuom Jul 30, 2017

Member

Can this occur?

@jksuom

jksuom Jul 30, 2017

Member

Can this occur?

This comment has been minimized.

@valglad

valglad Jul 30, 2017

Contributor

Yes, when len(s1)==len(s2), e.g. a*b < b*a.

@valglad

valglad Jul 30, 2017

Contributor

Yes, when len(s1)==len(s2), e.g. a*b < b*a.

This comment has been minimized.

@jksuom

jksuom Jul 30, 2017

Member

Even after these lines?

    if w1 < w2:
        w1, w2 = w2, w1

    s1, s2 = w1, w2
@jksuom

jksuom Jul 30, 2017

Member

Even after these lines?

    if w1 < w2:
        w1, w2 = w2, w1

    s1, s2 = w1, w2

This comment has been minimized.

@valglad

valglad Jul 30, 2017

Contributor

Ah, okay. Then it can't. Will change.

@valglad

valglad Jul 30, 2017

Contributor

Ah, okay. Then it can't. Will change.

@jksuom jksuom merged commit b5058ba into sympy:master Jul 31, 2017

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