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

Added AdaptiveTitForTat #697

Merged
merged 9 commits into from
Aug 24, 2016
Merged

Conversation

lukaszgajewski
Copy link

Hello,
I've implemented a basic version of Adaptive Tit For Tat based on this paper: http://users.softlab.ntua.gr/~brensham/Publications/SAB2000.pdf)
and some simple tests.
I'm not quite certain about memory-depth of the strategy though - on one hand it's looks at opponents last move (depth=1) on the other it has a continuous variable 'world' that in a peculiar way 'remembers' the whole game (depth=inf).
The paper also mentions a slightly extended model that I could try to implement as well, however, I'm not sure how to implement some of its features(like window or threshold) so that the class is user-friendly and universal ('window' parameter probably should depend on the number of round in a match and threshold somehow on the size of a window and so on). Also authors do not specify whether they mean 'n' move coincidences in a row within the window or simply within the window (I'd assume the latter).
Looking forward to your feedback,
L.

@marcharper
Copy link
Member

marcharper commented Aug 24, 2016

Thanks @l-liciniuslucullus !

  • Based on your description, I think the memory depth should be float("inf")
  • It looks you added the build directory to one of the commits, can you remove with git rm build/* and add another commit?
  • We'll probably need to squash the commits afterwards so that the build directory is never really added to the repository (I can help with this)
  • I'll take a look at the tests to see which is failing -- it's probably one of the doc tests that counts the strategies in the library.

I think your interpretation of the extended strategy is correct -- for a first pass, you could try setting the window to be 10 by default, and a reasonable threshold like 2 (using the example in the captions of figures 7 and 8). So if you want to give that a try that would be awesome :)

FYI: we can't always assume that the player will know the number of rounds (many tournaments vary this parameter and leave it unknown for theoretical reasons), so there needs to be a simple default for the window. I'm happy to help modify the strategy to use the number of rounds if it's known to the player (it's a little unusual but we have a few other strategies that use it).

@marcharper
Copy link
Member

So I think you can skip the squashing step -- I forgot that github lets us squash on merge now.

Sorry @ranjinidas -- that would have made your last PR much easier!

@init_args
def __init__(self, rate=0.5):

super().__init__()
Copy link
Member

Choose a reason for hiding this comment

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

This line is Python 3 only. We're currently also supporting Python 2.7, so we need to use Player.__init__(self) instead.

@lukaszgajewski
Copy link
Author

Build is apparently failing due to incompatibility with numpy-like doc:
https://groups.google.com/forum/#!msg/sphinx-users/MAFTlX8pAvk/B9hx7MuBVbIJ

@marcharper
Copy link
Member

marcharper commented Aug 24, 2016

The only other error is an overzealous docstring parsing issue (a warning treated as an error by Travis). Can we relax this somehow, @drvinceknight ?

Edit: Yep @l-liciniuslucullus , I'm paging @drvinceknight , he's the docs guru.

@marcharper
Copy link
Member

We'll probably also want a test or two that checks that self.world updates properly. Otherwise it's looking good! 👍

@@ -197,4 +197,5 @@
ZDGen2,
Copy link
Member

@drvinceknight drvinceknight Aug 24, 2016

Choose a reason for hiding this comment

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

Unless I'm missing it, you need to add AdaptiveTitForTat to the all_strategies list. I believe that might trigger a failure in the EDIT: docstrings doctests but that should just require incrementing a count :)

@drvinceknight
Copy link
Member

So I think you can skip the squashing step -- I forgot that github lets us squash on merge now.

I can't believe we didn't think of that 🙈 :) 👍 A good tactic to keep in mind :)

The only other error is an overzealous docstring parsing issue (a warning treated as an error by Travis). Can we relax this somehow, @drvinceknight ?

For the docs to compile we've removed the numpy docstring style guideline in the strategy docstrings (this is because it's automatically creating the html from the docstrings which end up looking like: http://axelrod.readthedocs.io/en/latest/reference/all_strategies.html#axelrod.strategies.titfortat.TitForTat so normal rst sections don't work).

If you could just remove all the ------------- that would fit with how we've done the rest of the strategy docs.

We'll probably also want a test or two that checks that self.world updates properly.

Could you also change how you have the reference at the end. You currently have:

References
----------
     http://users.softlab.ntua.gr/~brensham/Publications/SAB2000.pdf

To be in line with the way we're doing the rest of the strategies (currently working on this to update all the others) could you change that to:

Names

- Adaptive Tit For Tat: [Tzafestas2000]_

and add to https://github.com/Axelrod-Python/Axelrod/blob/master/docs/reference/bibliography.rst :

.. [Tzafestas2000] Tzafestas, E. (2000). Toward adaptive cooperative behavior. From Animals to Animals: Proceedings of the 6th International Conference on the Simulation of Adaptive Behavior {(SAB-2000)}, 2, 334–340.

This would then render like the following (with a link to the bibliography :)):

http://axelrod.readthedocs.io/en/latest/reference/all_strategies.html#axelrod.strategies.titfortat.TitForTat

@drvinceknight
Copy link
Member

If there's anything unclear about the docstrings: TitForTat is a good model to follow. :)

@marcharper
Copy link
Member

Thanks @drvinceknight , I didn't realize we were enforcing numpy style docstrings, now it makes sense!

p1.play(p2)
p1.play(p2)
self.assertEqual(p2.world, 0.75)

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 an extra test for the reset? Just to check that the value of the rate and world go back to the defaults?

If you call it test_reset it will overwrite the test here: https://github.com/Axelrod-Python/Axelrod/blob/master/axelrod/tests/unit/test_player.py#L163

So perhaps call it test_world_and_rate_reset (or something like that) :)

@drvinceknight
Copy link
Member

drvinceknight commented Aug 24, 2016

Thanks @drvinceknight , I didn't realize we were enforcing numpy style docstrings, now it makes sense!

No problemo 👍 😄

This is looking good @l-liciniuslucullus, I've just left two comments above, summarised as:

  1. Adding the strategy to the all_strategies list
  2. Adding a test that checks that the reset method resets world and rate

Thanks a bunch for this contribution :) 👍

@drvinceknight
Copy link
Member

This looks good to me! 👍 Don't forget to do a merge commit @marcharper :)

@marcharper marcharper merged commit b37cb12 into Axelrod-Python:master Aug 24, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants