Support new-style audio summaries #345

wchargin · 2017-08-11T21:02:09Z

Summary:
This commit adds support for new-style audio summaries to the plugin
frontend and backend.

Here is a screenshot of the resulting dashboard:

This change is fully compatible with the old data format.

Test Plan:
Run bazel run //tensorboard/plugins/audio:audio_demo to get data
using new-style audio summaries. Also grab some old-style audio
summaries, perhaps by running the version of that target that appeared
before this commit (and changing the logdir to avoid writing into the
same directory). Launch a TensorBoard with a logdir enclosing all of
this data, and verify that it all renders correctly.

When running bazel test //tensorboard/..., be aware that this diff
contains a particularly large number of uses of tf.compat.as_bytes and
friends. Thus, it is wise to run this test command in both py2 and py3
virtualenvs.

wchargin-branch: new-style-audio-frontend

wchargin · 2017-08-11T22:29:55Z

tensorboard/plugins/audio/audio_demo.py


 FLAGS = tf.flags.FLAGS

 tf.flags.DEFINE_string('logdir', '/tmp/audio_demo',
                       'Directory into which to write TensorBoard data.')

-tf.flags.DEFINE_integer('steps', 500,
+tf.flags.DEFINE_integer('steps', 50,


(Note: I made this change because it cuts down the output from 617.8 MB to "only" 79.6 MB, not because of anything to do with the new summaries.)

Sounds worth it to me. Maybe note in PR description.

chihuahua

Looking forward to all the summaries using SummaryMetadata! Almost there.

chihuahua · 2017-08-11T22:39:04Z

tensorboard/backend/application.py

@@ -41,12 +41,12 @@
 from tensorboard.backend.event_processing import event_multiplexer
 from tensorboard.plugins import base_plugin
 from tensorboard.plugins.core import core_plugin
+from tensorboard.plugins.audio import metadata as audio_metadata


Nit: I think audio comes before core.

After deep self-reflection, I think so, too. thanks :-)

chihuahua · 2017-08-11T22:40:08Z

tensorboard/plugins/audio/tf_audio_dashboard/tf-audio-loader.html

+        },
+        _hasMultipleSamples: {
+          type: Boolean,
+          computed: "_computeHasMultipleSamples(ofSamples)",


Nit: Maybe numberOfSamples?

It's not clear to me what you're suggesting. We can't change ofSamples to numberOfSamples on this line (this is an argument name, not a parameter name); do you want to rename the property entirely?

Yeah - maybe renaming the property to numberOfSamples is more clear?

Okay. It introduces a dissimilarity between this and the image dashboard, but that's not the end of the world.

(I'm using totalSamples because with numberOfSamples one is more likely to incorrectly infer that a particular card has multiple samples; using totalSamples clarifies that the aggregation is done at a higher level than the individual cards.)

Sounds good.

chihuahua · 2017-08-11T22:45:59Z

tensorboard/plugins/audio/audio_demo.py


 FLAGS = tf.flags.FLAGS

 tf.flags.DEFINE_string('logdir', '/tmp/audio_demo',
                       'Directory into which to write TensorBoard data.')

-tf.flags.DEFINE_integer('steps', 500,
+tf.flags.DEFINE_integer('steps', 50,


Sounds worth it to me. Maybe note in PR description.

wchargin · 2017-08-11T23:10:24Z

Looking forward to all the summaries using SummaryMetadata! Almost there.

I've been excited to save the easiest for last—one scalar.summary module coming up. :-)

Summary: This commit adds support for new-style audio summaries to the plugin frontend and backend. Here is a screenshot of the resulting dashboard: ![Screenshot](https://user-images.githubusercontent.com/4317806/29231118-d1ce0a44-7e9a-11e7-8029-0ffff6ab44de.png) This change is fully compatible with the old data format. While I was editing the demo, I changed its step count from 500 to 50. Because we encode into WAV (ugh), the old demo was generating 617.8 MB of data. It now generates “only” 79.6 MB. (This change has nothing to do with the actual new summaries.) Test Plan: Run `bazel run //tensorboard/plugins/audio:audio_demo` to get data using new-style audio summaries. Also grab some old-style audio summaries, perhaps by running the version of that target that appeared before this commit (and changing the logdir to avoid writing into the same directory). Launch a TensorBoard with a logdir enclosing all of this data, and verify that it all renders correctly. When running `bazel test //tensorboard/...`, be aware that this diff contains a particularly large number of uses of `tf.compat.as_bytes` and friends. Thus, it is wise to run this test command in both py2 and py3 virtualenvs. wchargin-branch: new-style-audio-frontend

Summary: This was a relic of a piecewise migration from the `event_accumulator`’s old form to its current `plugin_event_accumulator` form. (See #345 for an example of one of the intermediate states.) Now totally superfluous, it incurs needless cognitive and CPU overhead, as the `getattr` blocks standard optimizations. Test Plan: All tests pass, and TensorBoard can still load the standard demo data. wchargin-branch: eliminate-metaprogramming

wchargin requested a review from chihuahua August 11, 2017 21:02

wchargin commented Aug 11, 2017

View reviewed changes

wchargin force-pushed the wchargin-new-style-audio-frontend branch from 2c31866 to 7222873 Compare August 11, 2017 22:41

wchargin force-pushed the wchargin-tensor-audio-summary branch from 5dcdd53 to b9f711c Compare August 11, 2017 22:45

wchargin force-pushed the wchargin-new-style-audio-frontend branch from 7222873 to 8ef46dc Compare August 11, 2017 22:45

chihuahua approved these changes Aug 11, 2017

View reviewed changes

wchargin force-pushed the wchargin-new-style-audio-frontend branch from 8ef46dc to 225393f Compare August 12, 2017 01:53

wchargin mentioned this pull request Aug 12, 2017

What UI would you like to see for image/audio summaries? #296

Closed

wchargin force-pushed the wchargin-tensor-audio-summary branch from b9f711c to 8e833ab Compare August 14, 2017 22:11

wchargin force-pushed the wchargin-new-style-audio-frontend branch from 225393f to 5aee0dc Compare August 14, 2017 23:59

wchargin changed the base branch from wchargin-tensor-audio-summary to master August 15, 2017 18:42

wchargin force-pushed the wchargin-new-style-audio-frontend branch from 5aee0dc to 8ffd715 Compare August 15, 2017 18:43

wchargin merged commit 8e5c969 into master Aug 15, 2017

wchargin deleted the wchargin-new-style-audio-frontend branch August 15, 2017 20:07

wchargin mentioned this pull request Jul 30, 2019

backend: eliminate unnecessary metaprogramming #2478

Merged

wchargin mentioned this pull request Aug 19, 2019

Remove unused vestigial plugin EventMultiplexer APIs #2577

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support new-style audio summaries #345

Support new-style audio summaries #345

wchargin commented Aug 11, 2017

wchargin Aug 11, 2017 •

edited

Loading

chihuahua Aug 11, 2017

wchargin Aug 11, 2017

chihuahua left a comment

chihuahua Aug 11, 2017

wchargin Aug 11, 2017

chihuahua Aug 11, 2017

wchargin Aug 11, 2017

chihuahua Aug 11, 2017

wchargin Aug 12, 2017

wchargin Aug 12, 2017

chihuahua Aug 12, 2017

chihuahua Aug 11, 2017

wchargin commented Aug 11, 2017

Support new-style audio summaries #345

Support new-style audio summaries #345

Conversation

wchargin commented Aug 11, 2017

wchargin Aug 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chihuahua left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchargin commented Aug 11, 2017

wchargin Aug 11, 2017 •

edited

Loading