Unique strategy id and index

[meatballs]

closes #688

[marcharper]

This is my fault, I removed the encoding header line in my last cleanup PR. Just add # -*- coding: utf-8 -*- to test_player.py to fix it.

[drvinceknight]

Is index the right word? Makes me think of a number... Not terribly offended by index though.

[meatballs]

It makes me think of an index at the back of a book, or a database index - something where you look things up!!

Not too bothered myself, if there's a better suggestion.

[drvinceknight]

No I think now that you've said that I'm cool with index. Ignore me :)

[drvinceknight]

This looks good to me.

• Do we want to add this id as an attribute to the strategies? Thinking of the use case of writing them to data files.
• Could we have a test for strategy_index? Perhaps in here: https://github.com/Axelrod-Python/Axelrod/blob/master/axelrod/tests/unit/test_classification.py#L131, a check that all strategies have ids that are keys and also just hard checking a few of them?

[meatballs]

Do we want to add this id as an attribute to the strategies? Thinking of the use case of writing them to data files.

Not sure I quite understand the use case - I don't have any great objection, just want to understand!!

It would need to be a class attribute, if that makes any difference

[meatballs]

Could we have a test for strategy_index? Perhaps in here: https://github.com/Axelrod-Python/Axelrod/blob/master/axelrod/tests/unit/test_classification.py#L131, a check that all strategies have ids that are keys and also just hard checking a few of them?

OK. I'll put something together

[meatballs]

I've added two tests for the strategy index:

1. ensure that all strategies have an id that is included in the index. This test passes OK.
2. ensure that the lengths of the all_strategies list and the strategy_index dictionary are equal. (i.e. there are no duplicate strategy IDs). This test fails.

[meatballs]

Code to flush out the duplicates:

import axelrod as axl
import collections

ids = [axl.strategy_id(s) for s in axl.all_strategies]
print([(item, count) for item, count in collections.Counter(ids).items() if count > 1])

which gives:

[('player', 16), ('cycler', 5)]

[meatballs]

print([s for s in axl.all_strategies if s.name in ('Player', 'Cycler')])

gives

[<class 'axelrod.strategies.cycler.CyclerCCCCCD'>, <class 'axelrod.strategies.cycler.CyclerCCCD'>, <class 'axelrod.strategies.cycler.CyclerCCD'>, <class 'axelrod.strategies.cycler.CyclerDC'>, <class 'axelrod.strategies.cycler.CyclerDDC'>, <class 'axelrod.strategies.gobymajority.GoByMajority'>, <class 'axelrod.strategies.gobymajority.GoByMajority10'>, <class 'axelrod.strategies.gobymajority.GoByMajority20'>, <class 'axelrod.strategies.gobymajority.GoByMajority40'>, <class 'axelrod.strategies.gobymajority.GoByMajority5'>, <class 'axelrod.strategies.gobymajority.HardGoByMajority'>, <class 'axelrod.strategies.gobymajority.HardGoByMajority10'>, <class 'axelrod.strategies.gobymajority.HardGoByMajority20'>, <class 'axelrod.strategies.gobymajority.HardGoByMajority40'>, <class 'axelrod.strategies.gobymajority.HardGoByMajority5'>, <class 'axelrod.strategies.retaliate.LimitedRetaliate'>, <class 'axelrod.strategies.retaliate.LimitedRetaliate2'>, <class 'axelrod.strategies.retaliate.LimitedRetaliate3'>, <class 'axelrod.strategies.retaliate.Retaliate'>, <class 'axelrod.strategies.retaliate.Retaliate2'>, <class 'axelrod.strategies.retaliate.Retaliate3'>]

[meatballs]

the remaining 16 with an id of 'player' are those that use init_args to set the name. Since they only do so on instantiation, the class attribute still has 'Player' as its name'

[drvinceknight]

Not sure I quite understand the use case - I don't have any great objection, just want to understand!!

It would need to be a class attribute, if that makes any difference

Class attribute would be fine.

So at the moment @Nikoleta-v3 is dealing with a dataset of the following form (that's she's generating):

Player 1 name, Player 2 name,...
Cooperator, Tit For Tat,...

where this dictionary becomes useful is if instead the data set was:

Player 1 id, Player 2 id,...
id1, id2,...

So that if we were to read in the data set we could obtain the classes (EDIT: if we wanted to reproduce some of this) by going:

strategy_index[id1]

So... to obtain the id in the first place we would need to write: player.id (or whatever) to the data set).

This was what I was thinking you meant when wrote (#668):

The unique id is currently produced using a function which strips the whitespace from the player name and converts it to lower case. This could be done with a property in the Player class.

Without the players themselves having the id I'm not sure I understand the use case for the index dictionary? (Been a long day and I've just had a walk in the sun so I might be missing something).

the remaining 16 with an id of 'player' are those that use init_args to set the name. Since they only do so on instantiation, the class attribute still has 'Player' as its name'

That's a pity. Perhaps if we indeed put the id as a class property, then we could override it for these tricky ones and change the generation of the index to just grab the id from the class itself?

strategy_index = {s.id: s for s in all_strategies}

One other point (when we get there): could you add a brief line or two about this index to the docs: http://axelrod.readthedocs.io/en/latest/tutorials/advanced/classification_of_strategies.html

[marcharper]

Isn't the strategy name already a unique id at the class level? A new class attribute won't help with:

• Multiple instances, or
• Different parameters

[drvinceknight]

Good point.

On Tue, 16 Aug 2016, 17:48 Marc Harper, notifications@github.com wrote:

Isn't the strategy name already a unique id at the class level? A new
class attribute won't help with:

• Multiple instances, or
• Different parameters

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#694 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACCGWjj0AIxKQECgcqm6NyhLLz2FgQVpks5qgenRgaJpZM4JkUzU
.

[drvinceknight]

Good point.

The use case I described above could actually easily be taken care of using this dictionary:

d = {s().name:s for s in axl.strategies}

Perhaps, if what this PR is doing is creating a dictionary that maps lower case names with stripped white space (needed for the api right? for urls?) to classes, then this is better located in the api and doesn't add anything to the library?

[meatballs]

Perhaps, if what this PR is doing is creating a dictionary that maps lower case names with stripped white space (needed for the api right? for urls?) to classes, then this is better located in the api and doesn't add anything to the library?

Possibly, but it's also adding the index - yes, we could do that with the name as the key, but then I'd just end up repeating virtually the same index in the api but using the unique_id instead.

[meatballs]

so, if the index is no use, then this should just be in the api. However, if the index is useful then both should be in the library to keep things DRY between it and the api.

[drvinceknight]

I agree, both here or both in the api.

My current point of view is that as is the index is not useful. To use it,
a library user needs to know the name (stripped or not or equivalent) which
gives access to the class but that's equivalent to knowing the class and
calling it directly.

On Wed, 17 Aug 2016, 13:36 Owen Campbell, notifications@github.com wrote:

so, if the index is no use, then this should just be in the api. However,
if the index is useful then both should be in the library to keep things
DRY between it and the api.

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
#694 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACCGWu6XziNpohe4SdQ4FFKldCotkGLlks5qgwBIgaJpZM4JkUzU
.

[meatballs]

I think I'm coming round to that view myself.

[drvinceknight]

I suggest we close this PR?

Looking through the commit messages it looks like you might have fished out a couple of things that were wrong with some .names? Are those worth cherry picking?

[meatballs]

Yep. I'll close this PR and submit a new one. I need the names fixing in order for the indexing to work properly in the api.