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

Integrate dojo params into Axelrod #1256

Merged
merged 47 commits into from
Oct 16, 2019
Merged

Integrate dojo params into Axelrod #1256

merged 47 commits into from
Oct 16, 2019

Conversation

marcharper
Copy link
Member

@marcharper marcharper commented Sep 3, 2019

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.

@drvinceknight
Copy link
Member

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",
Copy link
Member

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

Copy link
Member Author

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.

@drvinceknight
Copy link
Member

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)?

@marcharper
Copy link
Member Author

marcharper commented Sep 6, 2019

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.

@drvinceknight
Copy link
Member

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.

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 👍

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.

I'd prefer to leave the invocation scripts out of the library but I agree with the algorithms 👍

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.

Just this little docstring please, everything looks good to me otherwise. 👍

axelrod/graph.py Show resolved Hide resolved
@drvinceknight
Copy link
Member

Thanks @marcharper, this looks good to me. 👍

@marcharper
Copy link
Member Author

@meatballs ready for you when you get a chance

@meatballs meatballs merged commit 72a5148 into master Oct 16, 2019
@meatballs meatballs deleted the integrate_dojo branch October 16, 2019 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants