Add the frontend for the PR curves plugin #426

chihuahua · 2017-08-26T08:14:35Z

This frontend for the PR curves plugin makes use of the vz-line-chart component that had been generalized in #388. Each tf-pr-curve-card contains 1 PR curve. The tf-pr-curve-steps-selector creates 1 slider per run and lets the user select steps.

This PR is a part of dividing #334 (which contains resourceful input from @wchargin) into smaller PRs.

@wchargin

This frontend for the PR curves plugin makes use of the `vz-line-chart` component that had been generalized in #388. Each `tf-pr-curve-card` contains 1 PR curve. The `tf-pr-curve-steps-selector` creates 1 slider per run and lets the user select steps. This PR is a part of dividing #334 (which contains resourceful input from @wchargin) into smaller PRs.

chihuahua · 2017-08-26T09:25:35Z

The UI WAI. With many runs, changing step is still a bit slow, but acceptable, which is in contrast to the UI being unusable when we redraw PR curves for all runs when the user changes step.

chihuahua · 2017-08-26T09:39:24Z

A good next step would be to visualize the display name in the header.

wchargin

High-level design looks good.

wchargin · 2017-08-27T14:44:01Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-steps-selector.html

+      } else if (timeDisplayType === 'wall_time') {
+        return new Date(value * 1000).toString();
+      }
+      console.error(


Can we just throw an error here? Returning undefined doesn't seem like the right thing to do.

wchargin · 2017-08-27T14:50:16Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-card.html

+        let url = router.pluginRoute('pr_curves', '/pr_curves');
+        url += url.indexOf('?') > -1 ? '&' : '?';
+        const urlRunsPortion = this.runs.map(r => `&run=${r}`).join('');
+        url += `tag=${this.tag}${urlRunsPortion}`;


The ordering of these two lines is confusing to me. What do you think of

let url = router.pluginRoute('pr_curves', '/pr_curves'); url += url.indexOf('?') > -1 ? '&' : '?'; url += `tag=${this.tag}`; url += this.runs.map(r => `&run=${r}`).join('');

?

+1, that is clearer.

wchargin · 2017-08-27T14:54:38Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-card.html

+      _computeSetOfRelevantRuns(runsWithStepAvailable) {
+        const setOfRelevantRuns = {};
+        _.forEach(runsWithStepAvailable, run => {
+          setOfRelevantRuns[run] = 1;


nit: Common practice is to use true here.

wchargin · 2017-08-27T14:55:53Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-card.html

+        if (!entry) {
+          return null;
+        }
+        return new Date(runToPrCurveEntry[run].wall_time * 1000).toString();


Might as well use entry.wall_time * 1000 here?

wchargin · 2017-08-27T14:56:37Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-dashboard.html

+        max-width: 540px;
+        margin: 80px auto 0 auto;
+      }
+      /** Not not let the steps selector occlude the run selector. */


wchargin · 2017-08-27T15:02:42Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-steps-selector.html

+      // do not make this handler run when _runToStepIndex is updated because
+      // asynchronously changing _runToStepIndex could create a rendering loop
+      // and jittery sliders.
+      setTimeout(() => {


Can this be replaced with this.async(() => { … })?

wchargin · 2017-08-27T15:09:31Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-steps-selector.html

+    },
+    _runToAvailableTimeEntriesChanged(runToAvailableTimeEntries) {
+      // The step for each run just refreshed. Change every slider to point to
+      // the last available step.


Wait, why? This behavior seems really strange. Suppose that an experiment is actively writing data to our logdir. Then, every thirty seconds, all the sliders will be shoved over to the end.

Would it suffice to make only newly added runs point to the last available step?

+1, that previous behavior seems jarring, ie after a reload, the steps unexpectedly change. Now, we only set sliders for newly added runs to the last available step - we initialize sliders to point to the last step, but never override the step again.

One small thing to note is that sampling logic samples a constant N values, so the step will slightly differ across reloads, but I think that's much less jarring than overriding step to the last value.

wchargin · 2017-08-27T15:11:49Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-steps-selector.html

+    },
+    _updateSliders(runToStepIndex) {
+      const sliders = this.querySelectorAll("paper-slider");
+      if (sliders) {


sliders should always be a NodeList, so will always be truthy:

var x = document.querySelectorAll('.not-very-likely'); [!!x, x.length] // [true, 0]

You can dissolve this if-statement, and just use:

this.querySelectorAll('paper-slider').forEach(slider => { slider[value] = runToStepIndex[slider.dataset.run]; });

wchargin · 2017-08-27T15:13:08Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-steps-selector.html

+      this._updateSliders(newRunToStepIndex);
+    },
+    _updateSliders(runToStepIndex) {
+      const sliders = this.querySelectorAll("paper-slider");


nit: Use single-quotes consistently?

wchargin · 2017-08-27T15:14:21Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-steps-selector.html

+      _.forOwn(runToStepIndex, (index, run) => {
+        const entries = runToAvailableTimeEntries[run];
+        if (!entries) {
+          return


Missing semicolon.

I'm surprised that this doesn't raise a warning with the closure compiler?

Done. Hmm, indeed, I see no Closure warnings. Maybe the transpilation from TS to uncompiled JS adds the semicolon back.

But this isn't TypeScript?

I just double-checked that no warning appears with no semicolon. Indeed, a warning should appear since suspiciousCode warnings are on by default: https://github.com/google/closure-compiler/wiki/Warnings

Maybe the TS is transpiled into JS before being compiled into a small JS binary, and Closure applies in the 2nd step?

chihuahua

The dashboard WAI. Sliders no longer jump to the last step after each reload.

chihuahua · 2017-08-27T20:00:22Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-card.html

+        let url = router.pluginRoute('pr_curves', '/pr_curves');
+        url += url.indexOf('?') > -1 ? '&' : '?';
+        const urlRunsPortion = this.runs.map(r => `&run=${r}`).join('');
+        url += `tag=${this.tag}${urlRunsPortion}`;


+1, that is clearer.

chihuahua · 2017-08-27T20:00:43Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-card.html

+      _computeSetOfRelevantRuns(runsWithStepAvailable) {
+        const setOfRelevantRuns = {};
+        _.forEach(runsWithStepAvailable, run => {
+          setOfRelevantRuns[run] = 1;


chihuahua · 2017-08-27T20:01:05Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-card.html

+        if (!entry) {
+          return null;
+        }
+        return new Date(runToPrCurveEntry[run].wall_time * 1000).toString();


chihuahua · 2017-08-27T20:02:03Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-dashboard.html

+        max-width: 540px;
+        margin: 80px auto 0 auto;
+      }
+      /** Not not let the steps selector occlude the run selector. */


chihuahua · 2017-08-27T20:02:43Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-dashboard.html

+      // The actual step value that each run should use. If a run + tag lacks a
+      // PR curve at this exact step value, the greatest step value less than
+      // this value will be used.
+      _runToStep: {


chihuahua · 2017-08-27T20:22:07Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-steps-selector.html

+      } else if (timeDisplayType === 'wall_time') {
+        return new Date(value * 1000).toString();
+      }
+      console.error(


chihuahua · 2017-08-27T20:49:43Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-steps-selector.html

+    },
+    _runToAvailableTimeEntriesChanged(runToAvailableTimeEntries) {
+      // The step for each run just refreshed. Change every slider to point to
+      // the last available step.


+1, that previous behavior seems jarring, ie after a reload, the steps unexpectedly change. Now, we only set sliders for newly added runs to the last available step - we initialize sliders to point to the last step, but never override the step again.

One small thing to note is that sampling logic samples a constant N values, so the step will slightly differ across reloads, but I think that's much less jarring than overriding step to the last value.

chihuahua · 2017-08-27T20:50:19Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-steps-selector.html

+      this._updateSliders(newRunToStepIndex);
+    },
+    _updateSliders(runToStepIndex) {
+      const sliders = this.querySelectorAll("paper-slider");


chihuahua · 2017-08-27T20:50:36Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-steps-selector.html

+    },
+    _updateSliders(runToStepIndex) {
+      const sliders = this.querySelectorAll("paper-slider");
+      if (sliders) {


chihuahua · 2017-08-27T20:52:00Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-steps-selector.html

+      _.forOwn(runToStepIndex, (index, run) => {
+        const entries = runToAvailableTimeEntries[run];
+        if (!entries) {
+          return


Done. Hmm, indeed, I see no Closure warnings. Maybe the transpilation from TS to uncompiled JS adds the semicolon back.

I forgot that we have to initialize in the first place.

wchargin

Thanks again for providing screenshots.

I note that the artifacts at the vertical axis—where the curve jumps down from 1.0 to 0.0—still exist. If I understand the previous discussion correctly, this is a problem with the summary op, not the frontend. Is this correct? (Also, is there a plan to fix it?)

Non-actionable: I wonder whether all the “[run] is at step [step]” will take up prohibitively much vertical space when many runs are used in practice. I suspect that we’ll hear feedback if that’s the case.

wchargin · 2017-08-29T20:58:40Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-card.html

+        // plot. The entry is based on which step the user currently selects.
+        _runToPrCurveEntry: {
+          type: Object,
+          value: {},


value: () => ({}) (likewise on _previousRunToPrCurveEntry)

Thanks! Done.

wchargin · 2017-08-29T20:59:48Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-card.html

+                (x) => isNaN(x) ? 'NaN' : valueFormatter(x);
+            return [
+              {
+                title: 'Name',


Is it better to call this "Name" or "Run"? (This isn't really a request to change; I have no preference.)

+1, I like "Run" much better. That parallels what most TF users would use. Done.

wchargin · 2017-08-29T21:01:49Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-card.html

+          this._updateSeriesDataForRun(run, entry);
+        });
+      },
+      _updateSeriesDataForRun(run, entryFor1Step) {


s/1/One, please. But perhaps prCurveEntry is clearer anyway—your choice.

Hmm. Yeah, maybe entryForOneStep for now.

jart

Took a high level pass. Nothing stands out to me as problematic. I support merging this as soon as @wchargin gives his nod. It will be nice to be able to include this plugin in the talk I'm giving on Thursday.

jart · 2017-08-30T01:22:30Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-card.html

@@ -0,0 +1,411 @@
+<!--
+@license


Please format these license headers the same way as the others, with the blank lines and all.

jart · 2017-08-30T01:25:49Z

tensorboard/components/tf_tensorboard/default-plugins.html

    'histograms': 'tf-histogram-dashboard',
    'projector': 'vz-projector-dashboard',
    'text': 'tf-text-dashboard',
+    'pr_curves': 'tf-pr-curve-dashboard',


It might look better, æsthetically speaking on the tab label, if this was "pr-curves".

Oh man ... I think this is linked with the plugin name in the backend, which is pr_curves. What do you think about in a separate change, we replace _ with a space in the navigation bar?

Yep: right now we unfortunately have the restriction that the tab in the plugin name must equal the plugin name on the backend. That's probably not good to have long-term. If you look at my proposed dashboards.ts from #202, you can see the approach I had in mind for resolving this. By giving each plugin a little configuration object, we can easily add an optional (or required) tabName field that takes the string "PR Curves" instead of "pr_curves". I believe at the time we chose not to do this just for simplicity and ease of merging (as #205 was created around the same time and it wasn't worth reconciling), but we might want to do something like that now.

I like @wchargin's idea of adding a tabName config property.

jart · 2017-08-30T01:26:04Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-card.html

+        align-items: center;
+        bottom: 0;
+        display: flex;
+        display: flex;


Repeated line here.

chihuahua

Hmm, the drop is caused by a tiny precision value at a tiny (but non-zero) recall value.

That is a good point. Those runs listed under each PR curve sure add up, and there are definitely people out there with many runs. I wonder if just using the color paired with step value suffices.

chihuahua · 2017-08-30T01:16:25Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-card.html

+        // plot. The entry is based on which step the user currently selects.
+        _runToPrCurveEntry: {
+          type: Object,
+          value: {},


Thanks! Done.

chihuahua · 2017-08-30T01:17:24Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-card.html

+                (x) => isNaN(x) ? 'NaN' : valueFormatter(x);
+            return [
+              {
+                title: 'Name',


+1, I like "Run" much better. That parallels what most TF users would use. Done.

chihuahua · 2017-08-30T01:18:31Z

tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-card.html

+          this._updateSeriesDataForRun(run, entry);
+        });
+      },
+      _updateSeriesDataForRun(run, entryFor1Step) {


Hmm. Yeah, maybe entryForOneStep for now.

chihuahua · 2017-08-30T01:35:52Z

For the strange, steep drop, I think we can consider solving the problem by removing the first point in the backend route, maybe here, we remove the point at recall = 0.
https://github.com/tensorflow/tensorboard/blob/master/tensorboard/plugins/pr_curve/pr_curves_plugin.py#L92

I want to also see if this could be solved at the summary op level though.

wchargin

Okay—LGTM! 🎉

I want to also see if this could be solved at the summary op level though.

Great. I certainly prefer this over removing the point at recall=0.

wchargin · 2017-08-30T03:19:02Z

tensorboard/components/tf_tensorboard/default-plugins.html

    'histograms': 'tf-histogram-dashboard',
    'projector': 'vz-projector-dashboard',
    'text': 'tf-text-dashboard',
+    'pr_curves': 'tf-pr-curve-dashboard',


Yep: right now we unfortunately have the restriction that the tab in the plugin name must equal the plugin name on the backend. That's probably not good to have long-term. If you look at my proposed dashboards.ts from #202, you can see the approach I had in mind for resolving this. By giving each plugin a little configuration object, we can easily add an optional (or required) tabName field that takes the string "PR Curves" instead of "pr_curves". I believe at the time we chose not to do this just for simplicity and ease of merging (as #205 was created around the same time and it wasn't worth reconciling), but we might want to do something like that now.

chihuahua force-pushed the chihuahua-pr-curves-frontend branch 5 times, most recently from fab71f6 to 6316efd Compare August 26, 2017 08:46

chihuahua requested review from jart and wchargin August 26, 2017 08:46

chihuahua added the type:feature (new plugin) label Aug 26, 2017

chihuahua force-pushed the chihuahua-pr-curves-frontend branch from 6316efd to c922b34 Compare August 26, 2017 08:50

chihuahua force-pushed the chihuahua-pr-curves-frontend branch from c922b34 to 9ae592f Compare August 26, 2017 09:10

Remove un-used styles

7dd8afe

chihuahua force-pushed the chihuahua-pr-curves-frontend branch from f231fa3 to 7dd8afe Compare August 27, 2017 10:42

wchargin reviewed Aug 27, 2017

View reviewed changes

Respond to comments

753c54f

chihuahua commented Aug 27, 2017

View reviewed changes

chihuahua added 4 commits August 27, 2017 14:01

Include all runs in the new runToStepIndex mapping

a33d7fd

Use let not const

fb81367

Remove the ready method

8c2cbca

Remove guard for whether a run had been added

5093caf

I forgot that we have to initialize in the first place.

wchargin reviewed Aug 29, 2017

View reviewed changes

Respond to comments

9f84567

jart approved these changes Aug 30, 2017

View reviewed changes

chihuahua commented Aug 30, 2017

View reviewed changes

Respond to comments

a58ea26

wchargin approved these changes Aug 30, 2017

View reviewed changes

chihuahua merged commit 0c23b61 into master Aug 30, 2017

chihuahua deleted the chihuahua-pr-curves-frontend branch August 30, 2017 05:43

chihuahua added the plugin:pr-curves label Sep 7, 2017

		@@ -0,0 +1,411 @@
		<!--
		@license

Add the frontend for the PR curves plugin #426

Add the frontend for the PR curves plugin #426

Uh oh!

Conversation

chihuahua commented Aug 26, 2017

Uh oh!

chihuahua commented Aug 26, 2017

Uh oh!

chihuahua commented Aug 26, 2017

Uh oh!

wchargin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chihuahua left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!