-
Notifications
You must be signed in to change notification settings - Fork 264
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
Conversation
This reverts commit 09fb251.
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:
This throws a very similar error as the one for docs building.
|
@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. |
@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!) |
No worries, there's always some failing tests when you change a low level library detail like this. Locally running @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.
That was my suspicion as well
Agreed, I'd suggest at least one test where |
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 Currently trying to digest how the Joss-Ann transformer works in order to see where this might be happening! |
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) |
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;
|
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 😆 ). |
Now passes CI and uses a system for turning |
Nice work! Will review this afternoon. |
There was a problem hiding this 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.
Almost there! thanks for sticking with it! |
Merging. Thanks for this @alexhroom, I'll try and put a new release out tomorrow (time permitting). |
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:
AsymmetricGame
class, which takes two payoff matricesA
andB
as input. This can be of any size. (see the new documentation, which shows how to create Rock Paper Scissors with theAsymmetricGame
class; if the simpler implementation is added, this can be done with the regularGame
class)r, p, s, t
values and creates an AsymmetricGame with matricesA
andA.T
.(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' beingself.scores
(so now it isself.A
andself.B
, the matrices).Action
from anEnum
to anIntEnum
so they can be used as array indices in thescore
method (e.g. for the Prisoner's Dilemma,A[C][C]
=3
, equivalent to the score of(C, C)
)Follow-up: