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

Moran plot #1122

Merged
merged 6 commits into from
Aug 23, 2017
Merged

Moran plot #1122

merged 6 commits into from
Aug 23, 2017

Conversation

marcharper
Copy link
Member

Add a plot for the Moran process. Closes #1048 .

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.

Looks great to me! A couple of minor requests.

@@ -385,6 +386,28 @@ def __len__(self) -> int:
"""
return len(self.populations)

def populations_plot(self, ax=None):
Copy link
Member

Choose a reason for hiding this comment

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

Could we just have a docstring?

Something like (but please modify/improve):

Plot the population distribution over iterations of the Moran process.

Parameters
----------------
    ax: a matplotlib axis
       Allows to plot this on a given matplotlib axis. Default is None.

Returns
-----------
    A matplotlibt axis object

>>> import matplotlib.pyplot as plt
>>> axl.seed(15) # for reproducible example
>>> N = 10
>>> players = [random.choice(axl.basic_strategies)() for i in range(N)]
Copy link
Member

Choose a reason for hiding this comment

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

Could we hard code a list of strategies please. Otherwise, if and when basic_strategies changes (with new strategies being added) this would not necessarily give the same plot.

>>> mp.winning_strategy_name
'Tit For Tat'
>>> ax = mp.populations_plot()
>>> plt.show() #doctest: +SKIP
Copy link
Member

Choose a reason for hiding this comment

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

No need to skip this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually the test hangs and all the windows stay open otherwise, something to do with plt not exiting perhaps because the plot object isn't gc'd.

Copy link
Member

Choose a reason for hiding this comment

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

gc'd (curious)?

No problem to skip.

Copy link
Member Author

Choose a reason for hiding this comment

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

garbage collected

# Test that can plot on a given matplotlib axes
axelrod.seed(15)
N = 5
players = [random.choice(axelrod.basic_strategies)() for _ in range(N)]
Copy link
Member

Choose a reason for hiding this comment

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

Same comment about the basic_strategies would this potentially change the axes in lines 345, 346 if and when new strategies come in?

For the purpose of the test let's just use demo_strategies (or any other hard coded list: perhaps just a cooperator and a defector?)

axelrod.seed(15)
N = 5
players = [random.choice(axelrod.basic_strategies)() for _ in range(N)]
mp = axelrod.MoranProcess(players=players, turns=200)
Copy link
Member

Choose a reason for hiding this comment

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

For the purpose of the test can we reduce turns? (for run time?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure but there's a much longer test in the moran_test.py file.

Copy link
Member

Choose a reason for hiding this comment

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

Feel free to reduce that one too :)

mp.play()
fig, axarr = plt.subplots(2, 2)
ax = axarr[1, 0]
mp.populations_plot(ax=ax)
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 also have a test where we just call mp.populations_plot() (to test the default runs without a problem).

Copy link
Member

Choose a reason for hiding this comment

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

(This is where the coverage is missing)

@marcharper
Copy link
Member Author

FYI all the tests pass locally, looks like a timeout issue.

@drvinceknight
Copy link
Member

FYI all the tests pass locally, looks like a timeout issue.

Is this rebased on to master with the stuff I did on #1121 ?

self.assertEqual(ax.get_xlim(), (-0.8, 16.8))
self.assertEqual(ax.get_ylim(), (0, 5.25))

# Run with a give axis
Copy link
Member

Choose a reason for hiding this comment

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

# Run without a given axis (I guess)

self.assertEqual(ax.get_ylim(), (0, 5.25))

# Run with a give axis
mp.populations_plot()
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 test something.

Perhaps (to avoid adding to the previous plot):

plt.figure()
self.assertIsInstance(mp.populations_plot(), matplotlib.pyplot.Figure)  # I'm guessing?

>>> import matplotlib.pyplot as plt
>>> axl.seed(15) # for reproducible example
>>> N = 10
>>> players = [random.choice(axl.demo_strategies)() for i in range(N)]
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for changing this, the plot file now needs to be updated to run with these strategies (eg Suspicious Tit For Tat is not in demo_strategies). Another alternative is to just hard code the names of the players in that plot file (instead of random.choice(axl.demo_strategies)) either way they need to match.

Copy link
Member

Choose a reason for hiding this comment

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

I think I would prefer the hard coded approach as there's less code to (potentially) understand but your call. As long as they match up.

@drvinceknight
Copy link
Member

drvinceknight commented Aug 20, 2017

Is this rebased on to master with the stuff I did on #1121 ?

It's looking like it's not, the PR run worked fine (and the log shows the timeout wait call) but the push didn't. If you rebase on to master it should sort that out.

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.

Looks awesome. Thanks for the adjustments! 👍

@meatballs meatballs merged commit 1266335 into master Aug 23, 2017
@meatballs meatballs deleted the moran_plot branch August 23, 2017 08:30
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