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

Implement two classical control problems for testing reinforcement learning method #989

Merged
merged 6 commits into from May 9, 2017

Conversation

Projects
None yet
3 participants
@ShangtongZhang
Member

ShangtongZhang commented Apr 25, 2017

No description provided.

@mlpack-jenkins

This comment has been minimized.

Show comment
Hide comment
@mlpack-jenkins

mlpack-jenkins Apr 25, 2017

Can one of the admins verify this patch?

mlpack-jenkins commented Apr 25, 2017

Can one of the admins verify this patch?

@ShangtongZhang

This comment has been minimized.

Show comment
Hide comment
@ShangtongZhang

ShangtongZhang Apr 25, 2017

Member
*****************************************unknown location(0): fatal error in "LogChebychevApproxSdp": std::runtime_error: chol(): failed to converge
/home/travis/build/mlpack/mlpack/src/mlpack/tests/sdp_primal_dual_test.cpp(217): last checkpoint
**********
Member

ShangtongZhang commented Apr 25, 2017

*****************************************unknown location(0): fatal error in "LogChebychevApproxSdp": std::runtime_error: chol(): failed to converge
/home/travis/build/mlpack/mlpack/src/mlpack/tests/sdp_primal_dual_test.cpp(217): last checkpoint
**********
@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq Apr 26, 2017

Member

Don't worry about the test, many mlpack tests are probabilistic, that fail in some cases e.g. if they don't converge in the specified number of iterations.

Member

zoq commented Apr 26, 2017

Don't worry about the test, many mlpack tests are probabilistic, that fail in some cases e.g. if they don't converge in the specified number of iterations.

@ShangtongZhang

This comment has been minimized.

Show comment
Hide comment
@ShangtongZhang

ShangtongZhang Apr 29, 2017

Member

I refactored the implementation, looking forward to any feedback

Member

ShangtongZhang commented Apr 29, 2017

I refactored the implementation, looking forward to any feedback

@ShangtongZhang

This comment has been minimized.

Show comment
Hide comment
@ShangtongZhang

ShangtongZhang May 1, 2017

Member

Hopefully it's now ready to merge

Member

ShangtongZhang commented May 1, 2017

Hopefully it's now ready to merge

* Initial velocity is 0.
* @return Initial state for each episode.
*/
State InitialSample() const

This comment has been minimized.

@zoq

zoq May 3, 2017

Member

We could avoid a copy if we pass the state by reference if the State is complex this could speed things up, but I'm not sure that is necessary here. What do you think?

@zoq

zoq May 3, 2017

Member

We could avoid a copy if we pass the state by reference if the State is complex this could speed things up, but I'm not sure that is necessary here. What do you think?

This comment has been minimized.

@ShangtongZhang

ShangtongZhang May 3, 2017

Member

I think it will be optimized by Return Value Optimization even with -o0 unless user specifies -fno-elide-constructors. In the other hand, by using const State & s = InitialSample() user can explicitly extend the lifetime of the local variable, so there is also no overhead. If we refactor it to const State& InitialSample() const, it seems to be an undefined behavior per this and clang does gives a warning for it.

@ShangtongZhang

ShangtongZhang May 3, 2017

Member

I think it will be optimized by Return Value Optimization even with -o0 unless user specifies -fno-elide-constructors. In the other hand, by using const State & s = InitialSample() user can explicitly extend the lifetime of the local variable, so there is also no overhead. If we refactor it to const State& InitialSample() const, it seems to be an undefined behavior per this and clang does gives a warning for it.

This comment has been minimized.

@zoq

zoq May 4, 2017

Member

Thanks for your thoughts and clarification; besides some minor style issues, I think this is ready to be merged. I'll go and fix the issues once I merge this in.

@zoq

zoq May 4, 2017

Member

Thanks for your thoughts and clarification; besides some minor style issues, I think this is ready to be merged. I'll go and fix the issues once I merge this in.

This comment has been minimized.

@ShangtongZhang

ShangtongZhang May 4, 2017

Member

That's cool! Thanks for your review!

@ShangtongZhang

ShangtongZhang May 4, 2017

Member

That's cool! Thanks for your review!

@zoq zoq merged commit 6d790c1 into mlpack:master May 9, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@zoq

This comment has been minimized.

Show comment
Hide comment
@zoq

zoq May 9, 2017

Member

Merged, I made some minor style fixes in: 859d813 let me know if I missed something.

Member

zoq commented May 9, 2017

Merged, I made some minor style fixes in: 859d813 let me know if I missed something.

@ShangtongZhang ShangtongZhang deleted the ShangtongZhang:rl-env branch May 16, 2017

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