Is the size of the action set the right thing to key on? Did you consider having tighter agreement between the player's outputs actions and the expectations of the Game rather than just mathematical compatibility?

I.e. IPD_actions = {C, D} (where C, D come from the IPD Game), then we could programmatically check if the Game's domain matches the Player's range. As other games are added, we could do something similar. For a game like Hawk-Dove, the actions would be {escalate, withdraw}, for rock paper scissors they would be {R, P, S}, and so on.

@marcharper I did consider this; however my main concern was a use case e.g. if a user wanted to play around with the Hawk-Dove game but still use strategies like axl.Cooperator or axl.TitForTat - having to make new axl.hawkdove.Cooperator or axl.hawkdove.TitForTat, both identical save for the actual action objects, would cause massive amounts of code duplication.

For strategies specific to one game, I trust that when the user is importing some axl.rockpaperscissors.Strategy that in the very act of typing "rockpaperscissors" they're aware that they're importing a rock-paper-scissors strategy. To me, warning the user because because they used an IPD strategy on a Hawk-Dove matrix would be adding safeguards that frustrate users more often than they're actually helpful - warnings should be saved for times when the user may be surprised by how the code interprets their input. The user is arguably correct to expect that "play action 2 on the Hawk-Dove game" means the same thing as "play action 2 on the Prisoners' Dilemma"; what may be more unexpected is playing axl.Cycler on rock-paper-scissors and discovering it just cycles between actions 1 and 2, not all 3 actions, or playing a rock-paper-scissors strategy on the IPD and having their tournament fall over when the strategy tries to play scissors.

