Skip to content

SpitefulCC from [Mathieu2015] #1364

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

Merged
merged 6 commits into from
Sep 9, 2020

Conversation

RomeroLaura
Copy link
Contributor

Added new SpitefulCC Player to grudger.py, updated the list of all strategies, and added coverage test in test_grudger.py.
I also run these files by black formatter as specified in the instructions.

Let me know if everything is in order

@marcharper
Copy link
Member

Hi, thanks for the addition. One of the documentation tests is failing, you'll just need to bump the total number of strategies up by one:

 ............
======================================================================
FAIL: ./docs/index.rst
Doctest: index.rst
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/hostedtoolcache/Python/3.7.8/x64/lib/python3.7/doctest.py", line 2204, in runTest
    raise self.failureException(self.format_failure(new.getvalue()))
AssertionError: Failed doctest test for index.rst
  File "./docs/index.rst", line 0

----------------------------------------------------------------------
File "./docs/index.rst", line 55, in index.rst
Failed example:
    len(axl.strategies)
Expected:
    236
Got:
    237

@marcharper
Copy link
Member

Note to other reviewers: isort issues will be fixed with #1288

@marcharper marcharper marked this pull request as ready for review August 12, 2020 00:23
@marcharper
Copy link
Member

Hi, can you rebase this onto the latest master? That will fix the outstanding isort issues. If this repository (not your fork) is origin, just run the following commands while on your branch:

git fetch origin
git rebase origin/master
git push -f

otherwise you'll need to add the root repository to your remotes.

git remote add root https://github.com/Axelrod-Python/Axelrod.git

and run the commands above with root in place of origin.

Since we changed the how random seeding works in #1288, one of the existing tests fails:

======================================================================
FAIL: test_strategy (strategies.test_meta.TestNMWEDeterministic)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/user/repos/axelrod/Axelrod/axelrod/tests/strategies/test_meta.py", line 642, in test_strategy
    self.versus_test(opponent=axl.Alternator(), expected_actions=actions, seed=11)
  File "/home/user/repos/axelrod/Axelrod/axelrod/tests/strategies/test_player.py", line 580, in versus_test
    match_attributes=match_attributes
  File "/home/user/repos/axelrod/Axelrod/axelrod/tests/strategies/test_player.py", line 649, in versus_test
    self.assertEqual((i, play), (i, expected_play))
AssertionError: Tuples differ: (2, C) != (2, D)

First differing element 1:
C
D

- (2, C)
?     ^

+ (2, D)

The fix for that is to change line 641 of axelrod/tests/strategies/test_meta.py to
actions = [(C, C), (C, D), (C, C), (D, D), (D, C)]

change line 641 of axelrod/tests/strategies/test_meta.py
@RomeroLaura
Copy link
Contributor Author

there seems to be a problem with one of the doctests which shows the following:

Failed example:
results.ranked_names == results2.ranked_names
Expected:
True
Got:
False

File "./docs/tutorials/advanced/setting_a_seed.rst", line 69, in setting_a_seed.rst

refering to players = (axl.Random(), axl.Cooperator(), axl.MetaMixer())

I am not quite sure what the expected functionality here is

@marcharper
Copy link
Member

Ah, it's a bad test. MetaMixer with default arguments includes most strategies in its ensemble, so a new strategy can break it.

@drvinceknight I recommend that we avoid these "change detector tests" for the Meta strategies in general, since it's a barrier to adding new strategies efficiently.

Also I vote we go ahead and merge this PR, then open up a new one to fix this test and the the other one that tripped above.

@drvinceknight
Copy link
Member

@drvinceknight I recommend that we avoid these "change detector tests" for the Meta strategies in general, since it's a barrier to adding new strategies efficiently.

Sorry for only responding to this now! My bad. I've seen #1370 where you discuss this.

Also I vote we go ahead and merge this PR, then open up a new one to fix this test and the the other one that tripped above.

I'll go ahead and do that in the next hour or so :)

@drvinceknight drvinceknight merged commit d535a74 into Axelrod-Python:master Sep 9, 2020
drvinceknight added a commit that referenced this pull request Sep 9, 2020
This was failing on #1364
but the fix is trivial.
drvinceknight added a commit that referenced this pull request Sep 9, 2020
This was failing on #1364
but the fix is trivial.
Nikoleta-v3 pushed a commit that referenced this pull request Sep 9, 2020
* Added new SpitefulCC Player to grudger.py, updated the list of all strategies and added coverage test in test_grudger.py

* changed 236 strategies to 237 in docs/index.rst

* changed SpitefulCC classifier and added strategy description

* changed description spitefulCC strategy

* Fix doctest.

This was failing on #1364
but the fix is trivial.

* Correct expected actions for meta strategy.

* Run black and isort.

Co-authored-by: Laura Romero <romerocarrillol@outlook.com>
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.

3 participants