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

N tit(s) for M tat(s) #1084

Merged
merged 12 commits into from
Jul 25, 2017
Merged

N tit(s) for M tat(s) #1084

merged 12 commits into from
Jul 25, 2017

Conversation

Chadys
Copy link
Member

@Chadys Chadys commented Jul 24, 2017

Resolve #1073 and add the NTitsForMTats asked for in #379

@drvinceknight
Copy link
Member

@Chadys looks like a doctest is failing:

File "./docs/tutorials/advanced/classification_of_strategies.rst", line 59, in classification_of_strategies.rst
Failed example:
    len(strategies)
Expected:
    28
Got:
    29

@Chadys
Copy link
Member Author

Chadys commented Jul 24, 2017

Yes I saw, NTitsForMTats is a memory one strategy if called with its default arguments, so I updated the returned number of memory one strategies in the example.

@drvinceknight
Copy link
Member

drvinceknight commented Jul 24, 2017

Yes I saw, NTitsForMTats is a memory one strategy if called with its default arguments, so I updated the returned number of memory one strategies in the example.

If I'm correct the default is TfT so I'd suggest not including this in axelrod.strategies (which is what I did for ReactivePlayer which doesn't actually have a default) just to avoid having two instances of TfT in there? (So in turn you would revert a49d448)

@Chadys
Copy link
Member Author

Chadys commented Jul 24, 2017

I thought all strategies had to be in axelrod.strategies, I changed it !

@drvinceknight
Copy link
Member

I thought all strategies had to be in axelrod.strategies,

Not quite :) This is similar to ReactivePlayer and LookerUp which are strategies that are intended to be used to create specific strategies so they're not added to the axelrod.strategies list. (And also the cheating strategies are not in there either.).

I'll review this properly tomorrow, logging off for the evening.

@marcharper
Copy link
Member

Can we add one instance to the library as a strategy, with say N=3 M=2 or similar?

@drvinceknight
Copy link
Member

drvinceknight commented Jul 25, 2017 via email

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.

Two very minor comments from me @Chadys. This looks excellent and good to go otherwise.

Just the other comment from @marcharper about including a default. I see two ways to go:

  • Make 3, 2 the default;
  • Include a single class 3TitsFor2Tats that's inherited. There are examples of this in the memory_one strategies where we have the generic memory one and LR player as well as a number of defaults.

I don't have a strong feeling either way on this :)

"""
Parameters
----------
N, int
Copy link
Member

Choose a reason for hiding this comment

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

Because of the type hints let's drop the type in the docstrings (in case at some point they are changed and we forget to change them in the docstrings).

if not self.M or opponent.history[-self.M:].count(D) == self.M:
self.retaliate_count = self.N
if self.retaliate_count:
self.retaliate_count-=1
Copy link
Member

Choose a reason for hiding this comment

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

PEP8: self.retaliate_count -= 1

@Chadys
Copy link
Member Author

Chadys commented Jul 25, 2017

All done, I went with the change in default values solution !

@drvinceknight
Copy link
Member

Looks good to me 👍

@meatballs meatballs merged commit e52b5cf into Axelrod-Python:master Jul 25, 2017
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.

None yet

4 participants