Also note that (contrary to what I said in the docs - I made a mistake on the rock-paper-scissors docs and have submitted #1419 to fix it) because of how the ordering of actions is defined (two actions are equal if their values are equal), any set of actions with a total ordering will be interoperable. For example, axl.hawkdove.Action.Escalate == axl.Action.D is True.

Let's think carefully about this. A strategy could be technically playable with any 2x2 game but designed for / intended for a specific game. For example, many of the IPD strategies assume specific values of RPST. It's not clear to me that game action size is the determining factor, nor should we rely on a total ordering and equate axl.hawkdove.Action.Escalate == axl.Action.D. Different libraries map C and D to 0 and 1 differently, and one can swap rows in a game matrix to effectively change the specific ordering (total orders are not unique for finite indexing sets).

It would be better for strategies to declare the games they are intended for, and perhaps also the games they are technically playable with, and we should warn the user when the player and the game are not aligned. A common rule of software development is that users can't be trusted to know what they are doing! In other words, we need to think carefully about the proper abstractions for the library rather than just push through what's technically feasible.

having to make new axl.hawkdove.Cooperator or axl.hawkdove.TitForTat, both identical save for the actual action objects, would cause massive amounts of code duplication.

This isn't necessarily true. Hypothetically, with better abstractions, both strategies could derive from a "Play index 1 always of a finite game" strategy, retaining proper type alignment between Player and Game. Also, this is a research library focusing on usability and reproducibility, so minimizing code duplication isn't necessarily a privileged optimization metric.

(Sorry! This ended up being a long comment, since I wanted to fully explain my thoughts on the design of this. TL;DR: Bullet point 4, and the final paragraph)

I understand your point, and it may be worth adding a classifier for if a game really depends on the specific game being played. However, I can see a few points where I'm not sure that using this as our main classifier would really increase usability, and it would add complexity to development at the same time:

• firstly just to note: the library already depends on the total ordering of actions and already bases this ordering on their values. Look at TitForTat; if I've created a different library mylib which has mylib.Action.D=0 and mylib.Action.C=1, then TitForTat would already evaluate mylib.Action.C == axl.Action.D as True and defect in that case.
• Strategies which rely on the specific values of RPST get said values from the Game object's RPST attribute. If the behaviour of these strategies is drastically different because the RPST values are drastically different, this would be an issue that's always existed (the Game class has always had the ability to modify RPST, by the looks of git blame), but again that may be the best place to add the safeguards.
• currently, games aren't 'coupled' to actions in any way; Games just treat actions as integers and return scores. Adding this coupling and then attaching all existing strategies to it would stop existing strategies from working with other games, and then going back through to generalise them again would be a huge piece of work modifying hundreds of strategies just to get back to where we started (update: semi-avoided this with the new implementation). I do definitely agree that generalising strategies which essentially just run a sequence of actions with no assumptions on the size of the game (e.g. Random, Cycler) would be a good idea, as they don't currently work with larger games and a user would likely expect that they should.
• if strategies are coded to be tied to specific games, then every time a new game is added it'd be necessary to go through the entire strategy index to update what games they're "technically playable with", adding a huge piece of tedious busywork to any contributor who wants to add a new game; likewise someone adding a new strategy would have to figure out which games it does and doesn't work with.
  • Note here also that if a user creates a custom game that they don't want to add to the library itself (e.g. for a research project) then they'd either have to create every strategy they want to use for their project from scratch, or put up with endless warnings; they can't go through modifying library source code to flag up that the strategies are compatible with their game.
• For usability, I agree that maybe code duplication in implementation isn't as important but strategy duplication absolutely is. If I pick up Axelrod for the first time and then have to figure out for my code whether "play action 1 unless you last played action 2, then play action 2" should use axl.TitForTat() or axl.hawkdove.TitForTat() or axl.GeneralisedTitForTat(actions={myaction.A, myaction.B}) etc. etc. would make my head spin. Fundamentally, the abstraction for a Player is "a mapping from play history onto the game's actions {1, 2, ..., n}" and the simplest path for the user is if strategies are designed that way.

I just think that the incidence of "someone accidentally uses a strategy which doesn't work on their custom game" is lower than that of "someone wants to use an existing strategy on a different game of the same size", and gearing our classification system towards the first one at the expense of the latter just feels like it'll create more ignorable warnings than actually informative ones. In my experience with developing research software, when a less code-savvy user gets a warning then they usually treat it like the check engine light on a car! With this in mind I think a warning that's not relevant more often than it's relevant is problematic.

As for development, my main rub is related to point 4. Enumerating exactly "this strategy works with game X, Y, Z and doesn't work with any other game" and then having to update it every time a game is added is almost certainly not the best abstraction (one could argue it isn't even an abstraction!) Perhaps for strategies relying on specific assumptions of a game, a better solution would be to add an "attributes" dict to the Game, and then an "assumptions" attribute to the strategy classifier. If the Game attributes disagree with the Player assumptions (e.g. the Player assumes something that isn't in the Game attributes, or the Game attribute value is not equal to the equivalent Player assumption attribute), then a warning is raised. This allows us to account for strategies which really rely on the game being a certain way, while leaving the rest of the library completely plug-and-play, and hugely reducing the work required to add a new game to the library; a new game could add its own assumptions and have that as a single point of truth for what strategies do and don't work with it, and vice versa with new strategies and valid games.

Reimplemented as a more general 'assumptions' model:

• When creating a strategy, the classifier can now contain 'assumptions' like game size, Action class, things like RPST existing, or even specifically what the game is
• When creating a game, the game now has 'attributes' which identify features that strategies can make assumptions about
• Then when a match is created, the player's assumptions are checked against the game's attributes. If the player makes an assumption that the game doesn't have as an attribute, or if the values mismatch, an error is raised
  • Matches also now have a strict_player_compatibility parameter which, if set to False, downgrades these errors to warnings
• This allows each individual strategy to have as tight or loose compatibility with games as it needs, with automatic support for new games whether they're added 'officially' or for personal/individual use.

Some examples:

• axl.Cooperator would not need to assume anything! It always chooses action 1, which always exists.
• axl.Adaptive would assume its action set has size 2 exactly, as it is adapting according to that.
• axl.Cycler's current implementation would need to assume that the actions are {C, D} as it specifically reads a string of C's and D's. (however it doesn't make assumptions on the actual game matrix itself!)
• axl.FirstByDowning would need to assume the game has RPST, since it uses that in its implementation.
• axl.EvolvedANN would assume precisely that the game is Prisoners' Dilemma, as it is trained on that game.

