-
Notifications
You must be signed in to change notification settings - Fork 264
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
just finite_state_machines #956
Conversation
It's ok if there are unreachable states, I think. Some of these are the output of ML training algorithms (anything that has Evolved in the name), and reachability wasn't enforced. That said, if we can show that removing some of the states does not affect the strategy at all then I'm in favor of simplifying them. |
@@ -101,14 +123,15 @@ class Fortress3(FSMPlayer): | |||
} | |||
|
|||
def __init__(self) -> None: | |||
transitions = ( | |||
(1, D, 2, D), | |||
transitions = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for the change to a list instead of a tuple?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the FSM and FSMPlayer used a list. then it got changed to tuple in all the subsequent ones. i spent about 15 minutes scratching my head trying to figure out why mypy was complaining before i noticed the difference. i used list because that's what i saw first and they all needed to be the same.
should i change all to tuples or leave as is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My preference is tuples since everything involved is immutable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. will do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mypy gave me trouble on the change. it only worked it the type hints were a generalized tuple. it did have the nice advantage that i could set the defaults in the kwargs and not after the fact.
@@ -174,13 +265,46 @@ class TestPredator(TestFSMPlayer): | |||
'manipulates_source': False, | |||
'manipulates_state': False | |||
} | |||
""" | |||
transitions = [ | |||
(0, C, 0, D), NO ROUTE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These states are still active on the first move? There's just no route back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a named strategy from the literature, FYI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they are not active on the first move because "initial_state" is set to one. should initial state get set to 0?
DOI:10.1109/CEC.2006.1688322.
i was wondering what that was!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's supposed to start at 0, good catch!
return False | ||
return (self._state, self._state_transitions) == (other.state, other.state_transitions) | ||
|
||
def __ne__(self, other): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we not get __ne__
for free by defining __eq__
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's no longer necessary in py3, can we remove it (less lines of code to maintain :)) and just have a test that checks we have the __ne__
behaviour?
@@ -55,16 +81,16 @@ class FSMPlayer(Player): | |||
'manipulates_state': False | |||
} | |||
|
|||
def __init__(self, transitions=None, initial_state=None, | |||
initial_action=None) -> None: | |||
def __init__(self, transitions: List[Tuple[int, Any, int, Any]] =None, initial_state: int =None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please check the spacing on the function definition
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you mean def something(num: int =5):
should be def something(num: int = 5)
? if that's it, i wasn't sure of the spacing convention for type hints.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's the convention we've been trying to use.
should they be commented out with a note about the ML? should they be deleted and numbers be left as is? The class names would be confusing otherwise. here's a test i wrote, that prints out proof of inaccessibility.
output was.
and
|
I vote to just delete the unreachable states, and let's leave that test in for future submissions. If something goes wrong we can look back in the git history for the deleted transitions. |
if un_callable_states: | ||
raise AssertionError('The following states are un-callable: {}'.format(un_callable_states)) | ||
error_message = 'The following states are un-reachable: {}'.format(list(un_callable_states)) | ||
self.assertEqual(error_message, 'The following states are un-reachable: []') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this supposed to be testing? I'm a bit confused by the need for this line...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from @marcharper
I vote to just delete the unreachable states, and let's leave that test in for future submissions.
it's supposed to test whether an FSM has states that can't actually be reached, as was found for the two EvolvedFSM classes . i thought the lines i originally wrote were clear:
if un_callable_states:
raise AssertionError('The following states are un-callable: {}'.format(un_callable_states)
but the coverage tool won't allow them, because the "if" clause is never called. and now that i wrote down my explanation, i see what to do and feel kinda dumb. please wait while i fix that. just a moment.
error_message = 'The following states are un-reachable: {}'.format(list(un_callable_states)) | ||
self.assertEqual(error_message, 'The following states are un-reachable: []') | ||
extra_info = 'The following states are un-reachable: {}'.format(un_callable_states) | ||
self.assertEqual(un_callable_states, set(), msg=extra_info) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense :D 👍
# (7, D, 15, C), states 7 or 9 | ||
(8, C, 2, C), | ||
(8, D, 4, D), | ||
# (9, C, 15, D), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm hoping to work through this properly today, can these commented out states simply be removed from the docstring or do we need them here for completeness?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the catch! i forgot them, and they need to be changed to tuples regardless. i'll await a decision and act on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's remove them
This looks good to me but will leave @marcharper to confirm as he's started looking at it (I'm fine for this to be merged Marc). |
I believe this PR addresses a bug present in With >>> axl.seed(0)
>>> C, D = axl.Actions.C, axl.Actions.D
>>> transitions = [(0, C, 0, C), (0, D, 3, C),
... (1, C, 5, D), (1, D, 0, C),
... (2, C, 3, C), (2, D, 2, D),
... (3, C, 4, D), (3, D, 6, D),
... (4, C, 3, C), (4, D, 1, D),
... (5, C, 6, C), (5, D, 3, D),
... (6, C, 6, D), (6, D, 6, D),
... (7, C, 7, D), (7, D, 5, C)]
>>> pair = (axl.FSMPlayer(transitions=transitions), axl.Random())
>>> match = axl.Match(pair, turns=5)
>>> match.play()
---------------------------------------------------------------------------
KeyError Traceback (most recent call last)
<ipython-input-3-e0ed4b66431c> in <module>()
11 pair = (axl.FSMPlayer(transitions=transitions), axl.Random())
12 match = axl.Match(pair, turns=20)
---> 13 match.play()
/home/vince/anaconda3/envs/moran/lib/python3.6/site-packages/axelrod/match.py in play(self)
119 while turn < self.turns:
120 turn += 1
--> 121 self.players[0].play(self.players[1], self.noise)
122 result = list(
123 zip(self.players[0].history, self.players[1].history))
/home/vince/anaconda3/envs/moran/lib/python3.6/site-packages/axelrod/player.py in play(self, opponent, noise)
166 def play(self, opponent, noise=0):
167 """This pits two players against each other."""
--> 168 s1, s2 = self.strategy(opponent), opponent.strategy(self)
169 if noise:
170 s1, s2 = self._add_noise(noise, s1, s2)
/home/vince/anaconda3/envs/moran/lib/python3.6/site-packages/axelrod/strategies/finite_state_machines.py in strategy(self, opponent)
73 return self.initial_action
74 else:
---> 75 action = self.fsm.move(opponent.history[-1])
76 # Record the state for testing purposes, this isn't necessary
77 # for the strategy to function
/home/vince/anaconda3/envs/moran/lib/python3.6/site-packages/axelrod/strategies/finite_state_machines.py in move(self, opponent_action)
29 def move(self, opponent_action: Action) -> Action:
30 """Computes the response move and changes state."""
---> 31 next_state, next_action = self.state_transitions[(self.state, opponent_action)]
32 self.state = next_state
33 return next_action
KeyError: (None, 'D') With current >>> axl.seed(0)
>>> C, D = axl.Actions.C, axl.Actions.D
>>> transitions = [(0, C, 0, C), (0, D, 3, C),
... (1, C, 5, D), (1, D, 0, C),
... (2, C, 3, C), (2, D, 2, D),
... (3, C, 4, D), (3, D, 6, D),
... (4, C, 3, C), (4, D, 1, D),
... (5, C, 6, C), (5, D, 3, D),
... (6, C, 6, D), (6, D, 6, D),
... (7, C, 7, D), (7, D, 5, C)]
>>> pair = (axl.FSMPlayer(transitions=transitions), axl.Random())
>>> match = axl.Match(pair, turns=5)
>>> match.play()
[('C', 'D'),
('C', 'D'),
('C', 'C'),
('D', 'C'),
('C', 'D')] |
I might throw that in as a test so we know it doesn't reappear. |
I found a bug working with v2.8.0, but Axelrod-Python#956 seems to have clear it up. I'm just adding a test to confirm it.
I've opened #967 |
#884
(that last commit should read "fixed type_hints". resolving a merge when two people had done almost the same work at almost the same time, was an ... unexpected surprise)
did a lot of fixing to FSM and its testing strategy. it looked like this should be its own PR separate from the other three i was planning to group it with. there are some issues with some of the FSMPlayers. They have states that are impossible to reach.
I tried to come up with a decent testing strategy, and need another opinion(s).