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

Add automaton for word reduction in rewriting systems #14735

Merged
merged 29 commits into from Jul 8, 2018
Merged
Show file tree
Hide file tree
Changes from 27 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
db6c210
Add fsm
RavicharanN May 20, 2018
809ac89
Fix the fsm code
RavicharanN May 21, 2018
a7b93a9
Compute prefixes and proper prefixes
RavicharanN May 22, 2018
f15eecd
Complete the automata design
RavicharanN May 22, 2018
3ee1d07
Add word reduction using automaton
RavicharanN May 23, 2018
13736ae
Fix fsm
RavicharanN May 23, 2018
cde6408
Remove trailing whitespaces
RavicharanN May 23, 2018
c2c57b2
Add more tests
RavicharanN May 26, 2018
5898726
remove add rule section in tests
RavicharanN May 26, 2018
71bb68a
Fix typos
RavicharanN May 26, 2018
173c6c1
make fsm an attribute of rewriting system class
RavicharanN May 26, 2018
1aa4f01
Add inverse rule to the automaton
RavicharanN May 28, 2018
dfe5122
fix key error in all_rules
RavicharanN May 29, 2018
2695919
change the type of 'states' attribute to dictionary
RavicharanN Jun 1, 2018
4bb64d3
Minor changes
RavicharanN Jun 2, 2018
5716eb9
Add method for add_transitions
RavicharanN Jun 3, 2018
e3969c1
Make right hand rule an attribute of State
RavicharanN Jun 9, 2018
bfbb650
Modify docstrings
RavicharanN Jun 10, 2018
51f58ce
Change subst to rh_rule
RavicharanN Jun 13, 2018
f20a178
Construct the automaton using the add_to_automaton method
RavicharanN Jun 14, 2018
57c95a2
Change the attributes of the class
RavicharanN Jun 14, 2018
930cfa9
Compute new_rules when added to the system
RavicharanN Jun 17, 2018
4fb9964
Add to new_rules in the _add_rule method
RavicharanN Jun 21, 2018
424f89f
Add state state in the __init__ method of the StateMachine class
RavicharanN Jun 23, 2018
8935d87
Minor fixes
RavicharanN Jun 29, 2018
d4d5741
Modify the state_type attribute
RavicharanN Jul 2, 2018
96d425e
Fix minor issues
RavicharanN Jul 4, 2018
14e5686
Fix bugs causing test failures
RavicharanN Jul 5, 2018
e58652e
Prevent the overlap between accept and dead states
RavicharanN Jul 7, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
177 changes: 177 additions & 0 deletions sympy/combinatorics/rewritingsystem.py
Expand Up @@ -2,6 +2,7 @@

from sympy import S
from sympy.combinatorics.free_groups import FreeGroupElement
from sympy.combinatorics.rewritingsystem_fsm import State, StateMachine

class RewritingSystem(object):
'''
Expand Down Expand Up @@ -31,11 +32,23 @@ def __init__(self, group):
# at any point
self._max_exceeded = False

# Reduction automaton
self.reduction_automaton = None
self._new_rules = {}

# dictionary of reductions
self.rules = {}
self.rules_cache = deque([], 50)
self._init_rules()


# All the transition symbols in the automaton
generators = list(self.alphabet)
generators += [gen**-1 for gen in generators]
# Create a finite state machine as an instance of the StateMachine object
self.reduction_automaton = StateMachine('Reduction automaton for '+ repr(self.group), generators)
self.construct_automaton()

def set_max(self, n):
'''
Set the maximum number of rules that can be defined
Expand Down Expand Up @@ -74,6 +87,9 @@ def _add_rule(self, r1, r2):
self._max_exceeded = True
raise RuntimeError("Too many rules were defined.")
self.rules[r1] = r2
# Add the newly added rule to the `new_rules` dictionary.
if self.reduction_automaton:
self._new_rules[r1] = r2

def add_rule(self, w1, w2, check=False):
new_keys = set()
Expand Down Expand Up @@ -285,3 +301,164 @@ def reduce(self, word, exclude=None):
if new != prev:
again = True
return new

def _compute_inverse_rules(self, rules):
'''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be part of construct_automaton, perhaps as a subfunction there - there is no reason for it to be among the RewritingSystem methods

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to make use of this method in the reduce_using_automaton method as well, so, I defined it as a RewritingSystem method.

Compute the inverse rules for a given set of rules.
The inverse rules are used in the automaton for word reduction.

Arguments:
rules (dictionary): Rules for which the inverse rules are to computed.

Returns:
Dictionary of inverse_rules.

'''
inverse_rules = {}
for r in rules:
rule_key_inverse = r**-1
rule_value_inverse = (rules[r])**-1
if (rule_value_inverse < rule_key_inverse):
inverse_rules[rule_key_inverse] = rule_value_inverse
else:
inverse_rules[rule_value_inverse] = rule_key_inverse
return inverse_rules

def construct_automaton(self):
'''
Construct the automaton based on the set of reduction rules of the system.

Automata Design:
The accept states of the automaton are the proper prefixes of the left hand side of the rules.
The complete left hand side of the rules are the dead states of the automaton.

'''
self._add_to_automaton(self.rules)
Copy link
Contributor

@valglad valglad Jun 23, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you do this, then you'll still have _new_rules contain all the rules. I think it might make sense to leave it until reduce_using_automaton is called - then you'll add self._new_rules and empty the dictionary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the time the control flow reaches here, the _new_rules dictionary will be empty since they are not yet computed as self.reduction_automaton = None by then.


def _add_to_automaton(self, rules):
'''
Add new states and transitions to the automaton.

Summary:
States corresponding to the new rules added to the system are computed and added to the automaton.
Transitions in the previously added states are also modified if necessary.

Arguments:
rules (dictionary) -- Dictionary of the newly added rules.

'''
# Automaton variables
automaton_alphabet = []
proper_prefixes = {}

# compute the inverses of all the new rules added
all_rules = rules
inverse_rules = self._compute_inverse_rules(all_rules)
all_rules.update(inverse_rules)

# Keep track of the accept_states.
accept_states = []


for rule in all_rules:
# The symbols present in the new rules are the symbols to be verified at each state.
# computes the automaton_alphabet, as the transitions solely depend upon the new states.
automaton_alphabet += rule.letter_form_elm
# Compute the proper prefixes for every rule.
proper_prefixes[rule] = []
letter_word_array = [s for s in rule.letter_form_elm]
len_letter_word_array = len(letter_word_array)
for i in range (1, len_letter_word_array):
letter_word_array[i] = letter_word_array[i-1]*letter_word_array[i]
proper_prefixes[rule] = letter_word_array
# Add dead states
if not rule in self.reduction_automaton.states:
self.reduction_automaton.add_state(rule, state_type='d', rh_rule=all_rules[rule])

for elem in proper_prefixes[rule]:
if not elem in self.reduction_automaton.states:
self.reduction_automaton.add_state(elem, state_type='a')
accept_states.append(elem)
Copy link
Contributor

@valglad valglad Jul 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Okay, I know I said that there shouldn't be any more changes to make but now that I've noticed multiple small inefficiencies in this part of the code, I can't just let them pass. I promise what I suggest here will be the absolutely last changes and this will be good to merge)

The last 4 for loops all iterate over all_rules - it can all be done in one loop where what follows after for elem in prop_prefix is inserted into where the prefixes are calculated to avoid yet another loop. Do you see this?


automaton_alphabet = set(automaton_alphabet)

# Add new transitions for every state.
for state in self.reduction_automaton.states:
current_state_name = state
current_state_type = self.reduction_automaton.states[state].state_type
# Transitions will be modified only when suffixes of the current_state
# belongs to the proper_prefixes of the new rules.
# The rest are ignored if they cannot lead to a dead state after a finite number of transisitons.
if current_state_type == 's':
for letter in automaton_alphabet:
if letter in self.reduction_automaton.states:
self.reduction_automaton.states[state].add_transition(letter, letter)
else:
self.reduction_automaton.states[state].add_transition(letter, current_state_name)
elif current_state_type == 'a':
next_sub = current_state_name
# Check if the transition to any new state in posible.
while len(next_sub):
if next_sub in accept_states:
# Add accept states.
for letter in automaton_alphabet:
_next = current_state_name*letter
while len(_next):
if _next in self.reduction_automaton.states:
self.reduction_automaton.states[state].add_transition(letter, _next)
break
elif len(_next) == 1:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not be len(_next) == 0?
Also, if you were to do

