-
Notifications
You must be signed in to change notification settings - Fork 269
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
Create fingerprint.py and a class for Ashlocks Fingerprint #759
Conversation
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.
My initial thoughts. As for #758 this will be easier to review once docs and tests are added (and indeed easier to refactor :)).
coordinates = list(product(np.arange(0, 1, granularity), | ||
np.arange(0, 1, granularity))) | ||
|
||
probe_coords = OrderedDict.fromkeys(coordinates) |
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 understand why this needs to be an OrderedDict. Would a simple dictionary not do?
def create_probes(self, probe, granularity): | ||
"""Creates a set of probe strategies over the unit square. | ||
|
||
Construncts probe strategies that correspond to (x, y) coordinates. The |
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.
Constructs
index=pd.MultiIndex.from_tuples(coord_scores.keys())) | ||
data_frame = ser.unstack().fillna(0) | ||
data_frame.shape | ||
return data_frame |
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.
A pandas data frame is an amalgamation of dictionaries and numpy, I suggest that in this particular case, just having the data as a dictionary mapping tuples to values would be simpler (and removes the issue of needing to add pandas as a dependancy for the library).
|
||
coord_scores = OrderedDict.fromkeys(probe_coords) | ||
for index, coord in enumerate(coord_scores.keys()): | ||
if sum(coord) > 1: |
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.
Perhaps a short inline comment describing what's happening with the two cases here (playing the dual or not).
http://matplotlib.org/examples/color/colormaps_reference.html | ||
""" | ||
self.data = self._generate_data(self.results, self.probe_players.keys()) | ||
sns.heatmap(self.data, cmap=col_map) |
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.
The choice of Default need to be carefully thought through.
For @marcharper and @meatballs: @theref has created a known fingerprint using all of the colormaps available. They don't all show the characteristics described in the paper so we need to pick a good default.
self.strategy = strategy | ||
self.probe = probe | ||
|
||
def create_probe_coords(self, granularity): |
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.
granularity
should be steps
.
An increase of granularity here would imply a less precise image/fingerprint but that's not usually what is meant by granularity.
from collections import OrderedDict | ||
|
||
|
||
class Fingerprint(): |
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.
A docstring will be necessary.
pass | ||
|
||
|
||
class AshlockFingerprint(Fingerprint): |
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.
A docstring will be necessary.
@marcharper, @meatballs and anyone else: to give some context (@theref and I have been working on this): This implements a general fingerprint class and a particular fingerprint class as defined by Ashlock. Ashlock's fingerprint is in fact an analytical function so this is actually a simulation of Ashlock's fingerprint. Because of that, it's actually an extension of Ashlock's fingerprint as it can be used on any strategy (and not just finite state machines strategies). @theref , @Nikoleta-v3 and I are working on some of the theoretic basis for that which at some point will need to be referenced but the basic idea is: if you want to fingerprint S, you need to play S against a strategy that depends on coordinates in the unit square (either a Joss-Ann transformation or the Dual of the Joss-Ann, depending on where you are in the unit square). |
for index, coord in enumerate(probe_dict.keys()): | ||
# Add 2 to the index because we will have to allow for the Strategy | ||
# and it's Dual | ||
if sum(coord) > 1: |
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.
An inline comment here to explain the two cases briefly perhaps.
|
||
Construncts edges that correspond to the probes in `probe_dict`. Probes | ||
whose coordinates sum to less/more than 1 will have edges that link them | ||
to 0/1 correspondingly. |
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.
Clarify that 0 correspond to the Joss Ann and 1 to the Dual of the Joss Ann.
spatial_tourn = axl.SpatialTournament(tourn_players, turns=turns, | ||
repetitions=repetitions, | ||
edges=self.edges) | ||
print("Begin Spatial Tournament") |
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.
No need for these print statements, pass a progress_bar
argument to the play
method (and to the fingerprint
method).
"""Creates a set of coordinates over the unit square. | ||
|
||
Construncts (x, y) coordinates that are separated by a step equal to | ||
`granularity`. The coordinates are over the unit squeare which implies |
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.
Square?
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.
Constructs
""" | ||
self.data = self._generate_data(self.results, self.probe_players.keys()) | ||
sns.heatmap(self.data, cmap=col_map) | ||
plt.show() |
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.
Well I believe @drvinceknight covered some of my issues. Such as the print and the order dictionary. Looking forward to the tests. My only further comment is that the documentation could be better, I am losing you at some parts.
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.
@Nikoleta-v3, if you could point out parts that you found unclear this would be helpful. :) 👍
Parameters | ||
---------- | ||
strategy : class | ||
A class that must be descended from axelrod.strategies. |
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.
axelrod.strategies
is a list. I think you mean axelrod.Player
"""Creates a set of coordinates over the unit square. | ||
|
||
Construncts (x, y) coordinates that are separated by a step equal to | ||
`granularity`. The coordinates are over the unit squeare which implies |
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.
Constructs
edges : list of tuples | ||
A list containing tuples of length 2. All tuples will have either 0 | ||
or 1 as the first element. The second element is the index of the | ||
corresponding probe (+2 to allow for including the Strategy and it's |
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.
+2 is not clear to me when I read this.
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.
also, its not it's
---------- | ||
data_frame : pandas.core.frame.DataFrame | ||
A pandas DataFrame object where the row and column headers | ||
correspond to coordinates. The cell values are the score of the |
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.
Once you remove pandas, maybe some of the information of the details you are keeping and creating this table can go higher up.
Parameters | ||
---------- | ||
granularity : float | ||
The seperation between each coordinate. Smaller granularity will |
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.
separation
def create_probe_coords(self, granularity): | ||
"""Creates a set of coordinates over the unit square. | ||
|
||
Construncts (x, y) coordinates that are separated by a step equal to |
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.
Constructs
probe_coords : ordered dictionary | ||
An Ordered Dictionary where the keys are tuples representing each | ||
coordinate, eg. (x, y). The value is automatically set to `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.
If the values are None
at this point, why bother returning a dictionary? You could simply return a list of tuples and use it later to build your dictionary once you have the actual values available.
if x + y > 1: | ||
probe_dict[coord] = JossAnnTransformer((1 - y, 1 - x))(probe)() | ||
else: | ||
probe_dict[coord] = JossAnnTransformer((x, y))(probe)() |
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 looks like a function that needs to be pulled out so that it can be tested. I also suspect that would allow the dictionary to be built using a comprehension.
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.
Further down you say:
I wonder if some of the methods in here might be better as module level functions. They could then be tested in complete isolation without the need to create a fingerprint instance.
For the new function you would like created: is this an example of where having the function inside the class doesn't actually gain us anything and would be easier to test if it was a module level function?
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.
In my opinion, yes. (But this sort of thing is always a matter of opinion)!
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.
For me, the clue in this one is that it doesn't use self
anywhere within the method.
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.
In my opinion, yes. (But this sort of thing is always a matter of opinion)!
I'm happy for this to be popped outside so that it's at the module level too: particularly useful (as @meatballs says) for testing.
From a user pov, as it's not really user facing, it doesn't really need to be in the class either (it's going to be called by other methods that the user will use right?).
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 reckon our good friend @brandon-rhodes puts this all rather nicely in this talk: https://www.youtube.com/watch?v=DJtef410XaM
repetitions : integer, optional | ||
The number of times the round robin should be repeated | ||
granularity : float, optional | ||
The seperation between each coordinate. Smaller granularity will |
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.
separation
granularity : float, optional | ||
The seperation between each coordinate. Smaller granularity will | ||
produce more coordinates that will be closer together. | ||
cores : integer, optional |
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.
can we call this processes
to keep in line with match parameter?
dual = DualTransformer()(self.strategy)() | ||
probes = self.probe_players.values() | ||
tourn_players = [original, dual] + list(probes) | ||
spatial_tourn = axl.SpatialTournament(tourn_players, turns=turns, |
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.
Can we have full, meaningful names rather than these abbreviations?
edge = (0, index + 2) | ||
coord_scores[coord] = edge_scores[edge] | ||
|
||
ser = pd.Series(list(coord_scores.values()), |
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.
again, full, meaningful variable name, please
""" | ||
self.data = self._generate_data(self.results, self.probe_players.keys()) | ||
sns.heatmap(self.data, cmap=col_map) | ||
plt.show() |
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 wonder if some of the methods in here might be better as module level functions. They could then be tested in complete isolation without the need to create a fingerprint instance.
Fixes a few words that were spelt wrong in the docstrings, also changes `granularity` to `step` as requested by Vince. See: Axelrod-Python#759
Move the logic regarding the construction of a Joss Ann probe player to a new function outside the class. Creating the new function makes the code more readable. Moving the function outside the class will help with testing. PR: Axelrod-Python#759
Creats tests for `create_probe_coords` and `create_probes`. Forces some refactoring of `create_probe_coords` by expecting only a list to be returned instead of a dictionary.
Changes `create_probe_coords` to only return a list of coordinates instead of an OrderedDict. In turn `create_probes` now accepts a list of coordinates as a parameter instead of a dictionary.
Provides a test for `create_edges` but changes the parameters to make it accept a list of coordinates instead of a dictionary.
self.strategy = strategy | ||
self.probe = probe | ||
|
||
def create_probe_coords(self, step): |
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 method looks like another candidate for being a module level function.
np.arange(0, 1, step))) | ||
return coordinates | ||
|
||
def create_probes(self, probe, probe_coords): |
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 method looks like another candidate for being a module level function.
for coord in probe_coords) | ||
return probe_dict | ||
|
||
def create_edges(self, probe_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.
This method looks like another candidate for being a module level function.
edges.append(edge) | ||
return edges | ||
|
||
def fingerprint(self, turns=50, repetitions=10, step=0.01, processes=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.
This method has a lot of side-effects but returns nothing. That makes it tricky to test and even more difficult to debug when things don't work as expected.
I think it needs refactoring into one or more functions/methods with defined return values, tests, and no side-effectd.
This moves the construction of `edges` and `tournament_players` to a new function called `construct_tournament_elements`.
A few functions within AshlockFingerprint did not need to be contained within the class so they have been moved outside. All the relevant tests have updated as well. PR: Axelrod-Python#759
Change `generate_data` to return dictionary `generate_data` used to return a Pandas data frame but instead returns a dictionary that maps coordinates to scores. This commit also adds tests for `generate_data`. PR: Axelrod-Python#759
`generate_data` used to return a Pandas data frame but instead returns a dictionary that maps coordinates to scores. This commit also adds tests for `generate_data`. PR: Axelrod-Python#759
Several of these functions (which look a lot nicer now 😄 ) return lists of similar looking tuples. Might a named tuple be worthwhile here? |
|
||
return edges, tournament_players | ||
|
||
def fingerprint(self, turns=50, repetitions=10, step=0.01, processes=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.
Every other function and method includes a verb as the first part of its name (e.g. 'create' or 'construct'). Could we do the same here for consistency (or remove them elsewhere).
|
||
return coord_scores | ||
|
||
def plot(self, col_map=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.
Same comment as for fingerprint method
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.
Could plot and fingerprint not be interpreted as a verb?
(I'm not overly bothered either way.)
(We also have all the plot methods in the Plot
class which are just called by the name of the type of plot, so keeping it as plot
or fingerprint
would be in line with that.)
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.
Could plot and fingerprint not be interpreted as a verb?
(We also have all the plot methods in the Plot class which are just called by the name of the type of plot, so keeping it as plot or fingerprint would be in line with that.)
This is what I thought, I'd be in favour of keeping it as is.
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.
OK!
@marcharper I don't think so! |
I will be posting some plots on here soon, I'll also look into which interpolation looks the 'best' |
Great will be neat to look at: but you don't need to make a choice for the purpose of the code right? You can just let the user pass the |
Yeah, I've put the functionality in and given it a default of |
New plot demonstrates the ability to pass a colour map and interpolation.
I'm happy. To give some context for the Majority image: that's a screengrab from the original article, it's a strategy for which you cannot get an anaytical function and is plotted with a different orientation/ordering of coordinates but a flip in the y=x axis gives the same image which is what we expect. (The @theref can you just confirm I've got that right). |
self.data = self.generate_data(self.interactions, self.points, edges) | ||
return self.data | ||
|
||
def plot(self, col_map='seismic', interp_method='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.
The tiniest of things (not a blocker for me). Do we want interp_method
to be interpolation
so that's it's the same variable name as for imshow
?
Not a blocker for me, happy to let @meatballs and/or @marcharper have the final say on that.
Parameters | ||
---------- | ||
step : float | ||
The separation between each coordinate. Smaller steps will |
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.
point rather than coordinate
---------- | ||
step : float | ||
The separation between each coordinate. Smaller steps will | ||
produce more coordinates that will be closer together. |
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.
points
Returns | ||
---------- | ||
points : list of Point | ||
Named Tuples of length 2 representing each coordinate (x, y) |
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.
points : list
of Point objects
Parameters | ||
---------- | ||
point : Point | ||
has coordinates (x, y) |
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.
get rid of the description line. It adds nothing and is a source of confustion
Returns | ||
---------- | ||
joss_ann: Joss-AnnTitForTat object | ||
`JossAnnTransformer` with parameters that correspond to (x, y). |
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.
point rather than (x, y)
def create_probes(self, probe, points): | ||
"""Creates a set of probe strategies over the unit square. | ||
|
||
Constructs probe strategies that correspond to (x, y) coordinates. The |
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.
points rather than coordinates
---------- | ||
probe : class | ||
A class that must be descended from axelrod.strategies. | ||
points : list of Points |
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.
same comment as previously
---------- | ||
probes : list | ||
A list of `JossAnnTransformer` players with | ||
parameters that correspond to (x, y). |
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.
point rather than (x, y)
Parameters | ||
---------- | ||
interactions : dictionary | ||
A dictionary of the interactions of a tournament |
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.
a dictionary mapping what to what? This could do with some more clarity here.
---------- | ||
interactions : dictionary | ||
A dictionary of the interactions of a tournament | ||
points : list of Points |
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.
same comment as previously
:align: center | ||
|
||
We are also able to specify a matplotlib colour map and interpolation. Passing | ||
parameters :code:`col_map='PuOr', interp_method='bicubic'` to the :code:`plot()` |
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 no longer correct (interp_method
has been changed to interpolation
).
This relies on #746.
Again, no tests written but I wanted to check the code written was acceptable first. I know @drvinceknight has a few things that could be changed but wanted the discussions to take place on here.