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

BM player addition #1002

Merged
merged 8 commits into from
Oct 26, 2017
Merged

BM player addition #1002

merged 8 commits into from
Oct 26, 2017

Conversation

GGOUSSEAUD
Copy link
Contributor

No description provided.

@GGOUSSEAUD
Copy link
Contributor Author

Fixed everything, everything should be ready to merge!


class BM(Player):
"""
A player that is based on Bush Mosteller reinforced learning algorithm, he decides what he'll
Copy link
Member

Choose a reason for hiding this comment

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

Could we please use gender neutral pronouns? - 'it' rather than 'he'

class BM(Player):
"""
A player that is based on Bush Mosteller reinforced learning algorithm, he decides what he'll
play only depending on his own previous payoffs.
Copy link
Member

Choose a reason for hiding this comment

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

'its' not 'his'

play only depending on his own previous payoffs.

The probability of playing C or D will be updated using a stimulus which represents
a win or a loss of value based on his previous play's payoff in the specified probability.
Copy link
Member

Choose a reason for hiding this comment

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

'its' not 'his'


The probability of playing C or D will be updated using a stimulus which represents
a win or a loss of value based on his previous play's payoff in the specified probability.
The more a play will be rewarded through rounds , the more the player will be tempted to use it.
Copy link
Member

Choose a reason for hiding this comment

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

typo: remove the space before the comma

self._learning_rate = learning_rate


def stimulus_update(self, opponent: Player):
Copy link
Member

Choose a reason for hiding this comment

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

This method needs a docstring

self._d_probability = self._d_probability + self._learning_rate * self._stimulus * self._d_probability


def BM_random_choice(self) -> Action:
Copy link
Member

Choose a reason for hiding this comment

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

docstring

Note that this test is referred to in the documentation as an example on
writing tests. If you modify the tests here please also modify the
documentation.
"""
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this docstring. It doesn't apply here

@meatballs
Copy link
Member

@GGOUSSEAUD There are a lot of PEP8 errors in bm.py which need to be fixed.

Shout on gitter if you need help with that.

@GGOUSSEAUD
Copy link
Contributor Author

Thank you for your review, I'll patch everything that is needed when I have time, right back at you when it's done.

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.

Could we make the name of the player and the file more verbose please?

I suggest bush_mosteller.py and BushMosteller.

@GGOUSSEAUD
Copy link
Contributor Author

Gotcha ! I'll fix this when I have some spare time. I'm having a tough time with PEP 8 , I get a lot of errors x)

GGOUSSEAUD and others added 4 commits October 25, 2017 08:36
Added Bush Mosteller test player and player
- Improving docstrings;
- PEP8 improvements;
- Removing unneeded reset
- Using the in built random choice
- Changing the name
@drvinceknight
Copy link
Member

@GGOUSSEAUD and @meatballs in 72ba385 I've made the minor adjustments:

  • Improving docstrings;
  • PEP8 improvements;
  • Removing unneeded reset
  • Using the in built random choice
  • Changing the name

There might well be other things (it's been a while since I looked at this PR) that are still needed.

(I've also fixed merge conflicts and rebased on to master, as it's been a while the diff was horrendous other wise :) 👍.)

@drvinceknight
Copy link
Member

@marcharper: @meatballs and I have gone through and OK'd this but I did the last couple of things and made minor adjustments like memory length etc. If you could have a quick check and OK that'd be great 👍

@marcharper marcharper merged commit f828f4b into Axelrod-Python:master Oct 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants