#884 lookerup and gambler #966

eric-s-s · 2017-04-08T07:02:01Z

I did a rather involved re-write of the re-write to Lookerup.

made keys to LookupTable a namedtuple, ActionKeys (could probably use a renaming)
moved a lot of functionality to new object LookupTable
made create_lookup_table_keys follow API
changed the "paramaters" parameter to require an ActionKeys and not a tuple.
refactored init and created helper functions for readablitiy
made a new function called lookup_table_display() to easily visualize information while writing tests.

ActionKeys.:
The keys to the lookup table are now the namedtuple, ActionKeys(self_plays, op_plays, op_openings). This will solve problems when moving to Actions(Enum) and even if the move never happens, removes reliance on knowledge of the value of Actions.C, Actions.D
LookupTable class:
All the functionality for creating a lookup table and getting information from it really was a class different from dictionary. This simplifies a lot of LookerUp init, error checking and strategy. It is also an immutable object (unless you want to muck around in private variables, but that's not my problem).
create_lookup_keys and API:
Every piece of API, uses (self_plays, op_plays, op_openings), but the original lookup_table keys used (op_openings, self_plays, op_plays). Having functions and inputs that relied on a different ordering for keys made this confusing. So i rewrote ActionKeys, LookupTable and create_lookup_keys to follow the API of everything else, which seemed like a more intuitive way to input information anyways. The nice advantage of a namedtuple and LookupTable obj is that this will now be easy to change, willynilly in the future.
Old data (in case you were wondering what happened to "3" above):
Once I changed how lookup_table keys were made, I had to change the order of all the set patterns. The original patterns are all hard-coded tested for in the tests. They are currently still in comment lines for ease of undoing the change that I did (and I have the original .csv file saved for easy reversal). Since create_action_keys has the same change, any newly created patterns from the axelrod dojo should work with the new API just fine.
The "parameters" parameter:
I changed "parameters" to using an ActionKeys instead of a tuple. This forces the explicit naming of each action. since ActionKeys is still treated as a tuple, old code using a tuple CAN be passed in without breaking anything (mypy should complain, but the code will still run)
refactored init:
the init function has a LOT of different things that needed to happen. It required a lot of comments, and took a while to read, and get the gist of everything it was doing. So i took all that functionality and made it pirvate helper functions. This helped expose some situations where bugs could crop up. Each of these helper functions is explicitly tested for, now. This, hopefully, made the init function easier to read.
lookup_table_display():
Using a named tuple of tuples for the lookup_table makes the repr hopelessly messy to read (but easy to change). i gathered that the point of changing the order of the inputs (see pt 2) was to make it easy to look up the next action on the lookup table. so i wrote this to make writing my tests easier. I then cleaned it up and wrote tests for it. And now moved it into LookupTable where it belongs. Even if the ActionKeys is renamed or re-ordered, the repr of a LookupTable.dictionary is now hopelessly noisy. the horrible dictionary repr. and the display functionSo this was written to address that disadvantage.

NOTE: lookup_table_display() relies on being able to sort actions. This could be an issue or it might be worthwhile anyways to create Actions.C < Actions.D

I hope these changes improve readability and readiness for change.

namedtuple=ActionKeys. All original data hard coded into tests and ready for change. next step is to change input oder.

…ails for look.

eric-s-s · 2017-04-08T11:08:52Z

you all might want to wait a day before looking at this. it occurs to me that this is slightly messy because LookupTable is most definitely a class by itself. i will push up some changes withing 24hrs.

drvinceknight · 2017-04-08T11:46:28Z

This has some implications in other places. For example https://github.com/Axelrod-Python/axelrod-dojo has some code that trains strategies so that would need to be adjusted.

Am I correct that the main motivation for this is the potential move to a ("real") class for actions?

i will push up some changes withing 24hrs.

Will wait to review properly :) 👍

eric-s-s · 2017-04-08T15:05:37Z

@drvinceknight thanks. good to know about the dojo. at a glance, it seems that this shouldn't affect that too much. it will create the keys in a different order now, but it will create the correct output (just in the new order). i was going to put "create_lookup_table_keys" inside the LookupTable class, but now see why i shouldn't.