while len(_next) and _next not in self.reduction_automaton.states:
    _next = _next.subword(1, len(_next))
if not len(_next):
    _next = 'start'
self.reduction_automaton.states[state].add_transition(letter, _next)

that would be a bit shorter.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright 👍

self.reduction_automaton.states[state].add_transition(letter, 'start')
_next = _next.subword(1, len(_next))
next_sub = next_sub.subword(1, len(next_sub))

# Add transitions for new states. All symbols used in the automaton are considered here.
# Ignore this if `reduction_automaton.automaton_alphabet` = `automaton_alphabet`.
if len(self.reduction_automaton.automaton_alphabet) != len(automaton_alphabet):
for state in accept_states:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the dead states?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead state transitions could be avoided anyway, right?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, you bring it back to start state in reduce_using_automaton. Yes, that's fine.

current_state_name = state
for letter in self.reduction_automaton.automaton_alphabet:
next = current_state_name*letter
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One of the tests in the build fails with TypeError: 'str' object cannot be interpreted as an integer on this line. Is thi because the name of the start state is a string, not a group element or is it something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is because of start being a string. I have seen that the tests failed but I didn't update this yesterday. I will do it by tonight.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like it's not because of start being a string. current_state_name has to be defined again in this scope. I guess I accidentally deleted that when I made these new changes. Anyways, I've updated it now.

len_next_word = len(next)
while len(next):
if next in self.reduction_automaton.states:
self.reduction_automaton.states[state].add_transition(letter, next)
break
elif len(next) == 1:
self.reduction_automaton.states[state].add_transition(letter, 'start')
next = next.subword(1, len(next))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The variable here is still called next


def reduce_using_automaton(self, word):
'''
Reduce a word using an automaton.

Summary:
All the symbols of the word are stored in an array and are given as the input to the automaton.
If the automaton reaches a dead state that subword is replaced and the automaton is run from the beginning.
The complete word has to be replaced when the word is read and the automaton reaches a dead state.
So, this process is repeated until the word is read completely and the automaton reaches the accept state.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much clearer 👍


Arguments:
word (instance of FreeGroupElement) -- Word that needs to be reduced.

'''
# Modify the automaton if new rules are found.
if self._new_rules:
self._add_to_automaton(self._new_rules)
self._new_rules = {}

flag = 1
while flag:
flag = 0
current_state = self.reduction_automaton.states['start']
word_array = [s for s in word.letter_form_elm]
for i in range (0, len(word_array)):
next_state_name = current_state.transitions[word_array[i]]
next_state = self.reduction_automaton.states[next_state_name]
if next_state.state_type == 'd':
subst = next_state.rh_rule
word = word.substituted_word(i - len(next_state_name) + 1, i+1, subst)
flag = 1
break
current_state = next_state
return word
60 changes: 60 additions & 0 deletions sympy/combinatorics/rewritingsystem_fsm.py
@@ -0,0 +1,60 @@
class State(object):
'''
A representation of a state managed by a ``StateMachine``.

Attributes:
name (instance of FreeGroupElement or string) -- State name which is also assigned to the Machine.
transisitons (OrderedDict) -- Represents all the transitions of the state object.
state_type (string) -- Denotes the type (accept/start/dead) of the state.
rh_rule (instance of FreeGroupElement) -- right hand rule for dead state.
state_machine (instance of StateMachine object) -- The finite state machine that the state belongs to.
'''

def __init__(self, name, state_machine, state_type=None, rh_rule=None):
self.name = name
self.transitions = {}
self.state_machine = state_machine
self.state_type = state_type[0]
self.rh_rule = rh_rule

