-
Notifications
You must be signed in to change notification settings - Fork 310
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
Replace ContinuousMLPQFunction #892
Conversation
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.
LGTM
Overall this LGTM. I'm a little concerned about a couple of the results on HalfCheetah and Walker2d (the hardest environments). It a couple cases seem inconsistent. I don't think it's indicative of a problem with the primitive, but instead it may be the algorithm. Can you check that we're using the same exploration strategy for both the baseline and our algo? I think @utkarshjp7 may have also unearthed some subtle differences between our DDPG and the baselines one, which might explain some differences. |
Utkarsh actually thought DDPG is fine, he just found something for TD3. I will have another pass on it and run benchmark on these two harder environments and see if it persist. |
I ran benchmark again and it looks better. Perhaps it's a matter of random seed? |
Yes I suspect it probably has something to do with these environments sensitivity to initialization and/or exploration. Here's a way you can check (which will help people in the future running benchmarks): Run the same 2 experiments here but using 30 random seeds. Plots the mean results (across all 30 seeds) for garage and garage_models (that's 2 curves) with a confidence interval. This is very easy to do with I suspect we will find that the confidence interval is wider than the variation we are seeing on the plots. |
Awesome! Feel free to merge this. If you can somehow add the code you used to produce these plots to master (and put both curves on the same plot), I think it would be super helpful for future benchmarking. At least post a branch or a gist for the people currently working on the models transition. |
8b7e6e0
to
ea36f13
Compare
ea36f13
to
cefde74
Compare
Codecov Report
@@ Coverage Diff @@
## master #892 +/- ##
===========================================
+ Coverage 50.84% 74.08% +23.23%
===========================================
Files 181 180 -1
Lines 10301 10250 -51
Branches 1323 1317 -6
===========================================
+ Hits 5238 7594 +2356
+ Misses 4814 2323 -2491
- Partials 249 333 +84
Continue to review full report at Codecov.
|
This PR does the following:
Summary: In most cases model-version performs better or at least as good as the old one.
For benchmark scripts, checkout the branch origin/benchmark_continuous_mlp_q_function.
Benchmark results:
![image](https://user-images.githubusercontent.com/10270515/65409189-ef8a5300-dd9b-11e9-9537-95f1ac97be64.png)
![image](https://user-images.githubusercontent.com/10270515/65409403-80f9c500-dd9c-11e9-9943-0646d65c162f.png)
![image](https://user-images.githubusercontent.com/10270515/65409241-0e88e500-dd9c-11e9-9364-d04af128f5f0.png)
![image](https://user-images.githubusercontent.com/10270515/65409276-23fe0f00-dd9c-11e9-9436-6109cc338b53.png)
![image](https://user-images.githubusercontent.com/10270515/65409302-38daa280-dd9c-11e9-80ae-2b34c8f27ce5.png)
![image](https://user-images.githubusercontent.com/10270515/65409349-5576da80-dd9c-11e9-811d-3532b0ed0367.png)
![image](https://user-images.githubusercontent.com/10270515/65409366-63c4f680-dd9c-11e9-8147-057bf5fa95ca.png)
This and #888 will close #526.