…r or tuple. parameter names changed.

eric-s-s · 2017-04-08T19:45:45Z

edited the opening description to reflect the changes i just wrote.

@drvinceknight

Am I correct that the main motivation for this is the potential move to a ("real") class for actions?

That was a big part of the motivation. I also wanted to make this more ready for change of any kind. with the extra classes, different functionality is hidden from different parts and future changes should be easier.

and then, of course, i enjoy sussing out the hidden structure in something to find what it's trying to say. and writing the tests to show that. i teach one of my students some programming for fun, and i'm probably one of the few people to ever utter the sentence, "Today, I'm going to show you something really cool, unit tests!"

marcharper · 2017-04-09T18:45:21Z

On a quick look this all looks fine in principle. Since we don't comprehensively test the patterns I'd like to see a full tournament run on this PR for a sanity check comparison on the pattern translation.

The dojo should be fine but the saved patterns from our previous runs won't be valid anymore. (Not a big deal IMO.)

drvinceknight · 2017-04-09T19:18:34Z

On a quick look this all looks fine in principle. Since we don't comprehensively test the patterns I'd like to see a full tournament run on this PR for a sanity check comparison on the pattern translation.

Would you be able to do this @eric-s-s or would you like one of us to do it?

Here is the code with the tournament: https://github.com/Axelrod-Python/tournament but I'm happy to grab this branch and run a tournament on our University cluster tomorrow.

The dojo should be fine but the saved patterns from our previous runs won't be valid anymore. (Not a big deal IMO.)

I'm happy with this if you are @marcharper 👍

eric-s-s · 2017-04-10T06:09:57Z