def add_transition(self, letter, state):
'''
Add a transition from the current state to a new state.

Keyword Arguments:
letter -- The alphabet element the current state reads to make the state transition.
state -- This will be an instance of the State object which represents a new state after in the transition after the alphabet is read.

'''
self.transitions[letter] = state
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel that a state should have a state machine associated with it, because it doesn't make sense to have a state without a machine. Maybe as a state_machine attribute? Then if you also add an alphabet attribute to StateMachine, you could check here whether letter is in state_machine.alphabet.


class StateMachine(object):
'''
Representation of a finite state machine the manages the states and the transitions of the automaton.

Attributes:
states (dictionary) -- Collection of all registered `State` objects.
name (str) -- Name of the state machine.
'''

def __init__(self, name, automaton_alphabet):
self.name = name
self.automaton_alphabet = automaton_alphabet
self.states = {} # Contains all the states in the machine.
self.add_state('start', state_type='s')

def add_state(self, state_name, state_type=None, rh_rule=None):
'''
Instantiate a state object and stores it in the 'states' dictionary.

Arguments:
state_name (instance of FreeGroupElement or string) -- name of the new states.
state_type (string) -- Denotes the type (accept/start/dead) of the state added.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think elaborating a bit to say that it's 'accept'/'start'/'dead' or 'a'/'s'/'d' respectively could be helpful to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay 👍

rh_rule (instance of FreeGroupElement) -- right hand rule for dead state.

'''
new_state = State(state_name, self, state_type, rh_rule)
self.states[state_name] = new_state

def __repr__(self):
return "%s" % (self.name)
17 changes: 17 additions & 0 deletions sympy/combinatorics/tests/test_rewriting.py
Expand Up @@ -12,6 +12,10 @@ def test_rewriting():
assert G.reduce(b**3*a**4*b**-2*a) == a**5*b
assert G.equals(b**2*a**-1*b, b**4*a**-1*b**-1)

assert R.reduce_using_automaton(b*a*a**2*b**-1) == a**3
assert R.reduce_using_automaton(b**3*a**4*b**-2*a) == a**5*b
assert R.reduce_using_automaton(b**-1*a) == a*b**-1

G = FpGroup(F, [a**3, b**3, (a*b)**2])
R = G._rewriting_system
R.make_confluent()
Expand All @@ -21,3 +25,16 @@ def test_rewriting():
# but also the system should actually be confluent
assert R._check_confluence()
assert G.reduce(b*a**-1*b**-1*a**3*b**4*a**-1*b**-15) == a**-1*b**-1
# check for automaton reduction
assert R.reduce_using_automaton(b*a**-1*b**-1*a**3*b**4*a**-1*b**-15) == a**-1*b**-1

G = FpGroup(F, [a**2, b**3, (a*b)**4])
R = G._rewriting_system
assert G.reduce(a**2*b**-2*a**2*b) == b**-1
assert R.reduce_using_automaton(a**2*b**-2*a**2*b) == b**-1
assert G.reduce(a**3*b**-2*a**2*b) == a**-1*b**-1
assert R.reduce_using_automaton(a**3*b**-2*a**2*b) == a**-1*b**-1
# Check after adding a rule
R.add_rule(a**2, b)
assert R.reduce_using_automaton(a**2*b**-2*a**2*b) == b**-1
Copy link
Contributor

@valglad valglad Jun 29, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed this: is this correct? With the rule you added, we should have R.reduce_using_automaton(a**2*b**-2*a**2*b) == b - even if b**-1 is equivalent to b under the rule, b is smaller in the ordering that the rewriting system enforces on generators. What does reduce return in this case? Does G.equals(b, b**-1) work? You could also try running make_confluent after adding the rule and seeing if that makes a difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does reduce return in this case?

reduce returns b**-1 in Python3 and b in Python 2.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even after make_confluent?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They return identity after make confluent

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, this does make sense. If reduce_using_automaton gives the identity after make_confluent as well, then this is fine.

assert R.reduce_using_automaton(a**4*b**-2*a**2*b**3) == b