does this address your concerns @marcharper? I think it's a much more flexible model for maintaining alignment between Player and Game without having to be fully locked-down (and without the greater quantity of busywork for future contributors described in the previous comment). If we wanted to then avoid using mismatched action sets, strategies could be generalised with an action_set parameter to their __init__ method to take an action set - this generalisation could be done alongside actually implementing the assumptions to all of the strategies (in a separate PR to this!)

I think I like the look of this @alexhroom, using the basic definition of a strategy as a way of mapping "information" to actions.

I understand your point @marcharper though and really trust your instincts here.

Would creating a specific kind of AxelrodError Invalid Game be a good thing to implement? So if a strategy requires something that's not in the game it raises a verbose error?

@drvinceknight one can see here https://github.com/Axelrod-Python/Axelrod/pull/1418/files#diff-0e43e29ffb206bbff9d1780ef3d83a2407c3ec58e893fd93c06bbcdd478a1278R275-R290 that currently a RuntimeError with a verbose message is raised, stating what assumption was violated. as far as I can see the only real benefit to using a unique exception type here is to be able to suppress it, but the raise_error=False option allows code to bypass it anyway

This is better, thanks. Some notes:

• It's true that it's always been possible to make strategies behave oddly with unusual RPST. Similarly, some of the older strategies hard code values that may be based on RPST implicitly. These will probably behave poorly on other games. It's always been possible to change the game matrix, but it's not been a prominent feature until recently.
• It's not true that the library depended on the order of {C, D} -- we've always used either the characters 'C' and 'D' or the enum (matching explicitly on action.C or action.D) once Python supported enums. (The enum values 0 and 1 are only there because the Python enum syntax requires something.) I'm not necessarily opposed to making such a change, but currently Cooperator isn't synonymous with "play first action" and Defector could be any of "play the opposite of C" or "play index 2 action" or "play last action"... We can change this but you can see how it doesn't uniquely generalize to nxn games canonically.
• I certainly agree that we don't want to make contributing a new strategy more tedious or necessitating many changes. As we start supporting a larger variety of games, there's more chance for confusion, as existing parts of the library like axelrod.strategies change semantics from "strategies usable with IPD" to a mix of strategies intended for different games that now need to be filtered somehow for use with any specific game without producing errors or non-intuitive results. (Imagine a RPS strategy all-Scissors strategy -- it won't work with IPD.) The classifier approach seems like a good way to handle this, we already have some filters for "obeys [the] axelrod [tournament rules]". However we might want to take a different approach, making axelrod.strategies a more complex datastructure, or splitting it per game, or something else.

We can change this but you can see how it doesn't uniquely generalize to nxn games canonically.

I understand - maybe better to keep those strategies as 2x2 for now. Like I said, this would be done in a different PR (i'd prefer this PR to just focus on implementing the framework) so the details of that can be hashed out then.

However we might want to take a different approach, making axelrod.strategies a more complex datastructure, or splitting it per game, or something else.

See the discussion in #1414. Maybe it'd be useful here to implement a helper function which takes a Game and a strategy or list of strategies and returns a bool or some other information on whether or not the strategies work with the game? this could then be used with a list comprehension in the same way as we can already classify strategies; then akin to axl.demo_strategies or axl.basic_strategies it's not too difficult to take axl.ipd_strategies etc.

• However we might want to take a different approach, making axelrod.strategies a more complex datastructure, or splitting it per game, or something else.

Not against a classifier approach but I do like the sound of axelrod.strategies.rps, axelrod.strategies.ipd, axelrod.strategies.ultimatum_game etc... I feel it would allow for a more "automatic"/easier classification.

I guess my issue with axelrod.strategies.rps, axelrod.strategies.ipd etc. would be where we put generic strategies; e.g. if we created some generic strategy axl.Static which just takes an action and plays it every turn (this is a generalisation of Cooperator, Defector, etc) or even specifically IPD strategies like axl.TitForTat which would be a valid strategy on, e.g. the Hawk-Dove game (where it withdraws on turn 1, then copies the opponent's previous move in future). maybe there's something cool with namespaces that would get the best of both worlds - i.e. we take a classifier approach but use some Python import wizardry to make it accessible both from axl.TitForTat or axl.strategies.ipd.TitForTat, where axl.strategies.ipd provides a namespace for both strategies specific to the IPD (which are in a specific strategies.ipd folder) and strategies that work with the IPD. i think i've seen similar before in another package (but I'm not sure where). that way users can get strategies from axl.strategies.ipd for a safety guarantee that their strategies will work with the game, or just from axl. if they want to mix and match some generic strategies, or borrow the strategy to add it to their own new game without having to remember what game it's "originally for".

I guess my issue with axelrod.strategies.rps, axelrod.strategies.ipd etc. would be where we put generic strategies; e.g. if we created some generic strategy axl.Static which just takes an action and plays it every turn (this is a generalisation of Cooperator, Defector, etc) or even specifically IPD strategies like axl.TitForTat which would be a valid strategy on, e.g. the Hawk-Dove game (where it withdraws on turn 1, then copies the opponent's previous move in future). maybe there's something cool with namespaces that would get the best of both worlds - i.e. we take a classifier approach but use some Python import wizardry to make it accessible both from axl.TitForTat or axl.strategies.ipd.TitForTat, where axl.strategies.ipd provides a namespace for both strategies specific to the IPD (which are in a specific strategies.ipd folder) and strategies that work with the IPD. i think i've seen similar before in another package (but I'm not sure where). that way users can get strategies from axl.strategies.ipd for a safety guarantee that their strategies will work with the game, or just from axl. if they want to mix and match some generic strategies, or borrow the strategy to add it to their own new game without having to remember what game it's "originally for".

Like the idea of something smart with namespaces (with the caveat for ease of maintenance...) -- I think the flat axelrod. namespace was something we would/should have done differently so would be in favour of taking advantage of a 5.0.0 to do that.

A thought: would a axl.strategies.generic namespace be helpful? For things like axl.strategies.generic.Static?

However this is all a different PR to this one. I think this discussion should be moved to a relevant issue instead!

👍

Minor point: Did you want to grab the commit with the change to fix the readthedocs build?

Minor point: Did you want to grab the commit with the change to fix the readthedocs build?

I'll just merge my fork's dev into this branch when #1419 is merged - trying to cherry-pick the commit without messing anything up is a nightmare in my experience (and suspiciously the read the docs build seems to now be succeeding without it)

Failing on coverage:

TOTAL                                                            17952      5    99%
Coverage failure: total of 99 is less than fail-under=100
Error: Process completed with exit code 2.

as far as I can tell the failure in the CI isn't caused by anything i've done - it seems to always almost pass except randomly fail sometimes (looking at both the test logs and running it locally a few times), unrelated to this PR

as far as I can tell the failure in the CI isn't caused by anything i've done - it seems to always almost pass except randomly fail sometimes (looking at both the test logs and running it locally a few times), unrelated to this PR

This happens sometimes with hypothesis. Have kicked the CI, hopefully that sorts it but if not will look in to it.

Do I understand correctly: with this PR every existing strategy assumes that the action set size is 2 and there are no other specified assumptions?

@marcharper indeed - my aim for this PR is just to add the actual framework for strategy classification (which is not a breaking change so could be merged to dev) and then a follow-up issue would be to actually go through all the strategies and add assumptions, probably alongside a bigger refactoring of the strategies folder to account for non-ipd games - this would be a breaking API change (so would be part of 5.0.0).

Should this go into the 5.0.0 branch now then, since we anticipate breaking behavior soon?

@marcharper sure! changed base branch (note that means the most recent commit to dev has been added to this pr!)

Thanks for all the work on this @alexhroom