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

ModalTFT, ModalDefector strategies - Ben McGuffog #1433

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from

Conversation

benjjo
Copy link

@benjjo benjjo commented Feb 7, 2024

Hi, I've added two strategies that are interesting (well at least I think). Here are their descritpions:

ModalTFT
A player starts by cooperating and then analyses the history of the opponent. If the opponent Cooperated in the
last round, they are returned with a Cooperation. If the opponent chose to Defect in the previous round,
then this strategy will return with the mode of the previous opponent responses.

ModalDefector
A player starts by Defecting and then analyses the history of the opponent. If the opponent Cooperated in the
last round, they are returned with a Defection. If the opponent chose to Defect in the previous round,
then this strategy will return with the mode of the previous opponent responses.

Interestingly, when pitted against a few of the classics from the orignal Axelrod tournement, they do quite well. When tested agianst all of the strategies in this repository, they rank 103 and 200 respectively.

I have never written code for other people in my life. This would be the first. My tests, while simple, seem to pass though.
This is also my first pull request ever so I'm probably doing a number on it.
I have tried searching through your code to see if these strategies already exist and I haven't found any like them. It may be that I've simply missread the code and im wasting your time right now, for which I appologise in advance.
It would be cool to have the contributors badge on my account though.

@marcharper
Copy link
Member

marcharper commented Mar 1, 2024

Hi, there's one doc test failing:

----------------------------------------------------------------------
File "/home/runner/work/Axelrod/Axelrod/./docs/index.rst", line 55, in index.rst
Failed example:
    len(axl.strategies)
Expected:
    240
Got:
    242


----------------------------------------------------------------------
Ran 65 tests in 76.362s

FAILED (failures=1)

It's because you added two new strategies.

Also, are you sure there's no similar strategies in the library? Many are TFT with a tweak like what you've done.

@benjjo
Copy link
Author

benjjo commented Mar 2, 2024 via email

Copy link
Member

@marcharper marcharper left a comment

Choose a reason for hiding this comment

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

Left some feedback, should you find that these are new strategies.

@@ -79,7 +79,7 @@ The simplest way to install is::

To install from source::

$ git clone https://github.com/Axelrod-Python/Axelrod.git
$ git clone https://github.com/benjjo/Axelrod.git
Copy link
Member

Choose a reason for hiding this comment

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

We'll want to keep this as is for others.

return D
else:
# returns with the mode of the opponent's history.
return statistics.mode(opponent.history)
Copy link
Member

Choose a reason for hiding this comment

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

Rather than use the statistics library, the history class already keeps a running count of C and D, so you can do something like if opponent.history.cooperations > opponent.history.defections return C else return D. That saves a library import and is more efficient (mode is going to be O(n^2) over the entire game, this way is linear).

return C
else:
# returns with the mode of the opponent's history.
return statistics.mode(opponent.history)
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 re: statistics.mode

"manipulates_state": False,
}

def test_strategy(self):
Copy link
Member

Choose a reason for hiding this comment

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

Try to add a few more tests that single out the specific behaviors like the first move and that the mode works for both C and D. If possible try to use a real strategy like Alternator rather than mock players, but mocks are ok if it's hard to target a particular case.

Copy link
Member

@Nikoleta-v3 Nikoleta-v3 left a comment

Choose a reason for hiding this comment

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

@benjjo thank you very much for your contribution! I agree with marcharper that you should add a few more tests for the strategies.

I am assuming you designed these strategies? If that's the case, could you add a line in the docstring like this:

     Names: 
  
     - Modal TFT: Original name by benjjo or Ben McGuffog

Here's an example:

class RiskyQLearner(Player):
"""A player who learns the best strategies through the q-learning
algorithm.
This Q learner is quick to come to conclusions and doesn't care about the
future.
Names:
- Risky Q Learner: Original name by Geraint Palmer
"""

then this strategy will return with the mode of the previous opponent responses.
"""

# These are various properties for the strategy
Copy link
Member

Choose a reason for hiding this comment

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

I believe this comment/line is from our contributing guidelines. You can remove it.

then this strategy will return with the mode of the previous opponent responses.
"""

# These are various properties for the strategy
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

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.

3 participants