-
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
Reconcile Moran processes #1043
Conversation
Use spatial Moran Processes throughout Also add ability to pass a prob_end to the Matches.
@@ -4,7 +4,7 @@ Moran Process on Graphs | |||
======================= | |||
|
|||
The library also provides a graph-based Moran process [Shakarian2013]_ with | |||
:code:`MoranProcessGraph`. To use this class you must supply at least one | |||
:code:`MoranProcess`. To use this class you must supply at least one |
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.
class --> feature since the class can be used without a graph.
axelrod/moran.py
Outdated
deterministic_cache=None, mutation_rate=0., mode='bd', | ||
match_class=Match): | ||
match_class=Match, interaction_graph=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.
Do we need match_class any longer?
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 don't think so, I'll take it out.
axelrod/moran.py
Outdated
interaction_graph = complete_graph(len(players), loops=False) | ||
if reproduction_graph is None: | ||
reproduction_graph = complete_graph(len(players)) | ||
if reproduction_graph is 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.
Is this the right default? I think we should consider using the same default in all cases.
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 the same default as it was implemented in MoranProcessGraph
but I'll change it. I've got a further idea for how I'd like to refactor this as well.
axelrod/moran.py
Outdated
@@ -124,15 +159,21 @@ def mutate(self, index): | |||
return new_player | |||
|
|||
def death(self, index=None): | |||
"""Selects the player to be removed. Note that the in the birth-death |
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 docstring is still relevant, no?
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.
Yup, not sure why I got rid of it.
One concern is that without the non-graph implementation we don't have a canonical implementation to compare to. We should consider adding some tests that really demonstrate that it is equivalent (e.g. explicitly tests the interaction pairs for various population sizes). |
Sure. I'm going to add to the tests a bit anyway, I'll try to incorporate your suggestion but I might need more guidance. 👍 |
- Add ability for a graph to add loops. - Ensure reproduction graph is same as inter graph - Remove Match class option from Moran process. - Uncomment property based test. Slipped by in a previous commit on this branch as a mistake. - Fix docstrings and add type hints. Addresses #808. Changed `fitness_proportionate_selection` to make mypy happy. - s/random.seed/axelrod.seed - Fix docs - Add numerous tests. No specific interactions tests: I feel this is fairly well covered now with some behaviour specifically tested as well.
I've just pushed a bunch of changes. Amongst other things I've added numpy docstrings and type hints. I've also gone through and added more unit type tests. I took a look at adding specific interaction tests but I'm not sure it's necessary:
I'm not at all averse to adding more tests if you're happy to spell them out for me (or perhaps they can be left as a further PR). 👍 |
@@ -389,14 +395,15 @@ class ApproximateMoranProcess(MoranProcess): | |||
Instead of playing the matches, the result is sampled | |||
from a dictionary of player tuples to distribution of match outcomes | |||
""" | |||
def __init__(self, players, cached_outcomes, mutation_rate=0.): | |||
def __init__(self, players: List[Player], cached_outcomes: dict, |
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.
We need all the parameters to pass through?
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.
Are you asking about the turns
prob_end
etc? In which case: no we don't. They are not used as all match results sampled from the cached_outcomes
.
If you're asking about the graphs
. I didn't want to tackle that here. It would be an easy thing to add.
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 would be an easy thing to add.
But let's do that at a later date.
mp = MoranProcess(players) | ||
populations = mp.play() | ||
self.assertEqual(len(mp), 9) | ||
self.assertEqual(len(populations), 9) | ||
self.assertEqual(populations, mp.populations) | ||
self.assertEqual(mp.winning_strategy_name, str(axelrod.Defector())) | ||
|
||
def test_standard_fixation(self): |
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.
Why remove this test?
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.
Because it was for a mock match class (it was a test for the match_class
attribute).
(It shouldn't have been called test_standard_fixation
)
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.
Actually it tested the constant fitness case (or else, that was the original intent).
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.
Most of the tests in here were testing multiple things (which I've tried to address). This test also tested that you could pass a different match class and has been removed because I've removed the match_class
attribute (as you suggested earlier).
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.
If you wanted to test constant fitness we can do that by passing a cache where all strategies have the same score.
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.
Actually, I don't understand what that test was doing at all with the Mock class (apart from testing that it worked). I've fixed the py3.6 problems and about to push with a version of this test (just using a normal match).
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 was returning a constant score (fitness) in a 2:1 ratio to see if the outcome was in the ballpark of what would be expected for an r=2 Moran process. This appears to be what the new test essentially does.
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.
Cool :) Let me know if you think there's anything else that we can add.
Graphs are undirected by default. Various intermediates such as the list of | ||
neighbors are cached for efficiency by the graph object. | ||
Graphs are undirected by default but you can pass :code:`directed=True` to | ||
create a directed graph.. Various intermediates such as the list of neighbors |
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.
..
Failures are py3.6 vs py3.5... Working on basically ensuring the dicts are ordered as expected. |
Use spatial Moran Processes throughout.
Also add ability to pass a prob_end to the Matches.
In terms of performance this actually seems faster:
output:
1 loop, best of 3: 8.7 s per loop
whereas current
master
:Output:
1 loop, best of 3: 9.92 s per loop
This complements #1042.