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

Added Python efr algorithm implementation #1094

Merged
merged 22 commits into from
May 31, 2024

Conversation

Jamesflynn1
Copy link
Contributor

@Jamesflynn1 Jamesflynn1 commented Jul 8, 2023

A Python implementation of the EFR (https://arxiv.org/abs/2102.06973) algorithm with the deviation types defined in the proposing paper. The implementation was developed as part of my undergraduate dissertation and I thought it would be beneficial if the implementation could be made available as part of OpenSpiel.

Uses a shortcut for external only deviations as found in https://github.com/dmorrill10/hr_edl_experiments (original experimental code)

Any guidance or feedback would be appreciated as this is my first contribution to an open source project.

Todo:

  • Look into issues with behavioural deviations
  • Look into possible incorrect definition with CSPS deviations
  • Add docstrings for all functions
  • Remove discounting code
  • Refactor code
  • Ensure adherence to Google Python style
  • Add updates to OpenSpiel docs

@google-cla
Copy link

google-cla bot commented Jul 8, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@lanctot
Copy link
Collaborator

lanctot commented Jul 22, 2023

Thanks @Jamesflynn1 !

Please let us know when it is ready for a review.

open_spiel/python/algorithms/efr.py Show resolved Hide resolved
open_spiel/python/algorithms/efr.py Outdated Show resolved Hide resolved
@Jamesflynn1 Jamesflynn1 marked this pull request as ready for review September 21, 2023 00:51
Copy link
Collaborator

@lanctot lanctot left a comment

Choose a reason for hiding this comment

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

Please fix the style to conform to Google Python style. I've pointed out a few examples, but please fix them all.

Use pylint with the pylintrc from the Google style guide

open_spiel/python/algorithms/efr_test.py Outdated Show resolved Hide resolved
open_spiel/python/algorithms/efr_test.py Show resolved Hide resolved
open_spiel/python/algorithms/efr.py Show resolved Hide resolved
open_spiel/python/algorithms/efr.py Show resolved Hide resolved
open_spiel/python/algorithms/efr.py Outdated Show resolved Hide resolved
open_spiel/python/algorithms/efr.py Show resolved Hide resolved
open_spiel/python/algorithms/efr.py Show resolved Hide resolved
@lanctot
Copy link
Collaborator

lanctot commented Oct 31, 2023

Hi @Jamesflynn1, if any of your recent commits have addressed the comments, can you reply to them and/or mark them as resolved?

@Jamesflynn1
Copy link
Contributor Author

Hi @Jamesflynn1, if any of your recent commits have addressed the comments, can you reply to them and/or mark them as resolved?

Hi @lanctot, apologies I haven't replied sooner, I have been very busy with university coursework and graduate applications.

I ran pylint with the Google style guide and I was still some of style issues (spaces after comments and blank lines), I'll take another look now and mark off the solved issues.

@Jamesflynn1
Copy link
Contributor Author

Hi @lanctot,

I have tried to refractor the files using the linked pylintrc file and I'm not getting all the commenting suggestions that has been pointed out. I've fixed (hopefully) all instances of the issues you have pointed out. However, I think it's possible that I have a different pylintrc file (I get no linting issues when I pylint the file) so to catch all issues would it be possible to compare files?

Any assistance would be appreciated!

Thanks,
James

@lanctot
Copy link
Collaborator

lanctot commented Jan 28, 2024

No problem, I can try to fix these on our side. Apologies for the delays.

@lanctot
Copy link
Collaborator

lanctot commented Apr 16, 2024

Hi @Jamesflynn1 , sorry for the extreme lateness. Did you ever verify the convergence matches what was expected from the papers? I'd like to import this, but it would be good to know that it's correct.

@lanctot lanctot self-requested a review April 16, 2024 09:36
Copy link
Collaborator

@lanctot lanctot left a comment

Choose a reason for hiding this comment

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

Can you add efr_test.py to the python/CMakeLists.txt

We need to ensure that it runs on Github Actions CI before we import it.

You might need to pull changes from master because that file has changed a few times since our last contact.

@lanctot lanctot added imported This PR has been imported and awaiting internal review. Please avoid any more local changes, thanks! merged internally The code is now submitted to our internal repo and will be merged in the next github sync. labels May 27, 2024
@lanctot lanctot merged commit f43d058 into google-deepmind:master May 31, 2024
2 checks passed
@lanctot
Copy link
Collaborator

lanctot commented Aug 19, 2024

Hi @Jamesflynn1

There seems to be an issue now on Github Actions CI (numpy issue causing the EFR test to fail)

Can you take a look? #1266

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imported This PR has been imported and awaiting internal review. Please avoid any more local changes, thanks! merged internally The code is now submitted to our internal repo and will be merged in the next github sync.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants