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

Refactored games to generalise & add asymmetric games #1413

Merged
merged 27 commits into from
Apr 24, 2023

Conversation

alexhroom
Copy link
Contributor

@alexhroom alexhroom commented Mar 20, 2023

This PR adds asymmetric games and refactors them as the base game class, with the original Game sitting on top. This is the simplest implementation that does not break any existing behaviour, and when we are willing to make breaking changes (for 5.0.0) the implementation can be simplified as specified here (which I will create as an issue once this is merged)

Solves #1411.

The docs currently do not build, but the unit tests work locally as far as I can tell - @drvinceknight kindly offered to look at the docs issue.

Changes:

  • Creates the AsymmetricGame class, which takes two payoff matrices A and B as input. This can be of any size. (see the new documentation, which shows how to create Rock Paper Scissors with the AsymmetricGame class; if the simpler implementation is added, this can be done with the regular Game class)
  • The Game class now creates a matrix from the r, p, s, t values and creates an AsymmetricGame with matrices A and A.T.
  • The score method now takes two integers (r, c) and returns the value of the matrices at row r, column c - this is an inversion from the initial 'single point of truth' being self.scores (so now it is self.A and self.B, the matrices).
  • Changed Action from an Enum to an IntEnum so they can be used as array indices in the score method (e.g. for the Prisoner's Dilemma, A[C][C] = 3, equivalent to the score of (C, C))
  • Added documentation on creating asymmetric and larger games.
  • Removes numpy mocking (as discussed below) so that the docs build with the new Game class.

Follow-up:

  • create asymmetric tournaments (which are quite a bit more work to implement)
  • create issue for breaking changes
  • create follow-up for generalising Actions to other games
  • create follow-up for categorising strategies by what game they're for

@alexhroom alexhroom changed the title Refactored games to add asymmetric games Refactored games to generalise & add asymmetric games Mar 20, 2023
@marcharper
Copy link
Member

There are some failing tests for me locally. I recommend fixing those first before the docs build issue. The tests for Game directly seem ok, but other tests fail. For example, try:

python3 -m unittest axelrod/tests/unit/test_match.py

This throws a very similar error as the one for docs building.

Traceback (most recent call last):
  File "/home/user/repos/axelrod/Axelrod/axelrod/tests/unit/test_match.py", line 100, in test_non_default_attributes
    @example(turns=5, game=axl.DefaultGame)
  File "/home/user/repos/axelrod/Axelrod/axelrod-venv/lib/python3.7/site-packages/hypothesis/core.py", line 1141, in wrapped_test
    raise the_error_hypothesis_found
  File "/home/user/repos/axelrod/Axelrod/axelrod/tests/property.py", line 382, in games
    game = axl.Game(r=r, s=s, t=t, p=p)
  File "/home/user/repos/axelrod/Axelrod/axelrod/game.py", line 107, in __init__
    super().__init__(A, A.transpose())
  File "/home/user/repos/axelrod/Axelrod/axelrod/game.py", line 38, in __init__
    pair: self.score(pair) for pair in ((C, C), (D, D), (C, D), (D, C))
  File "/home/user/repos/axelrod/Axelrod/axelrod/game.py", line 38, in <dictcomp>
    pair: self.score(pair) for pair in ((C, C), (D, D), (C, D), (D, C))
  File "/home/user/repos/axelrod/Axelrod/axelrod/game.py", line 67, in score
    return (self.A[row][col].item(), self.B[row][col].item())
AttributeError: 'int' object has no attribute 'item'

@marcharper
Copy link
Member

@drvinceknight #1412 also moves github's test ordering to run the unittests before the doc build, so it will give more useful errors in a case like this going forward.

@alexhroom
Copy link
Contributor Author

alexhroom commented Mar 23, 2023

@marcharper my bad, that test is failing for me now too - not sure how I missed it originally, will fix. I also don't think it's the same error as the one in the docs build - when @drvinceknight and I looked at it we found the docs error didn't occur when the mocked modules were changed to (subscriptable) MagicMock objects, although that fix then caused errors elsewhere. (I think this one is due to how Numpy tracks the datatypes of items in arrays!)

I'll also add some tests for asymmetric games (naturally, the implementation is already tested through the existing tests, but some for asymmetric games specifically wouldn't hurt!)

@marcharper
Copy link
Member

@marcharper my bad, that test is failing for me now too - not sure how I missed it originally, will fix.

No worries, there's always some failing tests when you change a low level library detail like this. Locally running ./test will run the entire test suite and often turns up some subtle bugs.

@drvinceknight Maybe we should split up the tests on github a bit so that the documentation and doc tests also run independently, that way each CI run will give more info if there's an early failing test. We could probably just build the docs on linux as with the format checker.

