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

This PR adds the Momentum strategy #1469

Merged
merged 12 commits into from
Feb 3, 2025

Conversation

dongwonmoon
Copy link
Contributor

Summary: This pull request introduces a new strategy, Momentum, inspired by the concept of Gradual and the Momentum optimizer used in deep learning. The strategy models the dynamics of trust evolution in a repeated game setting, with momentum reflecting how trust shifts based on previous interactions. The momentum formula used in this strategy is derived from the Momentum optimizer in deep learning, which adjusts the velocity of a model's parameters based on gradients and previous updates. Similarly, the Momentum strategy adapts based on the opponent's previous actions.

Key Features:

Momentum Formula: The strategy uses an alpha parameter to weight the influence of the previous momentum value and the current opponent's action (cooperate or defect). This formula is inspired by the Momentum optimizer in deep learning, which accelerates convergence by adjusting updates with a combination of previous gradients and the current gradient.
Threshold: A threshold value determines the decision to cooperate or defect. If the momentum is above the threshold, the strategy cooperates; otherwise, it defects.
Initial Momentum: When the history is empty (first round), the momentum is set to 1.0, and cooperation is guaranteed.
Implementation Details:

The strategy uses an alpha (momentum decay factor) and a threshold to determine the behavior.
The momentum is updated after each round based on the opponent's previous action, using the same principle as the Momentum optimizer.
The strategy's behavior evolves over time, with a more cooperative stance when momentum is high and a defection tendency when momentum is low.

@drvinceknight
Copy link
Member

drvinceknight commented Jan 22, 2025

Thanks for this @dongwonmoon.

The CI is failing with:

ImportError: cannot import name 'DataFrame' from 'dask.dataframe.core' 

I have been able to recreate this locally by rebuilding the tox environments:

$ python -m tox -r

The error is not caused by anything you're adding, it's something to do with an upstream update to task (I think!).

I have fixed the error by removing this line in test_resultset.py:

from dask.dataframe.core import DataFrame

I also noted when running things locally that you're going to need to run black on the two new files. If you need a hand with any of this let me know.

@dongwonmoon
Copy link
Contributor Author

dongwonmoon commented Jan 23, 2025

Thank you for the help with resolving the previous CI issue. However, a new issue has emerged.
It seems to be related to invalid escape sequences in the strings.

How sould i go about fixing this? @drvinceknight

@Nikoleta-v3
Copy link
Member

Hey 👋🏻 @dongwonmoon,

This isn’t exactly where the tests are failing. These are just warnings that invoke a warning message but don’t raise an error.

The tests are actually failing because of some other code that was recently added to the codebase. More specifically:

axelrod/strategies/frequency_analyzer.py:65: error: Need type annotation for "frequency_table" (hint: "frequency_table: dict[<type>, <type>] = ...")  [var-annotated]
Found 1 error in 1 file (checked 1 source file)

In another strategy that was introduced recently, one of the attributes’ types is not specified, and this is causing the MyPy tests to fail. Could you tweak line 65 in strategies/frequency_analyzer.py (see: link) to:

self.frequency_table: dict = dict()

The tests should pass after that 😅 (I’m about 90% sure). I assume it’ll be okay with the rest of the team if you include this fix in this pull request.

@dongwonmoon
Copy link
Contributor Author

Thank you for the detailed explanation. @Nikoleta-v3

This is my first PR, so I wanted to check—am I allowed to fix the error myself, or should I leave it for the maintainers to handle?

@drvinceknight
Copy link
Member

This is my first PR, so I wanted to check—am I allowed to fix the error myself, or should I leave it for the maintainers to handle?

I can do it if you'd like me to do it BUT usually what happens is you would make the change on the same branch you are already have (just adding a new commit). Then when you push it here it will just update.

Let me know if I can help with anything.

@dongwonmoon
Copy link
Contributor Author

Tests have been completed. Please let me know if there's anything else i should do.

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 looks good to me, I've made some minor stylistic suggestions as well as a request for a bit more documentation.

axelrod/strategies/momentum.py Outdated Show resolved Hide resolved
axelrod/strategies/momentum.py Outdated Show resolved Hide resolved
@dongwonmoon
Copy link
Contributor Author

I tried running black and isort locally, but nothing changed.
Can you give me some hint about this problem?

@drvinceknight
Copy link
Member

Can you give me some hint about this problem?

This could be an upstream update of black:

  ===== 5190 passed, 6 skipped, 1 xfailed, 78 warnings in 1163.11s (0:19:23) =====
  py311: commands[1]> python -m black -l 80 . --check
  would reformat /home/runner/work/Axelrod/Axelrod/axelrod/classifier.py
  would reformat /home/runner/work/Axelrod/Axelrod/axelrod/makes_use_of.py
  would reformat /home/runner/work/Axelrod/Axelrod/axelrod/strategies/memorytwo.py
  would reformat /home/runner/work/Axelrod/Axelrod/axelrod/tests/strategies/test_gambler.py

The CI installed black==25.1.0 could you check if that's different to the version you're running?

@dongwonmoon
Copy link
Contributor Author

dongwonmoon commented Jan 29, 2025

Thank you. Version was different.
It passed the test!

self.momentum = 1.0

def __repr__(self):
return f"Momentum: {self.alpha}, {self.threshold}"
Copy link
Member

Choose a reason for hiding this comment

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

Include {self.momentum} in the name as well for completeness.

Copy link
Member

@Nikoleta-v3 Nikoleta-v3 left a comment

Choose a reason for hiding this comment

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

Thank you, @dongwonmoon, again for your PR. One small comment: since the player depends on alpha, threshold, and momentum, all three values can be included in the name.

Everything else looks great!

@Nikoleta-v3
Copy link
Member

Your def test_repr(self): is failing because the name was changed 😄

@dongwonmoon
Copy link
Contributor Author

Thank you. I applied that.
Please check and let me know if there is anything else I need to do

@marcharper marcharper merged commit 8fa56d2 into Axelrod-Python:dev Feb 3, 2025
7 checks passed
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.

4 participants