Add custom x axes to TensorBoard #38

naeioi · 2019-11-23T04:48:02Z

This PR adds support for custom x-axes to the TensorBoard scalar plot. The axis names are specified in the tensorboard output constructor.

class TensorBoardOutput(LogOutput):
    def __init__(self,
                 log_dir,
                 x_axes=None,
                 flush_secs=120,
                 histogram_samples=1e3):

When x_axes is None, it falls back to use iterations as the x-axis.
If any x_axis is not present in the scalar tabular. A warning will be logged to the console.
If all x_axes are not present, it falls back to use iteration as the x-axis.

Screenshots of an experiment with Epoch and TotalEnvSteps as x-axes.

I will open a separate PR in garage repo to set TotalEnvSteps as the default x-axis.

codecov · 2019-11-23T04:48:17Z

Codecov Report

Merging #38 into master will decrease coverage by 0.07%.
The diff coverage is 93.93%.

@@            Coverage Diff             @@
##           master      #38      +/-   ##
==========================================
- Coverage    94.2%   94.13%   -0.08%     
==========================================
  Files           7        7              
  Lines         328      358      +30     
  Branches       48       58      +10     
==========================================
+ Hits          309      337      +28     
  Misses         12       12              
- Partials        7        9       +2

Impacted Files	Coverage Δ
src/dowel/logger.py	`93.67% <ø> (-0.08%)`	⬇️
src/dowel/tensor_board_output.py	`95.55% <93.93%> (-1.06%)`	⬇️

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 1ae0b7c...b76c6db. Read the comment docs.

ryanjulian · 2019-11-25T20:51:42Z

If any x_axis is not present in the scalar tabular. A warning will be logged to the console.

I think this should be an error -- you told me to expect a key and then didn't give it to me.

ryanjulian · 2019-11-25T20:53:07Z

I'm actually willing to endure the API breakage of not implicitly counting itrs in TensorBoardOutput -- because I think that it's a bad pattern.

@krzentner WDYT?

tests/dowel/test_tensor_board_output.py

src/dowel/logger.py

src/dowel/tensor_board_output.py

ryanjulian · 2019-11-25T21:07:05Z

src/dowel/tensor_board_output.py

+                for axis in self._x_axes:
+                    if axis not in nonexist_axes and key is not axis:
+                        x = data.as_dict[axis]
+                        self._record_kv('{}/{}'.format(axis, key), value, x)


if you were going to do this, you should use the prefix feature.

I recommend the following pattern, though, which I think will lead to better TensorBoard displays:

Default x-axis (either specific or just x_axes[0]) gets unmodified keys, such as Policy/Entropy

Additional x-axes get keys pre- or post-fixed with some non-/ char and their x-axis

e.g.:

post: : Policy/Entropy@Itr

pre: Itr@Policy/Entropy

I recommend you try both and see which is most intuitive for someone who doesn't know how to write a regex.

These are intended to:

Make it easy to filter the TB display to show only one kind of x-axis using regexes

Preserve the sections in the web-app, which are defined by the first level of / tokens

Default x-axis (either specific or just x_axes[0]) gets unmodified keys, such as Policy/Entropy

This is probably not a good idea, because the axis name is not displayed on tensorboard.

post: : Policy/Entropy@Itr

pre: Itr@Policy/Entropy

I like the post one. The pre one will double the number of sections if there are two axes, which is visually distracting.

re: defaults
Most users will be using one x-axis key, and the default should not be ugly. Perhaps you can accept both a str and list for x_axes as courtesy to users and only apply labels in the list case. I still think this isn't quite right, because even users of more than one x-axis have a default in mind which they will look at 90% of the time, and only use the others for debugging specific issues.

Alternatively, you could change the API to have 2 args: x_axis and additional_x_axes, and only label the for additional_x_axes. This is a little less magical than what I suggested with using x_axes[0].

As of today, we also don't have axis labels and the default is implicitly Itr but noted nowhere, so we already have the problem you mentioned today. AFAIK nobody's ever asked me what our x-axis represents. If you are scared of not exposing necessary information, you could always output a Text summary which tells the user how TensorBoardOutput is configured. In practice, I doubt that. This is a basic flaw in the TensorBoard interface, so we're not going to be able to solve it fully.

re:pre- vs post-fix
It's all about which distraction you prefer. It might be better to have all of the plots of one x-axis type grouped into their own section, so you can look at just those without using a regex.

Perhaps you can make examples of both for us to play with?

re: defaults
When the axis name is not displayed on tensorboard, printing this name to console helps a bit, but will easily be omitted. There are simply too many messages on the console and people will only scrutinize them when the program fails.

TensorBoard has the flaw of not displaying the x-axis name, but your suggestion of using @axis is an excellent workaround to hint people what they are reading.

Plus, the default x-axis is specified in experiment_wrapper.py, which is a file that most regular users will not touch. Don't expect anyone to know what is x-axis.

Printing this name to TensorBoard will add some redundancy, but I think it wins by being much easier to understand.

@axis is nice for non-defaults but it makes the default case worse, which is the case which 90% of users will only ever experience. this is pretty unpythonic. see: https://www.python.org/dev/peps/pep-0008/#a-foolish-consistency-is-the-hobgoblin-of-little-minds

this intention here is to establish that the default x-axis in garage is always TotalEnvSteps. we can put it in the documentation and all over the code. beating users over the head with it over and over again in the TensorBoard is consistent in the way that makes programmers feel "correct" and all warm and fuzzy inside, and makes life harder for everyone else for no reason.

i'll add more reviewers to ask what they think. if you really want to convince people your way is better then make some demos. you are proposing changing the status quo, so the burden is on you to convince everyone.

I will take your suggestion of using x_axis + additional_x_axes as TensorboardOutput's interface.

Meanwhile, I want to point out that TotalEnvSteps as x-axis is not the status quo in open-source frameworks' support for Tensorboard. Most frameworks use iterations, just like what garage is doing now. To list some of the most popular ones,

Dopamine
tf-agents
ray-rllib, code1, code2
keras-rl

So after this PR, garage will be one of the few frameworks, if not the only one, that support using TotalEnvSteps as the x-axis in TensorBoard.

Yes, and IMO it will be a great differentiator. One of the great hassles of using RL libraries is that the x-axis of the output logs differ greatly between libraries when the mathematically-valid x-axis is always TotalEnvSteps (or perhaps TotalGradSteps).

Publications use TotalEnvSteps almost universally to compare algorithms. Determining the actual number of TotalEnvSteps, if it's not explicitly logged, can itself be a challenge which takes hours to figure out in an unfamiliar codebase, depending on how obfuscated the code is.

btw, tf_agents uses the global gradient step counter (e.g. TotalGradSteps), not Itr as defined by garage. This can make a lot sense sometimes in off-policy algorithms, but is mostly a hold-over from the practices used for supervised learning. The relevant publication benchmark is still TotalEnvSteps.

ryanjulian · 2019-11-25T21:07:51Z

If you want to share a sample with us, you can use https://tensorboard.dev

src/dowel/tensor_board_output.py

src/dowel/logger.py

naeioi · 2019-11-27T00:49:05Z

I found that allowing missing axes is inevitable. For example, DDPG calls logger.dump() every epoch, but the scalar table is empty for the beginning several epochs. So I keep this as a warning but not an error.

I also found that '@' in the name will be replaced with underline by tensorboard. The followings are candidate naming formats.

post_slash: https://tensorboard.dev/experiment/sydhawaaQr2BFlCnqzF2qQ/#scalars
post_underline: https://tensorboard.dev/experiment/wQsEzHaGQkekfnB5GqSz9g/#scalars
pre_slash: https://tensorboard.dev/experiment/zZi3idwRRGOq4SVOybs2Nw/#scalars
pre_underline: https://tensorboard.dev/experiment/Bt1o8m20S6Op5nXInfQgvA/#scalars
pre_full_axes_name: https://tensorboard.dev/experiment/jUElLlA3TQu7d7AskiBuvw/#scalars

ryanjulian · 2019-11-27T00:52:12Z

Ah, let me take a look at those. What a bummer that TF removes the @

avnishn

LGTM

naeioi requested a review from a team as a code owner November 23, 2019 04:48

naeioi assigned ryanjulian and krzentner Nov 23, 2019

naeioi mentioned this pull request Nov 23, 2019

Set TotalEnvSteps as the default Tensorboard x-axis rlworkgroup/garage#1069

Merged

naeioi unassigned ryanjulian and krzentner Nov 23, 2019

naeioi requested review from ryanjulian and krzentner November 23, 2019 06:18

ryanjulian reviewed Nov 25, 2019

View reviewed changes

tests/dowel/test_tensor_board_output.py Outdated Show resolved Hide resolved

ryanjulian reviewed Nov 25, 2019

View reviewed changes

src/dowel/logger.py Outdated Show resolved Hide resolved

ryanjulian reviewed Nov 25, 2019

View reviewed changes

src/dowel/tensor_board_output.py Outdated Show resolved Hide resolved

ryanjulian reviewed Nov 25, 2019

View reviewed changes

ryanjulian requested review from ahtsan, zhanpenghe and utkarshjp7 November 26, 2019 01:03

ryanjulian reviewed Nov 27, 2019

View reviewed changes

src/dowel/tensor_board_output.py Outdated Show resolved Hide resolved

ryanjulian reviewed Nov 27, 2019

View reviewed changes

src/dowel/logger.py Outdated Show resolved Hide resolved

ryanjulian approved these changes Nov 27, 2019

View reviewed changes

ryanjulian requested review from krzentner and removed request for zhanpenghe and krzentner December 3, 2019 01:14

naeioi requested a review from avnishn December 4, 2019 00:27

avnishn approved these changes Dec 4, 2019

View reviewed changes

Add custom x axes

6228c46

naeioi added 5 commits December 3, 2019 17:34

Address comments

c67eaff

Address comments

1b9ff76

Fix CI

5c20cce

Fix CI

03c08ba

Use 'key/axis' naming in Tensorboard

63e6c31

naeioi force-pushed the custom_x_axes branch from 3c4f489 to 63e6c31 Compare December 4, 2019 01:34

Update test

b76c6db

ryanjulian merged commit 7b9fed2 into master Dec 4, 2019

ryanjulian deleted the custom_x_axes branch December 4, 2019 02:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add custom x axes to TensorBoard #38

Add custom x axes to TensorBoard #38

naeioi commented Nov 23, 2019

codecov bot commented Nov 23, 2019 •

edited

Loading

ryanjulian commented Nov 25, 2019

ryanjulian commented Nov 25, 2019

ryanjulian Nov 25, 2019

naeioi Nov 26, 2019

naeioi Nov 26, 2019

ryanjulian Nov 26, 2019

naeioi Nov 26, 2019 •

edited

Loading

ryanjulian Nov 26, 2019

naeioi Nov 26, 2019 •

edited

Loading

ryanjulian Nov 26, 2019

ryanjulian commented Nov 25, 2019

naeioi commented Nov 27, 2019 •

edited

Loading

ryanjulian commented Nov 27, 2019

avnishn left a comment

Add custom x axes to TensorBoard #38

Add custom x axes to TensorBoard #38

Conversation

naeioi commented Nov 23, 2019

codecov bot commented Nov 23, 2019 • edited Loading

Codecov Report

ryanjulian commented Nov 25, 2019

ryanjulian commented Nov 25, 2019

ryanjulian Nov 25, 2019

Choose a reason for hiding this comment

naeioi Nov 26, 2019

Choose a reason for hiding this comment

naeioi Nov 26, 2019

Choose a reason for hiding this comment

ryanjulian Nov 26, 2019

Choose a reason for hiding this comment

naeioi Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

ryanjulian Nov 26, 2019

Choose a reason for hiding this comment

naeioi Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

ryanjulian Nov 26, 2019

Choose a reason for hiding this comment

ryanjulian commented Nov 25, 2019

naeioi commented Nov 27, 2019 • edited Loading

ryanjulian commented Nov 27, 2019

avnishn left a comment

Choose a reason for hiding this comment

codecov bot commented Nov 23, 2019 •

edited

Loading

naeioi Nov 26, 2019 •

edited

Loading

naeioi Nov 26, 2019 •

edited

Loading

naeioi commented Nov 27, 2019 •

edited

Loading