@drvinceknight thank you for that offer. I would very much appreciate you running this on your university cluster. A single repetition of each took about 12 minutes on the family computer. This means about 20hrs of whirring CPU fan that would drive me up the wall (it's a loud fan.)

drvinceknight · 2017-04-10T08:19:33Z

I would very much appreciate you running this on your university cluster.

I'll get that running later today 👍

In the meantime here are two Ashlock fingerprints for EvolvedLookerUp_2_2_2:

Looks good to me (but will grab the full tournament run too).

marcharper · 2017-04-11T00:25:43Z

Is there a standard difference measure for the fingerprints? maybe mean squared difference on the pixel values?

drvinceknight · 2017-04-11T06:01:13Z

Is there a standard difference measure for the fingerprints? maybe mean squared difference on the pixel values?

We have the underlying data in the fingerprint class so yeah, we can just compare those. I'll do that and post back here later. 👍

eric-s-s · 2017-04-11T10:52:28Z

@drvinceknight that fingerprint module is pretty cool. i noticed some stuff as i was reading through. any objections to my taking a crack at some refactoring?

drvinceknight · 2017-04-11T11:06:13Z

i noticed some stuff as i was reading through. any objections to my taking a crack at some refactoring?

No objections, if it can be cleared up/improved that's always good. Feel free to open an issue if you're thinking of big changes in case it's worth discussing before you spend a lot of time on things.

drvinceknight · 2017-04-11T12:32:42Z

Sorry for taking a while to get this done but I've run the following on both master and this branch:

import axelrod as axl
import csv

def write_data_to_file(fp, filename):
    """
    Write the fingerprint data to a file.
    """
    columns = ['x', 'y', 'score']

    with open(filename, 'w') as f:
        w = csv.writer(f)
        w.writerow(columns)
        for key, value in fp.data.items():
            w.writerow([key.x, key.y, value])

lookerups = [axl.EvolvedLookerUp1_1_1, axl.EvolvedLookerUp2_2_2, 
             axl.Winner12, axl.Winner21]

axl.seed(0)
for lookerup in lookerups:
    af = axl.AshlockFingerprint(strategy=lookerup)
    _ = af.fingerprint()
    write_data_to_file(fp=af, filename="{}.csv".format(lookerup))

that generated the data available in the following tar file:

Comparing Lookerups.tar.gz

I then ran:

def make_dict(filename):
    """Make a dictionary from a data file"""

    with open(filename, "r") as f:
        reader = csv.reader(f)
        return {(x, y): score for x, y, score in reader}  

for lookerup in lookerups:
    fingerprint_on_master = make_dict("master/{}.csv".format(lookerup))
    fingerprint_on_884 = make_dict("884/{}.csv".format(lookerup))
    print(fingerprint_on_master == fingerprint_on_884)

The output is:

True
True
True
True

So I think we're ok to merge this in.

drvinceknight · 2017-04-11T12:36:47Z

So I think we're ok to merge this in.

If perhaps someone else could run the above to confirm (because of the branch change there's a manual step that I could have messed up). It doesn't take long.

eric-s-s · 2017-04-12T04:08:04Z

@drvinceknight i'll do those tests today.

eric-s-s · 2017-04-12T15:26:03Z

i've confirmed your fingerprint results, and i also included all the PSOGambler classes. they checked out too.

drvinceknight · 2017-04-12T17:07:30Z

On a quick look this all looks fine in principle. Since we don't comprehensively test the patterns I'd like to see a full tournament run on this PR for a sanity check comparison on the pattern translation.

Great. I think this is good to go @marcharper?

marcharper · 2017-04-13T00:51:01Z

How are the dictionaries equal? The plots are slightly different.

drvinceknight · 2017-04-13T05:57:21Z

The plots weren't done with the same seed. I seeded the dictionaries though.

…

On Thu, 13 Apr 2017, 01:51 Marc Harper, ***@***.***> wrote: How are the dictionaries equal? The plots are slightly different. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#966 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACCGWpwc62mf_kcWiXghC1l7zlCHtCjjks5rvXF1gaJpZM4M3nom> .

drvinceknight · 2017-04-13T06:12:57Z

axelrod/strategies/lookerup.py

-    parameters=Plays(self_plays=player_depth: int,
-                     op_plays=op_depth: int,
-                     op_openings=op_openings_depth: int).
+    | LookerUp can also be instantiated with pattern=str/tuple of actions, and


What's the pipe for here?

what is a pipe? you mean like in bash?

the docstrings use rst notation, correct? I thought it would be useful to preserve the line-breaks there. so i used the rst "| " notation. without the "hey rst! preserve linebreaks"-notation, it threw an error with one of the auto checks for having naughty indentation. should i change to "::" notation?

while i think of it, can i go ahead and delete all the commented-out original data?

I hadn't seen the | in rst before. I'm not sure it looks like it should though. Here's what the docs build looks like on this branch:

Is that what you were going for?

This tweak:

235 ~ 236 LookerUp can also be instantiated with :code:`pattern=str/tuple` of actions ~ 237 , and:: ~ 238 ~ 239 parameters=Plays( ~ 240 self_plays=player_depth: int, + 241 op_plays=op_depth: int, + 242 op_openings=op_openings_depth: int). 243 244 It will create keys of len=2 ** (sum(parameters)) and map the pattern to 245 the keys.

looks like:

while i think of it, can i go ahead and delete all the commented-out original data?

Sounds good to me. 👍

drvinceknight · 2017-04-13T06:28:03Z

Here are the plots with the same seed:

This branch:

Current master:

meatballs · 2017-04-13T12:04:48Z

If perhaps someone else could run the above to confirm (because of the branch change there's a manual step that I could have messed up). It doesn't take long.

I've repeated this and can confirm the result

drvinceknight

I think we're all happy? Marking this as good to go. :) Thanks @eric-s-s!

eric-s-s · 2017-04-14T16:43:45Z

@drvinceknight

Thanks @eric-s-s!

:)

eric-s-s added 8 commits April 5, 2017 00:03

changed dict keys from str's to namedtuple

ae50f9e

namedtuple=ActionKeys. All original data hard coded into tests and ready for change. next step is to change input oder.

formatted data for new input method

f2c11e5

refactor finished. must rewrite tests and include new functions

1326d62

main testing done. next is tests on specialized strategies

d355d38

lookerup and tests finished. test_gambler left

e8b97ab

added display function. fixed gambler memory_depth. full gambler tests

48303b8

double check. ready for PR

bc38716

changed Lookerup param, "parameters" to ActionKeys. changed minor det…

5a5dd16

…ails for look.

eric-s-s added 2 commits April 9, 2017 02:46

new class LookupTable. LookerUp init value, "pattern" can now take st…

a812d41

…r or tuple. parameter names changed.

added equality tests for LookupTable. ??fixed problem with docstring??

c03046d

changed ActionsKey to Plays. edited docstring.

2adf194

drvinceknight mentioned this pull request Apr 11, 2017

some ideas on refactoring fingerprint #969

Closed

eric-s-s added 2 commits April 13, 2017 00:30

120 chars changed to 80 chars

abe9ffc

fixed docstring formatting?

f2f1687

drvinceknight reviewed Apr 13, 2017

View reviewed changes

docstring cleanup. rename gambler variable

4524453

drvinceknight approved these changes Apr 14, 2017

View reviewed changes

drvinceknight added the ready-to-merge label Apr 14, 2017

meatballs merged commit 263a60f into Axelrod-Python:master Apr 16, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#884 lookerup and gambler #966

#884 lookerup and gambler #966

eric-s-s commented Apr 8, 2017 •

edited

eric-s-s commented Apr 8, 2017

drvinceknight commented Apr 8, 2017

eric-s-s commented Apr 8, 2017

eric-s-s commented Apr 8, 2017

marcharper commented Apr 9, 2017

drvinceknight commented Apr 9, 2017

eric-s-s commented Apr 10, 2017

drvinceknight commented Apr 10, 2017 •

edited

marcharper commented Apr 11, 2017

drvinceknight commented Apr 11, 2017

eric-s-s commented Apr 11, 2017

drvinceknight commented Apr 11, 2017

drvinceknight commented Apr 11, 2017

drvinceknight commented Apr 11, 2017

eric-s-s commented Apr 12, 2017

eric-s-s commented Apr 12, 2017 •

edited

drvinceknight commented Apr 12, 2017

marcharper commented Apr 13, 2017

drvinceknight commented Apr 13, 2017 via email

drvinceknight Apr 13, 2017

eric-s-s Apr 13, 2017 •

edited

drvinceknight Apr 13, 2017

drvinceknight commented Apr 13, 2017

meatballs commented Apr 13, 2017

drvinceknight left a comment

eric-s-s commented Apr 14, 2017 •

edited

#884 lookerup and gambler #966

#884 lookerup and gambler #966

Conversation

eric-s-s commented Apr 8, 2017 • edited

eric-s-s commented Apr 8, 2017

drvinceknight commented Apr 8, 2017

eric-s-s commented Apr 8, 2017

eric-s-s commented Apr 8, 2017

marcharper commented Apr 9, 2017

drvinceknight commented Apr 9, 2017

eric-s-s commented Apr 10, 2017

drvinceknight commented Apr 10, 2017 • edited

marcharper commented Apr 11, 2017

drvinceknight commented Apr 11, 2017

eric-s-s commented Apr 11, 2017

drvinceknight commented Apr 11, 2017

drvinceknight commented Apr 11, 2017

drvinceknight commented Apr 11, 2017

eric-s-s commented Apr 12, 2017

eric-s-s commented Apr 12, 2017 • edited

drvinceknight commented Apr 12, 2017

marcharper commented Apr 13, 2017

drvinceknight commented Apr 13, 2017 via email

drvinceknight Apr 13, 2017

Choose a reason for hiding this comment

eric-s-s Apr 13, 2017 • edited

Choose a reason for hiding this comment

drvinceknight Apr 13, 2017

Choose a reason for hiding this comment

drvinceknight commented Apr 13, 2017

meatballs commented Apr 13, 2017

drvinceknight left a comment

Choose a reason for hiding this comment

eric-s-s commented Apr 14, 2017 • edited

eric-s-s commented Apr 8, 2017 •

edited

drvinceknight commented Apr 10, 2017 •

edited

eric-s-s commented Apr 12, 2017 •

edited

eric-s-s Apr 13, 2017 •

edited

eric-s-s commented Apr 14, 2017 •

edited