I think this one is due to how Numpy tracks the datatypes of items in arrays!)

That was my suspicion as well

I'll also add some tests for asymmetric games (naturally, the implementation is already tested through the existing tests, but some for asymmetric games specifically wouldn't hurt!)

Agreed, I'd suggest at least one test where B != A.transpose() and at least one matrix that isn't square (maybe the same test?). Otherwise this code will be called many times in the rest of the test suite so it should be very well covered.

@alexhroom
Copy link
Contributor Author

alexhroom commented Mar 27, 2023

Removing the change of type to Python types seems to fix matches (and not cause additional issues - i'm not too sure what I originally put it there to fix!).

However there's now a weird issue with strategy transformers - when creating a Joss-Ann transformer, sometimes the transformed player gives a 0 or 1 to the strategy which causes an error when proposed_action.flip() is called. I've not been able to track down where this happens, but I've only seen 0's and 1's (both in the np.int64 datatype) so I'm guessing somewhere the Action values are being cast to np.int64 - I'd also assume this is the issue as it doesn't happen when the Action class is an Enum rather than an IntEnum (although this creates messier design problems elsewhere).

Currently trying to digest how the Joss-Ann transformer works in order to see where this might be happening!

@drvinceknight
Copy link
Member

@drvinceknight Maybe we should split up the tests on github a bit so that the documentation and doc tests also run independently, that way each CI run will give more info if there's an early failing test. We could probably just build the docs on linux as with the format checker.

Yeah that's a good idea. I can pick that up in a separate PR (or if it makes things easier here I can help @alexhroom)

@alexhroom
Copy link
Contributor Author

alexhroom commented Apr 9, 2023

should all be working now (apart from the docs issue) - essentially some functions were casting Actions to Ints and then failing because they used methods such as .flip(). the fix was simply ensuring they were cast to Action before being flipped.

I also added some unit tests for asymmetric games; these test arbitrary shapes via Hypothesis.

If you'd like to review the code, I think my implementation is now finished unless anything weird turns up!

as for the docs issue - I see two potential solutions;

  1. figure out how to run the mock classes for numpy such that it is a much more accurate 'mock' of numpy arrays. i can't seem to find a way of doing this, but there might be a creative solution here.
  2. @drvinceknight I think you said that we mock the modules for the docs so that the user does not need the modules installed in order to build the docs? perhaps mocking every module except numpy would be a compromise that fixes this pretty quickly

axelrod/history.py Outdated Show resolved Hide resolved
axelrod/history.py Outdated Show resolved Hide resolved
@drvinceknight
Copy link
Member

as for the docs issue - I see two potential solutions;

figure out how to run the mock classes for numpy such that it is a much more accurate 'mock' of numpy arrays. i can't seem to find a way of doing this, but there might be a creative solution here.
@drvinceknight I think you said that we mock the modules for the docs so that the user does not need the modules installed in order to build the docs? perhaps mocking every module except numpy would be a compromise that fixes this pretty quickly

The reason for this mocking might be outdated: this was mainly to ensure read the docs didn't need to install them. BUT I think that's no longer necessary. I could take a look at this if you want (I think it would require removing all mocking, and changing a setting on read the docs), otherwise I'm fine with either of your two solutions (whatever makes it work to be honest 😆 ).

@alexhroom
Copy link
Contributor Author

Now passes CI and uses a system for turning Actions into integers with far fewer side effects. :-)

@drvinceknight
Copy link
Member

Now passes CI and uses a system for turning Actions into integers with far fewer side effects. :-)

Nice work! Will review this afternoon.

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.

Everything looks great to me. A couple of minor requests

Do you have capacity to add to the how to an example of running a tournament or match (and interpreting outcome) with the asymmetric game?

And also, ideally add a unit test for a match, tournament and moran process with the asymmetric game. So we know it all works.

axelrod/action.py Outdated Show resolved Hide resolved
docs/how-to/use_different_stage_games.rst Show resolved Hide resolved
docs/how-to/use_different_stage_games.rst Outdated Show resolved Hide resolved
axelrod/action.py Outdated Show resolved Hide resolved
axelrod/tests/property.py Show resolved Hide resolved
axelrod/history.py Outdated Show resolved Hide resolved
axelrod/action.py Outdated Show resolved Hide resolved
@marcharper
Copy link
Member

Almost there! thanks for sticking with it!

@alexhroom alexhroom mentioned this pull request Apr 20, 2023
@drvinceknight
Copy link
Member

Merging. Thanks for this @alexhroom, I'll try and put a new release out tomorrow (time permitting).

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