Generalize tf-scalar-chart.html #470

chihuahua · 2017-09-04T01:03:44Z

Before this change, tf-scalar-chart.html contains much logic specific to
the scalars plugin. We extract that scalars-specific logic into a new
component called tf-scalar-card. Some of the logic extracted includes
properties that allow for setting X and Y axes, the buttons under the
chart, and the callback that is run when data is received from the
server.

This change is a step towards allowing any plugin to make use of the
tf-scalar-chart component (albeit a subsequent change will move and
rename this component to clarify that it is generally applicable). That
goal is desirable because tf-scalar-chart contains a lot of fixes that
correct for bugs in the vz-line-chart component and Plottable. Those fixes
include #163, #189, and #204.

Screenshot (The scalars dashboard WAI):

chihuahua · 2017-09-11T06:51:03Z

Ping. Could someone review? An internal team is considering a new plugin that uses line charts, and they're about to use vz-line-charts. We might be able to generalize this in time.

wchargin · 2017-09-11T12:27:22Z

tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-chart.html

-          </div>
-        </div>
-      </template>
+      <content select=".buttons"></content>


I'd recommend against having tf-scalar-chart own the DOM for the buttons: we had something like this before I removed all the chart scaffolding logic, and anything nontrivial required a super hacky implementation, was not accessible, etc.

Let's just have the parent element write

<tf-scalar-chart … /> <div>  </div>

If there's a benefit to doing it as currently written that I'm not seeing—maybe in terms of CSS/layout?—I'm sure that there is an easy way to redesign it for equivalent functionality.

Done. Indeed, I see no major benefits there - maybe we write the CSS for buttons once, but that's not too compelling.

wchargin · 2017-09-11T12:29:16Z

tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-chart.html

