-
Notifications
You must be signed in to change notification settings - Fork 263
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
Move classifiers outside of Players #1300
Conversation
Do the tests pass locally? |
…ssing for certain test players. This is noisy, not an error, and not helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also run black on these commits (if not done so already)? I'm surprised that so many indentations changed. It's fine if that's what black did.
docs/tutorials/contributing/strategy/adding_the_new_strategy.rst
Outdated
Show resolved
Hide resolved
FYI the travis build can be seen here: https://travis-ci.org/github/Axelrod-Python/Axelrod/pull_requests?utm_medium=notification&utm_source=github_status |
This is great feedback. Thanks! |
Looks really nice. I left a few minor comments/questions but otherwise I'm happy with it. |
Thanks for my requested modifications. Looks good to me 👍 |
The failing test looks like it's got something to do with the filter set:
|
Also the warning comes up a lot during the tests, perhaps we want to suppress it, or change various invocations to use an instance instead. |
I don't know how I'm supposed to get that last line of coverage. I can do something hacky... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's one suggestion @gaffney2010 (https://coverage.readthedocs.io/en/v4.5.x/excluding.html)
Co-Authored-By: Vince Knight <vincent.knight@gmail.com>
Thanks @gaffney2010 looks good 👍 |
I want to make it so that we can allow classifiers to be a function.
This will allow us to store the result of:
#740
to a classifier after we program.
It will also allow us to program in the memory-depth of FSMs that I had done previously; currently, we just ran and copied the values in.
There are probably other places where this functionality will help.
I thought about replacing the default_classifier dict with a dict whose values are lambdas. But decided against this because 1) it would really clutter the Player code and spread out classifier logic when we want to handle different strategies differently, and 2) it would make memoizing difficult (see below).
I decided to move the classifiers out of the Player class, and keep a table lookup in a singleton class called Classifiers. I made a function that runs all classifiers on all strategies and saves the result to a yaml function; Classifiers reads from this. This is necessary for classifiers that take a while to calculate. (This was the problem with memory-depth on FSMs.) I did a yaml file so that developers can check the calculated classifiers.
I continue to use the classifier dict in the strategies, where it exists, with the new logic as more of a replacement of the default classifier. This makes the change backwards compatible, but is also necessary for factory-generated strategies. I store the classifiers to the yaml also, which isn't necessary, but provides a nice check.
Most of the change here is switching classifier lookups to use the Classifiers class. This isn't technically necessary, because of the backwards compatibility, but this is how we should lookup.