Skip to content

Add BetterAndBetter, WorseAndWorse2 and WorseAndWorse3 Strategies #786

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 7 commits into from
Dec 7, 2016

Conversation

varung97
Copy link
Contributor

@varung97 varung97 commented Dec 5, 2016

No description provided.

@varung97
Copy link
Contributor Author

varung97 commented Dec 5, 2016

This pull request is currently incomplete as I haven't yet implemented all the strategies.
I was hoping you could glance over it to see whether it meets the guidelines for this project.

Also, I was wondering whether, since the BetterAndBetter strategy is random, I should add a test that checks if say given any seed, the number of defects out of the first 20 is very high, or a test which checks that after a 1000 turns the player always cooperates? I guess with the first there would be a certain probability that the test fails, which could be an issue.

Thanks!

@drvinceknight
Copy link
Member

This pull request is currently incomplete as I haven't yet implemented all the strategies.
I was hoping you could glance over it to see whether it meets the guidelines for this project.

Sorry for taking a while to get back to you @varung97, this looks right in terms of guidelines. Please carry on implementing the other strategies and we'll review it properly :)

Also, I was wondering whether, since the BetterAndBetter strategy is random, I should add a test that checks if say given any seed, the number of defects out of the first 20 is very high, or a test which checks that after a 1000 turns the player always cooperates? I guess with the first there would be a certain probability that the test fails, which could be an issue.

The way we've done things so far is to include tests with seeds.

@varung97
Copy link
Contributor Author

varung97 commented Dec 6, 2016

Alright, thanks! I'm closing this pull request while I work on the other strategies, will reopen it when I'm done.

@varung97 varung97 closed this Dec 6, 2016
@marcharper
Copy link
Member

@varung97 You can leave it open if you want, just tag it with "ready for review" when you are ready. We're usually happy to help write tests as time allows.

@varung97 varung97 reopened this Dec 6, 2016
return opponent.history[-1]
else:
probability = 20 / float(current_round)
return random_choice(probability)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think I should make a random_choice_defects function or something of the sort, as it might be a little confusing that the stated probability is 1 - 20 / current turn, but the probability in the code is just 20 / current turn?
Or should I add a comment to that effect?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't make a new function, I think it's fine as it is :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okies

@varung97
Copy link
Contributor Author

varung97 commented Dec 6, 2016

I'm not able to add labels to the PR I think 😅 (unless I'm doing something wrong)

@drvinceknight
Copy link
Member

Yeah only admins can add labels but just ping us here when this is ready :) 👍

@varung97
Copy link
Contributor Author

varung97 commented Dec 6, 2016

Oh I see. Could you add the ready for review label then?

@drvinceknight
Copy link
Member

Sure thing (on my phone right now but I'll do it as soon as I get on front of my machine) 👍

@varung97
Copy link
Contributor Author

varung97 commented Dec 6, 2016

Alright, thanks!

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.

This is looking good: thanks.

A few minor comments about being able to remove float division and otherwise a request for a stylistic change on your last test class (just use methods for each case). Let me know if I can assist/clarify.


def strategy(self, opponent):
current_round = len(self.history) + 1
probability = float(current_round) / 1000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Axelrod doesn't support python 2 anymore so you don't need to force float division, you can have this simply as current_round / 1000.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops right will fix that

elif current_round <= 20:
return opponent.history[-1]
else:
probability = 20 / float(current_round)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for force float division.

if current_round == 1:
return C
else:
probability = 1 - opponent.defections / float(current_round - 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for float.

'manipulates_state': False
}

class TestWAW3vsCooperator(TestHeadsUp):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to create another class here, you can simply use different methods for each other these.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So should I do something like this?
opponent = axelrod.Cooperator()
player = self.player()
match = axelrod.Match((opponent, player), turns=10)
self.assertEqual(match.play(), [('C', 'C')] * 10)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup you could do that, you could also use the inbuilt testing help function: http://axelrod.readthedocs.io/en/latest/tutorials/contributing/strategy/writing_test_for_the_new_strategy.html

@varung97
Copy link
Contributor Author

varung97 commented Dec 6, 2016

Is this what you were looking for?

@drvinceknight
Copy link
Member

Is this what you were looking for?

Yup, this looks good to me. Giving it a 👍, fyi we have a two core reviewer policy: @marcharper and/or @meatballs might have some other requests :)

@marcharper
Copy link
Member

Looks good to me. Thanks for the contribution!

@marcharper marcharper merged commit 1fa74da into Axelrod-Python:master Dec 7, 2016
@varung97
Copy link
Contributor Author

varung97 commented Dec 7, 2016

Yay, thanks guys!

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.

4 participants