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

Create fingerprint.py and a class for Ashlocks Fingerprint #759

Merged
merged 65 commits into from
Nov 18, 2016

Conversation

theref
Copy link
Member

@theref theref commented Nov 5, 2016

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.

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.

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)
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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:
Copy link
Member

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)
Copy link
Member

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):
Copy link
Member

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():
Copy link
Member

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):
Copy link
Member

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.

@drvinceknight
Copy link
Member

@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).

@drvinceknight
Copy link
Member

drvinceknight commented Nov 5, 2016

@theref if you rebase this on to you branch for #758 you'll have the transformers here which will help the review (and merging this will automatically merge #758).

(If you need a hand with this let me know.)

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:
Copy link
Member

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.
Copy link
Member

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")
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 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
Copy link
Member

Choose a reason for hiding this comment

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

Square?

Copy link
Member

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()
Copy link
Member

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.

Copy link
Member

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.
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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.

Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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`.
"""
Copy link
Member

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)()
Copy link
Member

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.

Copy link
Member Author

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?

Copy link
Member

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)!

Copy link
Member

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.

Copy link
Member

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?).

Copy link
Member

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
Copy link
Member

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
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 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,
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 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()),
Copy link
Member

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()
Copy link
Member

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):
Copy link
Member

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):
Copy link
Member

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):
Copy link
Member

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):
Copy link
Member

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
@meatballs
Copy link
Member

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):
Copy link
Member

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):
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 as for fingerprint method

Copy link
Member

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.)

Copy link
Member Author

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.

Copy link
Member

Choose a reason for hiding this comment

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

OK!

@meatballs
Copy link
Member

and now it seems too late to try to join.

@marcharper I don't think so!

@theref
Copy link
Member Author

theref commented Nov 17, 2016

I will be posting some plots on here soon, I'll also look into which interpolation looks the 'best'

@drvinceknight
Copy link
Member

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 interpolation variable to fingerprint.plot which would pass it to imshow, so the user can use whichever method they want.

@theref
Copy link
Member Author

theref commented Nov 17, 2016

don't need to make a choice for the purpose of the code right?

Yeah, I've put the functionality in and given it a default of 'none'... which is different to None...not seen that before

@theref
Copy link
Member Author

theref commented Nov 18, 2016

No interpolation on these plots, it's just to demonstrate that they are finally doing what they're supposed to

WSLS
screen shot 2016-11-18 at 10 52 24
screen shot 2016-11-17 at 22 03 13

@theref
Copy link
Member Author

theref commented Nov 18, 2016

Majority
screen shot 2016-11-17 at 22 04 19
screen shot 2016-11-17 at 21 04 51

@theref
Copy link
Member Author

theref commented Nov 18, 2016

TFT
screen shot 2016-11-17 at 22 01 51
screen shot 2016-11-17 at 21 04 30

@drvinceknight
Copy link
Member

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'):
Copy link
Member

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
Copy link
Member

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.
Copy link
Member

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)
Copy link
Member

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)
Copy link
Member

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).
Copy link
Member

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
Copy link
Member

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
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 as previously

----------
probes : list
A list of `JossAnnTransformer` players with
parameters that correspond to (x, y).
Copy link
Member

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
Copy link
Member

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
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 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()`
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 no longer correct (interp_method has been changed to interpolation).

@meatballs meatballs merged commit eff8b53 into Axelrod-Python:master Nov 18, 2016
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.

5 participants