Skip to content

Issue 138 - Adds functions to calculate the state distribution of a Match #717

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

Merged
merged 3 commits into from
Sep 26, 2016

Conversation

theref
Copy link
Member

@theref theref commented Sep 19, 2016

Related to #138 #701

Copy link
Member

@drvinceknight drvinceknight left a comment

Choose a reason for hiding this comment

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

Just the two minor docstring requests from me.

@@ -159,6 +159,12 @@ def normalised_cooperation(self):
"""Returns the count of cooperations by each player per turn"""
return iu.compute_normalised_cooperation(self.result)

def state_distribution(self):
return iu.compute_state_distribution(self.result)
Copy link
Member

@drvinceknight drvinceknight Sep 19, 2016

Choose a reason for hiding this comment

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

Could we have a brief docstring for these two methods (similar to the ones in the interactions_utils).

def compute_state_distribution(interactions):
"""
Returns the count of each state for a set of interactions.
"""
Copy link
Member

Choose a reason for hiding this comment

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

The docstring needs the full definition of the arguments and return value

"""
Returns the count of each state for a set of interactions.
"""
if len(interactions) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

This could just be if not interactions:

Copy link
Member Author

Choose a reason for hiding this comment

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

I just copied what was done in some of the other functions...should I change them all?

Copy link
Member

Choose a reason for hiding this comment

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

You don't need to change them all (unless you wanted to!) but if you could keep the new ones according the convention that would be much appreciated. We have #347 as an ongoing issue to back-fix the formatting.

def compute_normalised_state_distribution(interactions):
"""
Returns the normalized count of each state for a set of interactions.
"""
Copy link
Member

Choose a reason for hiding this comment

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

Docstring needs to define arguments and return value

"""
Returns the normalized count of each state for a set of interactions.
"""
if len(interactions) == 0:
Copy link
Member

Choose a reason for hiding this comment

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

if not interactions:

def state_distribution(self):
return iu.compute_state_distribution(self.result)

def normalised_state_distribution(self):
Copy link
Member

Choose a reason for hiding this comment

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

Docstring needed here


for key in normalized_count:
normalized_count[key] /= total
return normalized_count
Copy link
Member

Choose a reason for hiding this comment

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

this might be a bit more 'pythonic' with a comprehension. I've not tested it, but something like:

interactions_count = Counter(interactions)
total = sum(interactions_count.values(), 0.0)
# By starting the sum with 0.0 we make sure total is a floating point value,
# avoiding the Python 2 floor division behaviour of / with integer operands (Stack Overflow)

return Counter({key: value / total for key, value in interactions_count.items()})

@drvinceknight
Copy link
Member

The failure is not your fault @theref, it's hypothesis stumbling upon a particular case where it's a tie and due to floating point errors the names do not get ordered the same. This commit on my own branch should address that: 970b544

@drvinceknight
Copy link
Member

This commit on my own branch should address that: 970b544

I'm just about to open another PR that I think offers a better fix still.

@drvinceknight
Copy link
Member

@theref I've opened #725, you might want to grab the commit from there for now.

@drvinceknight
Copy link
Member

@theref that's now been merged in. If you merge or rebase on to master you should be good to go :) 👍

@drvinceknight drvinceknight merged commit d9b6147 into Axelrod-Python:master Sep 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants