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

Set TotalEnvSteps as the default Tensorboard x-axis #1069

Merged
merged 4 commits into from
Dec 5, 2019

Conversation

naeioi
Copy link
Member

@naeioi naeioi commented Nov 23, 2019

This PR sets TotalEnvSteps as the default tensorboard scalar log x-axis. This PR depends on rlworkgroup/dowel#38.

@naeioi naeioi requested a review from a team as a code owner November 23, 2019 04:56
@naeioi naeioi changed the title Set TotalEnvSteps as the default Tensorboard output Set TotalEnvSteps as the default Tensorboard x-axis Nov 23, 2019
@naeioi naeioi assigned ryanjulian and krzentner and unassigned ryanjulian and krzentner Nov 23, 2019
@ryanjulian
Copy link
Member

We need to merge the dowel changes first, release a dowel version, then bump the version in garage.

@ryanjulian ryanjulian mentioned this pull request Nov 26, 2019
@ryanjulian ryanjulian requested a review from a team December 3, 2019 01:14
@ghost ghost requested review from gitanshu and removed request for a team December 3, 2019 01:14
Copy link
Member

@avnishn avnishn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Additionally, I like this feature, it is useful for comparing against other paper results that log in this fashion

@codecov
Copy link

codecov bot commented Dec 4, 2019

Codecov Report

Merging #1069 into master will increase coverage by 4.14%.
The diff coverage is 50%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1069      +/-   ##
==========================================
+ Coverage      82%   86.15%   +4.14%     
==========================================
  Files         161      161              
  Lines        7809     7807       -2     
  Branches      992      992              
==========================================
+ Hits         6404     6726     +322     
+ Misses       1208      890     -318     
+ Partials      197      191       -6
Impacted Files Coverage Δ
src/garage/experiment/experiment_wrapper.py 86.04% <50%> (+0.55%) ⬆️
src/garage/misc/tensor_utils.py 88.23% <0%> (-2.36%) ⬇️
...rc/garage/sampler/off_policy_vectorized_sampler.py 97.75% <0%> (+1.12%) ⬆️
src/garage/tf/algos/npo.py 95.29% <0%> (+1.56%) ⬆️
src/garage/envs/base.py 91.37% <0%> (+1.72%) ⬆️
src/garage/tf/misc/tensor_utils.py 76.11% <0%> (+2.98%) ⬆️
.../exploration_strategies/epsilon_greedy_strategy.py 100% <0%> (+3.7%) ⬆️
src/garage/tf/policies/categorical_cnn_policy.py 100% <0%> (+4.25%) ⬆️
src/garage/tf/baselines/continuous_mlp_baseline.py 100% <0%> (+4.54%) ⬆️
src/garage/tf/algos/vpg.py 81.81% <0%> (+9.09%) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4c9e4c4...5efc084. Read the comment docs.

@naeioi
Copy link
Member Author

naeioi commented Dec 4, 2019

This PR will be ready when dowel 0.0.3 is tagged.

@naeioi naeioi force-pushed the set_default_axis_tbx branch from 87cb32a to e717b5d Compare December 4, 2019 04:07
@ryanjulian
Copy link
Member

Let me know when this is ready and I will force-push it.

@naeioi
Copy link
Member Author

naeioi commented Dec 5, 2019

@ryanjulian If we don't care about the codedev, then this should be ready.
The uncovered code is the serializer in the experiment wrapper.

@ryanjulian ryanjulian merged commit 1966673 into master Dec 5, 2019
@ryanjulian ryanjulian deleted the set_default_axis_tbx branch December 5, 2019 19:53
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.

4 participants