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

Implemented Weiner, k41r from Axelrod's Second #1145

Merged
merged 2 commits into from
Dec 1, 2017

Conversation

gaffney2010
Copy link
Member

@gaffney2010 gaffney2010 commented Nov 28, 2017

Fingerprint below.

Some notes about the translation from Fortran: The Fortran cycles through these three states, labeled by ICASE. I replaced the third state with a flag, called forgive_flag. The first two states, it goes back and forth between these as the opponent Defects; when the opponent eventually Cooperates, this strategy goes to state three (raises the forgive_flag) if and only if it's in the second state at that point (i.e. there have been an odd number of Defects since last Cooperate). I wrote the code to replace the first two states with a parity-check on the number of Defects between Cooperates, which I call defect_padding. This reworking should make it a little clearer, and draws out the infinite memory depth that wasn't clear with the original code.

basic_fortran
basic_python

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.

Looks great, thank you @gaffney2010 👍

@drvinceknight
Copy link
Member

(FYI, I have run the tests on axelrod_fortran with this deterministic strategy and they pass.)

@@ -812,11 +813,13 @@ def strategy(self, opponent: Player) -> Action:
if opponent.history[0] == D:
number_defects -= 1

if number_defects in [4, 7, 9]: return D
if number_defects in [4, 7, 9]:
Copy link
Member

Choose a reason for hiding this comment

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

Hey @gaffney2010, it looks like you've copied this manually from #1143?

No big deal at all, I suspect you've done this to try and avoid merge conflicts going forward but for info, that's not considered best practice in terms of etiquette. A better way to do this would be to "go get" the actual git commits that make this change, there are various ways to do this (for example a command called git cherry-pick. One of the main reasons for this is that it ends up causing problems on other people's branches (as well as potentially erasing their contributions in terms of git commit logs).

Not a big problem at all, and not suggesting you change anything (although once #1143 goes in you might end up with a merge conflict here), just letting you know 👍 :)

Copy link
Member

Choose a reason for hiding this comment

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

@gaffney2010 now that #1143 has gone in there are a couple of merge conflicts. Let me know if you need a hand 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay. I can see why it would be better to go get the changes directly from the commit. How can you tell that I did it manually. Did I miss something or does it show up differently?

Copy link
Member

Choose a reason for hiding this comment

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

No it shows up the same but the commit that made the code on #1143 isn't in the history here so the conclusion is that you did it manually.

Not a big deal :)

@drvinceknight
Copy link
Member

I believe this is good to go now @marcharper @meatballs. 👍

@marcharper
Copy link
Member

@gaffney2010 Please resolve the merge conflict(s).

@gaffney2010
Copy link
Member Author

@marcharper Sorry. I'm not seeing how to do that. [I think I saw something earlier that said fix merge conflicts, but that's not there now.] ...

@drvinceknight
Copy link
Member

[I think I saw something earlier that said fix merge conflicts, but that's not there now.] ...

I don't believe there are any merge conflicts on this anymore and it's good to go. Let's wait for @marcharper to perhaps expand on what he meant in case he's noticed something we're both missing. 👍

@marcharper marcharper merged commit be2c4b0 into Axelrod-Python:master Dec 1, 2017
@marcharper
Copy link
Member

Under "rebase and merge" (my default) github had a merge conflicts warning. However "merge pull request" worked.

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.

None yet

4 participants