Simplify moran mutate function #1460
Conversation
| if self.mutation_rate > 0: | ||
| # Choose another strategy at random from the initial population | ||
| r = self._random.random() | ||
| if r < self.mutation_rate: | ||
| s = str(self.players[index]) | ||
| j = self._random.randrange(0, len(self.mutation_targets[s])) | ||
| p = self.mutation_targets[s][j] | ||
| return p.clone() | ||
| # Just clone the player | ||
| return self.players[index].clone() | ||
| if self._random.random() > self.mutation_rate: |
There was a problem hiding this comment.
I believe but I might be remembering incorrectly that we had it this way to avoid sampling a random number if no such sample was needed. I think this helped with performance.
There was a problem hiding this comment.
Did some quick tests, and I'm seeing that drawing a random number takes 1.4 microseconds, while an if takes 2.1 microseconds. We actually reduce an if when mutation_rate = 0, so we may be saving time. But moreover, it doesn't seem like a meaningful optimization.
I'm not sure, though. Should we add a performance test?
There was a problem hiding this comment.
I'm not sure, though. Should we add a performance test?
Yeah sounds like a good idea to be sure. Running a reasonably sized tournament perhaps? (I have played around with https://pypi.org/project/pytest-benchmark/ which is a neat way to get benchmarks but I have no idea if this is the sort of thing you were thinking: whatever you think is best :) -- no need to do a huge amount for this PR)
There was a problem hiding this comment.
Note that the class now uses the bulk random number generator, which should be faster at scale. This came later and may have altered the tradeoffs.
There was a problem hiding this comment.
I will add a benchmark in another change, coming soon.
| @@ -122,20 +122,6 @@ def __init__( | |||
| self._random = RandomGenerator(seed=seed) | |||
| self._bulk_random = BulkRandomGenerator(self._random.random_seed_int()) | |||
| self.set_players() | |||
| # Build the set of mutation targets | |||
There was a problem hiding this comment.
This code was first added here. The idea is that a player can't mutate into the same type if the random pull dictates a mutation (that's the case if the if clause fails). Typically in the literature for the Moran process when mutating one only chooses from the set of initial types (regardless of their prevalence). So the point of the mutation_targets dict is to (1) remove duplicate types from the initial population, and (2) remove the current type. Later, when you want to mutate a player, you can just choose uniformly at random from the mutation dictionary's possibilities, i.e. we've cached all the logic for the choices. (Note this assumes that no new player types are formed, that's a different kind of mutation.)
This is not well explained in the comments, and also mutation_targets might be better named mutation_possibilities or something else. In any case I think the logic of the current code is correct.
There was a problem hiding this comment.
Okay, I missed a subtlety that this was deduping initial_strategies. I changed the code to create a single list of mutation_targets and leave the responsibility of choosing a different type to the mutate function. A little closer now to the original code, but still (imo) more easily to understand.
There was a problem hiding this comment.
But isn't this less efficient? If the number of initial types is small (which is commonly the case) then the updated code may call for a lot of extra random numbers. The existing code removes the need to check for the same type. Agreed that it could use some cleaning up and clarification though.
The algorithm is supposed to, for given mutation rate \mu, chose another type at random with probability \mu / (n-1) for the other (n-1) types and (1-\mu) for the same type. Maybe there's a simpler/clearer way to do this without sacrificing efficiency.
There was a problem hiding this comment.
At most it would double (on avg) the number of randoms drawn. My gut says this is insignificant. I will add a performance test to see.
|
Top table before change, bottom table after change. Looks like there is significant performance regression. I will close the PR. Will follow-up with:
|
|
Thanks for looking into it, let's definitely update the comments to better explain what's happening and why. And maybe exiting early on |
Makes a few changes for readability:
mutation_targetsin the init.Many tests are failing because we've changed how many random numbers get drawn and when. If the code looks good to a reviewer, I'll update the tests.