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

Enum actions #816 #1052

Merged
merged 9 commits into from
Jul 1, 2017
Merged

Enum actions #816 #1052

merged 9 commits into from
Jul 1, 2017

Conversation

eric-s-s
Copy link
Contributor

There is still some work to do, but this should pass all tests. (i hope. i'll have to wait for TravisCI. i'm on windows). I tried to keep changes to an absolute minimum. I needed to add a test in test_resultset.py (it has a comment why it's there) and I wanted to check inputs on Human, so I added 4 tests in test_human.py .

work not done yet -

  • flip_action can be replaced by the action's .flip() method, but basic tests first.
  • it was mentioned moving random_choice to an Action method. I have not touched that yet.
  • the alias Action = Actions can now be removed. I didn't want to do it yet, because it affects the import statement of almost every module and there's more than enough code to look through already.
  • if this looks good, should the enum be "Action" or "Actions"?


def test_from_char_error(self):
self.assertRaises(UnknownAction, Actions.from_char, '')
self.assertRaises(UnknownAction, Actions.from_char, 'c')
Copy link
Member

Choose a reason for hiding this comment

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

Can we add 'd' as well?

@@ -210,8 +211,8 @@ def _write_interactions_to_file(self, results):
row = list(index_pair)
row.append(str(self.players[index_pair[0]]))
row.append(str(self.players[index_pair[1]]))
history1 = "".join([i[0] for i in interaction])
history2 = "".join([i[1] for i in interaction])
history1 = action_sequence_to_str([i[0] for i in interaction])
Copy link
Member

Choose a reason for hiding this comment

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

actions_to_str might be a better name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it certainly is.

@marcharper
Copy link
Member

Thanks for the contribution! This is looking pretty good to me. We have a 2 review policy for merges and some of us are traveling but we should be able to get this in soon.

I left a couple of comments and there's no major concerns from me on a first pass.

@eric-s-s
Copy link
Contributor Author

eric-s-s commented Jun 28, 2017

i was happy to do it. i'll push up those changes and a couple of minor things . oops. upon proof-reading, those minor things included several docstrings taht contained 'C' and 'D'. aside from docstrings, i just added tests for Actions.__str__ , and removed a self.maxDiff = None

@marcharper
Copy link
Member

marcharper commented Jun 30, 2017

Looks great to me! Only one suggestion: UnknownAction should be UnknownActionError since it's an exception. (I should have suggested this before).

@meatballs I approved it regardless of the suggestion above.

elif character == 'D':
return cls.D
else:
raise UnknownActionError('Character must be "C" or "D".')
Copy link
Member

Choose a reason for hiding this comment

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

How about using a dict here to make it a little more 'pythonic'? e.g.

actions = {
    'C': cls.C,
    'D': cls.D
}
try:
    return actions[character]
except KeyError:
    raise UnknownActionError('Character must be "C" or "D".')

Copy link
Member

Choose a reason for hiding this comment

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

And, doing it this way would remove the need for the UnknownActionError - it could just raise a KeyError with a message

Copy link
Member

Choose a reason for hiding this comment

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

That's how it was before I think, I suggested this way thinking it would be more efficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's pretty close. It originally caught the KeyError and raised a ValueError. Any way you slice it, it has catch the KeyError and raise a new error to have a useful message, so it could be whatever error we please.

So:

UnknownActionError('Character must be "C" or "D".') or ValueError('Character must be "C" or "D".') or KeyError('The only accepted action keys are "C" and "D".')

fyi, i got curious and timed it:
10000 loops of if/else = 4ms
10000 loops of dict = 5.5ms
so if/else saves about 150ns.

I don't have a strong feeling on this. i kind of prefer ValueError or UnknownActionError to KeyError, and dictionary to if/else, but that's about the extent of it. I'll happily go with "do it .... way, please." or I'll make the call if you wish. what shall i do?

Copy link
Member

Choose a reason for hiding this comment

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

AKA approx 1.5/5.5 * 100 = 27.3 % faster :)

Copy link
Member

@meatballs meatballs Jul 1, 2017

Choose a reason for hiding this comment

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

i kind of prefer ValueError or UnknownActionError to KeyError, and dictionary to if/else

Me too. It's disappointing (and suprising) that using a dict causes such a performance hit. Let's leave things as they are. I'll merge this now.

@meatballs
Copy link
Member

Nice! I've made one suggestion but, other than that, it looks good to go and a nice contribution.

return '{}'.format(self.name)

def flip(self):
"""Returns the opposite Action. """
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 believe i originally wrote this as also using a dict. Is it better to use the if statements or the dict? I changed it to follow the style of the function below.

    def flip(self):
        """Returns the opposite Action. """
        opposites = {Actions.C: Actions.D,
                     Actions.D: Actions.C}
        return opposites[self]

should i use this code instead?

@meatballs meatballs merged commit 1947fb0 into Axelrod-Python:master Jul 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants