-
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 tournaments #1042
Reconcile tournaments #1042
Conversation
This is with a view to reconciling all tournament types in to one type of tournament.
Put all parameters needed for match generation in a single match generator: - edges - noise - turns - prob_end
Simply pass the parameters to Tournament: - edges - prob_end - turns - noise I had to adjust some things: - Refactor of property based tests (all the types still exists but they just call a tournament) - Refactor of fingerprint (just use the Tournament with an edges argument) - Refactor of some tests
Make minor modifications to the doctests.
This ensures that when repeating matches they resample the match length for each repetition.
I like this a lot! I'll have a proper look through the code.... |
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 suggesting a change rather than requesting one! Feel free to ignore
axelrod/match.py
Outdated
else: | ||
turns = sample_length(prob_end) | ||
elif turns is None: | ||
turns = 20 |
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 know it's my usual reaction to conditional statements but, especially with nesting, I'd be tempted to use a dict here. Something like:
turns_definition = {
(True, True): 20,
(True, False): sample_length(prob_end),
(False, True): turns,
(False, False): min(turns, sample_length(prob_end))
}
turns = turns_definition[(turns is None, prob_end 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.
I've just realised I need to move this logic anyway to the play
method (for the cache to work as we'd want), I'll see if I see a way of getting a dict to work there (although it'd need to be different to this so that it avoids unnecessary random samples). 👍
axelrod/tournament.py
Outdated
name: str = 'axelrod', game: Game = None, turns: int = 200, | ||
repetitions: int = 10, | ||
noise: float = 0, with_morality: bool = True) -> None: | ||
match_generator: MatchGenerator = MatchGenerator, |
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 match_generator still needed as a parameter?
Would it be better for Tournament to take a dictionary of match parameters?
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 match_generator still needed as a parameter?
I'd be happy to remove it. The only reason we really have it is to allow for the making new tournaments
idea: http://axelrod.readthedocs.io/en/stable/tutorials/advanced/making_tournaments.html
But that's such a niche thing I'd be happy to take it out. I'll take it out and we can see what we think :)
Would it be better for Tournament to take a dictionary of match parameters?
I don't think so. It's essentially a dictionary anyway (just kwargs
) and as is is more helpful to the user (autocompletion etc...).
axelrod/tournament.py
Outdated
self.edges = edges | ||
|
||
if turns is None and prob_end is None: | ||
turns = 200 |
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.
Maybe we should have a constant DEFAULT_TURNS somewhere
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.
Good idea.
Have change default match length to 200 in: - Matches - Tournaments - Moran Processes Adjusted Moran process tests as a result of it.
Also removed the tutorial on creating new ones. This could be added in again (potentially modifying it by showing how to create new tournaments).
All matches have both a turns and a prob_end attribute. Standard matches have a finite turns and a prob_end of 0. Prob end matches have an infinite number of turns and a prob_end > 0. The play method of the match is where a Match "decides" how many turns to play for. This ensures that the cache will work properly. This also implies that the len of an infinite match is simply not defined and raises an error. Also changed how the length of a prob end match is sampled: do not sample the length if not required.
Have included a test and also adjusted the tests for the fingerprints.
I've pushed a few more changes to this:
The other commits are minor things (@meatballs in 1c162eb I moved over to use a dictionary for the input selection instead of nested |
When they intended to be used as a constant (which python doesn't have) rather than an ordinary variable. This would be a good example of using shoutiness correctly! |
You've no idea how happy that makes me! |
Ok cool. I'll change it! |
- 1 warning in games.rst because I had actually pointed at incorrect label. - Other doctests needed to be fixed: - Some returned to prior state (they had been changed when I was sampling more often than necessary) - Changed a random seed due to default match length being 200.
More Pythonic this way.
Nice piece of work! |
LGTM but I do have one suggestion. For the tournament class |
This wouldn't really work as all the calculations for the result set are done as the interactions are read in (and due to memory constraints, the interactions are discarded). Invoking it on the result set would imply an entire reread of all the interactions. I agree that it's out of place. One option would be to make it no longer default and just have it as something that's always being calculated. I think having it as a optional is legacy. |
Another (perhaps better) option is not to make it part of the |
I'm hitting the button, let's table the morality discussion |
On #1042 @marcharper mentioned that it was perhaps worth moving this argument out of the Tournament.init. I took a look and turns out that this argument is not being used anymore (I'm not sure since when). Probably since the move to doing the result calculations on disk.
This would introduce backwards incompatibility at the Tournament call level. This PR would be for v3.0.0
Status quo:
To create a standard tournament:
To create a probabilistic ending tournament:
To create a standard spatial tournament:
To create a probabilistic ending tournament:
This PR changes that and removes the
ProbEndTournament
,SpatialTournament
,ProbEndSpatialTournament
classes so to create the above we would have:I've done this by carrying out various things:
MatchGenerator
to theMatch
class. The main consideration here is that I've consideredturns
andprob_end
as stopping conditions for a match:None
the other is used. (Ifturns
isNone
andprob_end
is notNone
then we have a probabilistic ending match)None
then a default of 20 turns is used. (Currently no default length is in place, I've picked 20 out of a hat, could use 200 to be in line with tournaments?)None
then both are used (so in essence we have a finite match of max lengthturns
but with a probability of finishing earlier).match_generator
module: it had 4 different generators (and 1 parent class), they have all been put in theMatchGenerator
class which passes the match parameters to theMatch
class. The one thing this does is create anedges
generator if noedges
are passed to it. In this case theedges
are the edges of the complete graph (so everyone plays everyone else).One final change is a potential fix of a bug. Currently, in a prob end tournament, the match length between any two strategies is randomly sampled and that sampled length is repeated. In cc4d537 I change this so that a new Match is created at each repetition which in turn means a new length is sampled. I've done this as a separate commit so that it's isolated but I feel that this should be the way it is (I've written a test for it too).
Advantages
Disadvantages
ProbEndSpatialTournament
etc no longer can). So this would be forv3.0.0
.Let me know what you think :)