-
Notifications
You must be signed in to change notification settings - Fork 268
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
Integrate dojo params into Axelrod #1256
Conversation
I'll aim to take a close look through on Thursday 👍 |
@@ -56,6 +56,8 @@ def __init__( | |||
interaction_graph: Graph = None, | |||
reproduction_graph: Graph = None, | |||
fitness_transformation: Callable = None, | |||
mutation_method="transition", |
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.
how do we feel about having booleans instead of passing a string argument?
atomic=False, transition=True
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.
You can't have both at the same time so I'd prefer not to use individual booleans. I considered using an enum instead.
This looks like it'll be a great addition/move. I think we might need to document some of this somewhere, perhaps in the docstrings of the particular sublayers or perhaps better to have a section in the documentation showing how one of them can be used and how it behaves (and pointing at all the others)? |
For sure, we'll want to add some more documentation. If we're settled on the API and other changes that I've made then I'll add some docs but I didn't want to yet in case there were some major objections. We may also want to move the rest of the dojo over -- it's basically just the two algorithms and the invocation scripts now. I'd prefer to do that in another PR though. That should be easy, this PR is the deeper integration. |
Yeah sounds good. I think I'm settled on the API, very nice work. Having some example use cases documented will just confirm that for me 👍
I'd prefer to leave the invocation scripts out of the library but I agree with the algorithms 👍 |
…t take a mutation_probability
09c21ed
to
c7e6b61
Compare
a850ec9
to
e95b41c
Compare
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.
Just this little docstring please, everything looks good to me otherwise. 👍
Thanks @marcharper, this looks good to me. 👍 |
@meatballs ready for you when you get a chance |
Re: #1240, this PR merges the Parameter classes for the training algorithms as evolvable players in Axelrod by introducing a subclass of each of the archetypes. The
init_kwargs
attribute now does most of the heavy lifting and there are many new tests to ensure identical play.The dojo will also be simultaneously updated (see this associated PR).
There are a few uncovered lines and mypy tests to be fixed but I think it's ready for someone else to look at.