-        showDownloadLinks: Boolean,
+
+        /**
+         * A function that takes as inputs:


Cool, I like this API.

wchargin · 2017-09-11T12:30:33Z

tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-card.html

+  const X_TYPE = {
+    STEP: 'step',
+    RELATIVE: 'relative',
+    WALL: 'wall',


wall_time? (Not that the values of X_TYPE are ever used.)

wchargin · 2017-09-11T12:32:11Z

tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-card.html

+
+        _dataUrl: {
+          type: Function,
+          value: () => getRouter().pluginRunTagRoute('scalars', '/scalars'),


If you don't mind, I'd like to merge #494 and its dependency family before this; that family is currently halfway merged, blocking on review of #491, and it would be preferable to not introduce occurrences of old-style code right as we're trying to excise them.

Thanks to @dandelionmane for reviewing.

wchargin · 2017-09-11T12:44:13Z

tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-card.html

+  A card that contains an individual scalar chart.
+-->
+<dom-module id="tf-scalar-card">
+<template>


It seems to me that tf-scalar-card should own the tf-card-heading. That is, the tf-scalar-chart component should display just a chart and nothing more: the log scale toggle, download links, etc. should be handled by the enclosing component.

+1, done. It seems like cards could indeed differ in header logic (for instance, due to differences in merging descriptions).

teamdandelion · 2017-09-11T18:32:26Z

At a high level:
The intended factorization of components was:

tf-scalar-chart - specific to the scalar dashboard
vz-line-chart - good for generic usage

After the change, we will have:

tf-scalar-card - specific to the scalar dashbaord
tf-scalar-chart - good for generic usage
vz-line-chart - useless?

Should we just be moving all of the tf-scalar-chart behaviors that are needed for general usage into vz-line-chart?

Or is there a more subtle factorization of functionality here that I'm missing (e.g. vz-line-chart is useful for non-tf applications where we don't assume you have point/step/wall_time structuring? I don't think this is true because lots of the true application logic live in the Plottable code inside vz-line-chart)

teamdandelion · 2017-09-11T18:21:43Z

tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-card.html

+<link rel="import" href="tf-scalar-chart.html">
+
+<!--
+  A card that contains an individual scalar chart.


Could this be more descriptive? It should be clear to a reader of the docstring why this is different from tf-scalar-chart

How about now?

Before this change, tf-scalar-chart.html contains much logic specific to the scalars plugin. We extract that scalars-specific logic into a new component called tf-scalar-card. Some of the logic extracted includes properties that allow for setting X and Y axes, the buttons under the chart, and the callback that is run when data is received from the server. This change is a step towards allowing any plugin to make use of the tf-scalar-chart component (albeit a subsequent change will move and rename this component to clarify that it is generally applicable). That goal is desirable because tf-scalar-chart contains a lot of logic that corrects for bugs in the vz-line-chart component and Plottable. That logic includes #163, #189, and #204.

chihuahua

I verified that the scalars dashboard WAI. The buttons still work. The CSV and JSON links still work.

@dandelionmane, merging with vz-line-chart SG. How about we do that in 3 steps.

We extract scalars logic from tf-scalar-chart (this PR).
We move tf-scalar-chart into components/ and rename it to say tf-line-chart-loader (analogous to image-loader).
We move vz-line-chart into tf-scalar-chart and make the former only visible to the latter.

chihuahua · 2017-09-12T03:38:36Z

tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-card.html

+  A card that contains an individual scalar chart.
+-->
+<dom-module id="tf-scalar-card">
+<template>


+1, done. It seems like cards could indeed differ in header logic (for instance, due to differences in merging descriptions).

chihuahua · 2017-09-12T03:41:46Z

tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-card.html

+<link rel="import" href="tf-scalar-chart.html">
+
+<!--
+  A card that contains an individual scalar chart.


How about now?

chihuahua · 2017-09-12T03:53:26Z

tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-card.html

+  const X_TYPE = {
+    STEP: 'step',
+    RELATIVE: 'relative',
+    WALL: 'wall',


chihuahua · 2017-09-12T03:53:36Z

tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-card.html

+
+        _dataUrl: {
+          type: Function,
+          value: () => getRouter().pluginRunTagRoute('scalars', '/scalars'),


Thanks to @dandelionmane for reviewing.

chihuahua · 2017-09-12T03:54:23Z

tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-chart.html

-          </div>
-        </div>
-      </template>
+      <content select=".buttons"></content>


Done. Indeed, I see no major benefits there - maybe we write the CSS for buttons once, but that's not too compelling.

chihuahua · 2017-09-12T03:54:44Z

tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-chart.html

-        showDownloadLinks: Boolean,
+
+        /**
+         * A function that takes as inputs:


chihuahua · 2017-09-14T18:51:10Z

Are there any other actionable items?

jart

I'll second what @dandelionmane said earlier, about how it might be better if some of this was further generalized and documented, and moved into the vz_line_chart package. Since this appears to be a step in that direction, LGTM.

jart · 2017-09-14T21:15:59Z

tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-card.html

+  </style>
+</template>
+<script>
+  import {addParams} from '../tf-backend/urlPathHelpers.js';


Note: Sometime in Q4 we're most likely going to be refactoring the codebase to move these script tags out into separate .js or .ts files, so we can use rules_typescript. Doesn't need to happen now.

OK. @wchargin noted advantages to keeping scripts inline to make them easier to follow, but it would be nice to support the BUILD rules.

jart · 2017-09-14T21:19:04Z

tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-card.html

+-->
+<dom-module id="tf-scalar-card">
+<template>
+  <tf-card-heading


[optional] Consider putting a <figure> tag around all this stuff, since I programmed tf-card-heading earlier to use figcaption. I'm not sure what the benefit would be, but it is nice semantic HTML. Possibly also make the div buttons below a figcaption.

Ah, the hard part is that <figcaption> must be the first or last child within a <figure> element. How about we achieve that in a separate refactoring. The use of figure semantics seems promising though.

jart · 2017-09-14T21:31:03Z

tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-chart.html

-        showDownloadLinks: Boolean,
+
+        /**
+         * A function that takes as inputs:


The scalars dashboard had been broken by #470 because the buttons had not been moved out of the tf-scalar-chart container. Furthermore, the tf-card-heading now must be block-positioned for the margin to take effect.

wchargin · 2017-09-29T19:38:11Z

@dandelionmane:

After the change, we will have:

tf-scalar-card - specific to the scalar dashbaord

tf-scalar-chart - good for generic usage

vz-line-chart - useless?

Not sure that I agree with this. I'd write:

tf-scalar-card - specific to the scalar dashbaord
tf-scalar-chart (now "tf-line-chart-data-loader") - good for usage that loads data from the network using the standard TensorBoard run–tag organization
vz-line-chart - good if the above does not apply

At a higher level: vz-line-chart is a "dumb component" and tf-scalar-chart is a "smart component." Especially given that they're already separate, which is usually considered the best practice, I don't see a need to inline vz-line-chart into tf-scalar-chart.

What do you think?

chihuahua · 2017-09-29T20:12:40Z

SG. Lets just keep them separate and avoid the onus of refactoring then fixing what's broken.

chihuahua added the plugin:scalars label Sep 4, 2017

chihuahua requested review from jart and wchargin and removed request for jart and wchargin September 4, 2017 01:03

chihuahua requested review from jart and teamdandelion September 11, 2017 06:50

wchargin reviewed Sep 11, 2017

View reviewed changes

teamdandelion reviewed Sep 11, 2017

View reviewed changes

chihuahua force-pushed the chihuahua-scalars-refactoring branch from 76b8487 to 55ec296 Compare September 12, 2017 03:59

chihuahua commented Sep 12, 2017

View reviewed changes

chihuahua requested a review from wchargin September 12, 2017 04:04

jart approved these changes Sep 14, 2017

View reviewed changes

chihuahua merged commit c27023e into master Sep 15, 2017

chihuahua deleted the chihuahua-scalars-refactoring branch September 15, 2017 00:10

chihuahua mentioned this pull request Sep 15, 2017

Fix buttons for the scalars dashboard. #536

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize tf-scalar-chart.html #470

Generalize tf-scalar-chart.html #470

chihuahua commented Sep 4, 2017

chihuahua commented Sep 11, 2017

wchargin Sep 11, 2017

chihuahua Sep 12, 2017

wchargin Sep 11, 2017

chihuahua Sep 12, 2017

jart Sep 14, 2017

chihuahua Sep 14, 2017

wchargin Sep 11, 2017

chihuahua Sep 12, 2017

wchargin Sep 11, 2017

teamdandelion Sep 11, 2017

chihuahua Sep 12, 2017

wchargin Sep 11, 2017

chihuahua Sep 12, 2017

teamdandelion commented Sep 11, 2017

teamdandelion Sep 11, 2017

chihuahua Sep 12, 2017

chihuahua left a comment

chihuahua Sep 12, 2017

chihuahua Sep 12, 2017

chihuahua Sep 12, 2017

chihuahua Sep 12, 2017

chihuahua Sep 12, 2017

chihuahua Sep 12, 2017

chihuahua commented Sep 14, 2017

jart left a comment

jart Sep 14, 2017

chihuahua Sep 14, 2017

jart Sep 14, 2017

chihuahua Sep 14, 2017

jart Sep 14, 2017

wchargin commented Sep 29, 2017

chihuahua commented Sep 29, 2017

Generalize tf-scalar-chart.html #470

Generalize tf-scalar-chart.html #470

Conversation

chihuahua commented Sep 4, 2017

chihuahua commented Sep 11, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teamdandelion commented Sep 11, 2017

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

chihuahua commented Sep 14, 2017

jart 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

wchargin commented Sep 29, 2017

chihuahua commented Sep 29, 2017