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 Continuous Mountain Car to Reinforcement Learning Environment #1368

Merged
merged 10 commits into from Apr 27, 2018

Conversation

Projects
None yet
3 participants
@mirraaj
Contributor

mirraaj commented Apr 15, 2018

This is an implementation of continuous mountain car game available on Openai Gym.
Reference : https://gym.openai.com/envs/MountainCarContinuous-v0/

It is difficult to write a test file as reinforcement learning module doesn't support actor-critic methods presently . Thus for the coming week, I will be working to add DDPG support to mlpack. This algorithm can be then used to test the Continous Car environment.

DDPG Paper : https://arxiv.org/abs/1509.02971

@mirraaj mirraaj changed the title from Added Continous Mountain Car to Added Continous Mountain Car to Reinforcement Learning Environment Apr 15, 2018

mirraaj added some commits Apr 15, 2018

@mirraaj mirraaj changed the title from Added Continous Mountain Car to Reinforcement Learning Environment to Added Continuous Mountain Car to Reinforcement Learning Environment Apr 15, 2018

@mirraaj

This comment has been minimized.

Contributor

mirraaj commented Apr 15, 2018

Dear @zoq , @rcurtin I have added continuous mountain car game to mlpack. I have also added the tests for the same and it is working. I would like to know if this is ready to be added or should I proceed with DDPG implementation as well.

@zoq

This comment has been minimized.

Member

zoq commented Apr 16, 2018

Thanks, from my side I think this is still valuable without DDPG. So, I'll take a closer look at the code and make comments if necessary, so that we can merge this one.

@mirraaj

This comment has been minimized.

Contributor

mirraaj commented Apr 16, 2018

@zoq. Please let me know if any changes are required. Thanks :)

@mirraaj

This comment has been minimized.

Contributor

mirraaj commented Apr 17, 2018

@zoq : for style error, merging #1372 will solve the problem.

/**
* Construct a state instance.
*/
State(): data(dimension, arma::fill::zeros)

This comment has been minimized.

@zoq

zoq Apr 18, 2018

Member

Pedantic style issue; missing space between :.

/**
* Construct a state based on the given data.
*
* @param data Data for the velocityand position.

This comment has been minimized.

@zoq

zoq Apr 18, 2018

Member

Missing space between: velocityand.

*/
struct Action
{
double action[1];

This comment has been minimized.

@zoq

zoq Apr 18, 2018

Member

Can you add a comment here what this action does? Also what do you think, should we use enum here, I think this would make the overall code more readably, especially the step part.

{
// Calculate acceleration.
double force = std::min(std::max(action.action[0], -1.0), 1.0);
nextState.Velocity() = state.Velocity() + 0.001 * force - 0.0025 *

This comment has been minimized.

@zoq

zoq Apr 18, 2018

Member

Should we make 0.001 and 0.0025 configurable?

{
State state;
state.Velocity() = 0.0;
state.Position() = arma::as_scalar(arma::randu(1)) * 0.2 - 0.6;

This comment has been minimized.

@zoq

zoq Apr 18, 2018

Member

What about using:

inline double Random()
{
return randUniformDist(randGen);
}

that way we could avoid arma::as_scalar.

@zoq

zoq approved these changes Apr 24, 2018

No more comments from my side, thanks. I'll go ahead and merge this in 3 days to leave time for any other comments.

@rcurtin

Looks good to me, thanks for the contribution! :)

@zoq zoq merged commit e14ba25 into mlpack:master Apr 27, 2018

4 of 5 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
Memory Checks
Details
Static Code Analysis Checks Build finished.
Details
Style Checks Build finished.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@zoq

This comment has been minimized.

Member

zoq commented Apr 27, 2018

Thanks for another great contribution.

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