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

Refactoring master before async merge #23

Closed
lake4790k opened this issue May 27, 2016 · 1 comment · Fixed by #30
Closed

Refactoring master before async merge #23

lake4790k opened this issue May 27, 2016 · 1 comment · Fixed by #30

Comments

@lake4790k
Copy link
Collaborator

Before the async methods from async branch are merged master could be refactored to allow for reusing most of the code between async and ER methods. Also I believe this refactoring would improve the structure of the existing ER solution in itself. Currently single files (classes) contain functionality for multiple aspects, would be better to create objects dealing with one aspect at a time. I already started this approach in async if followed in master as well merge will be smooth and easy.

I suggest the following:

main would only contain option parsing. Currently it is also responsible for

  • training the ER Agent
  • validating the Agent
  • evaluating or demoing the Agent with one episode

These I would separate in classes

  • ERTrainer training loop
  • ValidationAgent validation loop, also the validation related functionality would be moved here from the ER Agent, ie. stats, report, saliency. This agent would also work with the async agents in its own thread in async mode later.
  • EvaluationAgent showcasing, movie making

So main would parse the options and then either create a

  • a single threaded ERTrainer + ValidationAgent
  • an EvaluationAgent
  • later the multithreaded async training / evaluation

I would not share the ER Agent code with async agents (1/n Q and A3C) for now. In the async agents I took the approach of having separate (subclassed) implementations of different methods to make it easier to look at one algorithm at a time. The ER Agent contains many methods, but all based on ER logic. I think this separation is fine. Much of the simplicity of async code comes from not having to deal with sampled batches.

Splitting up main is not strictly necessary the way I'm suggesting, we only gain the reuse of validation logic with it. Other parts can be still reused from async (eg. Model, CircularQueue, BinaryHeap) without doing this, but I do believe doing it would improve the ER code as well.

I would submit multiple PRs for the suggested steps, so would be easier to look at what's going on. @Kaixhin let me know if you like this or have other idea!

@Kaixhin
Copy link
Owner

Kaixhin commented May 27, 2016

Sounds like a sensible plan to me - thanks for working this out. I agree on the PR approach - if you can make a sequence of PRs to master, each adding a bit more modularity, then it'll be easier to check/look back on the commit history.

I'll leave this up to you, but I think having a partially abstract Agent as a base for all of these would be nice for readability.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants