Implement commitChanges on vz-line-chart2 (#3571)

This is a roll forward of 46b6e61. The commit partiall rolls back 27d0023 which introduces `commitChanges` which was supposed to be invoked after setting data, visible series, or metadata. This change addresses the regression caused by 27d0023 which forgot to call the new API in all places where we use the vz-line-chart2.
tensorflow · May 7, 2020 · 8ddcceb · 8ddcceb
1 parent 3777aec
commit 8ddcceb
Show file tree

Hide file tree

Showing 9 changed files with 76 additions and 43 deletions.
diff --git a/tensorboard/components/tf_line_chart_data_loader/tf-line-chart-data-loader.html b/tensorboard/components/tf_line_chart_data_loader/tf-line-chart-data-loader.html
@@ -219,11 +219,8 @@
           this.$.chart.setSeriesMetadata(name, metadata);
         },
 
-        /**
-         * Not yet implemented.
-         */
         commitChanges() {
-          // Temporarily rolled back due to PR curves breakage.
+          this.$.chart.commitChanges();
         },
 
         redraw() {

diff --git a/tensorboard/components/vz_line_chart2/line-chart.ts b/tensorboard/components/vz_line_chart2/line-chart.ts
@@ -90,7 +90,6 @@ namespace vz_line_chart2 {
     private lastPointsDataset: Plottable.Dataset;
     private fillArea?: FillArea;
     private datasets: Plottable.Dataset[];
-    private onDatasetChanged: (dataset: Plottable.Dataset) => void;
     private nanDataset: Plottable.Dataset;
     private smoothingWeight: number;
     private smoothingEnabled: boolean;
@@ -139,10 +138,6 @@ namespace vz_line_chart2 {
       // varies based on whether smoothing is enabled.
       this.symbolFunction = symbolFunction;
 
-      // need to do a single bind, so we can deregister the callback from
-      // old Plottable.Datasets. (Deregistration is done by identity checks.)
-      this.onDatasetChanged = this._onDatasetChanged.bind(this);
-
       this._defaultXRange = defaultXRange;
       this._defaultYRange = defaultYRange;
       this.tooltipColumns = tooltipColumns;
@@ -322,16 +317,6 @@ namespace vz_line_chart2 {
       return new Plottable.Components.Group(groups);
     }
 
-    /** Updates the chart when a dataset changes. Called every time the data of
-     * a dataset changes to update the charts.
-     */
-    private _onDatasetChanged(dataset: Plottable.Dataset) {
-      if (this.smoothingEnabled) {
-        this.resmoothDataset(dataset);
-      }
-      this.updateSpecialDatasets();
-    }
-
     public ignoreYOutliers(ignoreYOutliers: boolean) {
       if (ignoreYOutliers !== this._ignoreYOutliers) {
         this._ignoreYOutliers = ignoreYOutliers;
@@ -833,32 +818,53 @@ namespace vz_line_chart2 {
     }
 
     /**
-     * Update the selected series on the chart.
+     * Stages update of visible series on the chart.
+     *
+     * Please call `commitChanges` for the changes to be reflected on the chart
+     * after making all the changes.
      */
     public setVisibleSeries(names: string[]) {
+      this.disableChanges();
       names = names.sort();
+      names.reverse(); // draw first series on top
       this.seriesNames = names;
+    }
 
-      names.reverse(); // draw first series on top
-      this.datasets.forEach((d) => d.offUpdate(this.onDatasetChanged));
-      this.datasets = names.map((r) => this.getDataset(r));
-      this.datasets.forEach((d) => d.onUpdate(this.onDatasetChanged));
-      this.linePlot.datasets(this.datasets);
+    private dirtyDatasets = new Set<string>();
+
+    private disableChanges() {
+      if (!this.dirtyDatasets.size) {
+        // Prevent plots from reacting to the dataset changes.
+        this.linePlot.datasets([]);
+        if (this.smoothLinePlot) {
+          this.smoothLinePlot.datasets([]);
+        }
+
+        if (this.marginAreaPlot) {
+          this.marginAreaPlot.datasets([]);
+        }
+      }
+    }
+
+    public commitChanges() {
+      this.datasets = this.seriesNames.map((r) => this.getDataset(r));
+      [...this.dirtyDatasets].forEach((d) => {
+        if (this.smoothingEnabled) {
+          this.resmoothDataset(this.getDataset(d));
+        }
+      });
+      this.updateSpecialDatasets();
 
+      this.linePlot.datasets(this.datasets);
       if (this.smoothingEnabled) {
         this.smoothLinePlot.datasets(this.datasets);
       }
       if (this.marginAreaPlot) {
         this.marginAreaPlot.datasets(this.datasets);
       }
-      this.updateSpecialDatasets();
-    }
 
-    /**
-     * Not yet implemented.
-     */
-    public commitChanges() {
-      // Temporarily rolled back due to PR curves breakage.
+      this.measureBBoxAndMaybeInvalidateLayoutInRaf();
+      this.dirtyDatasets.clear();
     }
 
     /**
@@ -885,21 +891,30 @@ namespace vz_line_chart2 {
     }
 
     /**
-     * Sets the data of a series on the chart.
+     * Stages a data change of a series on the chart.
+     *
+     * Please call `commitChanges` for the changes to be reflected on the chart
+     * after making all the changes.
      */
     public setSeriesData(name: string, data: vz_chart_helpers.ScalarDatum[]) {
+      this.disableChanges();
       this.getDataset(name).data(data);
-      this.measureBBoxAndMaybeInvalidateLayoutInRaf();
+      this.dirtyDatasets.add(name);
     }
 
     /**
-     * Sets the metadata of a series on the chart.
+     * Sets a metadata change of a series on the chart.
+     *
+     * Please call `commitChanges` for the changes to be reflected on the chart
+     * after making all the changes.
      */
     public setSeriesMetadata(name: string, meta: any) {
-      const newMeta = Object.assign({}, this.getDataset(name).metadata(), {
+      this.disableChanges();
+      this.getDataset(name).metadata({
+        ...this.getDataset(name).metadata(),
         meta,
       });
-      this.getDataset(name).metadata(newMeta);
+      this.dirtyDatasets.add(name);
     }
 
     public smoothingUpdate(weight: number) {

diff --git a/tensorboard/components/vz_line_chart2/microbenchmark/renders_spec.ts b/tensorboard/components/vz_line_chart2/microbenchmark/renders_spec.ts
@@ -61,6 +61,7 @@ benchmark({
     context.container.appendChild(context.chart);
 
     context.chart.setVisibleSeries([]);
+    context.chart.commitChanges();
 
     await polymerFlush();
   },
@@ -82,6 +83,7 @@ benchmark({
 
     context.chart.setSeriesData('sine', DATA_POINTS.sine1k);
     context.chart.setVisibleSeries(['sine']);
+    context.chart.commitChanges();
 
     await polymerFlush();
   },
@@ -103,6 +105,7 @@ benchmark({
 
     context.chart.setSeriesData('sine', DATA_POINTS.sine1k);
     context.chart.setVisibleSeries(['sine']);
+    context.chart.commitChanges();
 
     await polymerFlush();
   },
@@ -124,6 +127,7 @@ benchmark({
 
     context.chart.setSeriesData('cosine', DATA_POINTS.cosine100k);
     context.chart.setVisibleSeries(['cosine']);
+    context.chart.commitChanges();
 
     await polymerFlush();
   },
@@ -146,6 +150,7 @@ benchmark({
     context.chart.setSeriesData('sine', DATA_POINTS.sine1k);
     context.chart.setSeriesData('cosine', DATA_POINTS.cosine1k);
     context.chart.setVisibleSeries(['cosine']);
+    context.chart.commitChanges();
     context.even = true;
 
     await polymerFlush();
@@ -177,6 +182,7 @@ benchmark({
     context.chart.setVisibleSeries(
       FIVE_HUNDRED_1K_DATA_POINTS.map(({name}) => name)
     );
+    context.chart.commitChanges();
 
     await polymerFlush();
   },
@@ -201,6 +207,7 @@ benchmark({
       context.chart.setSeriesData(name, data);
     });
     context.chart.setVisibleSeries(datapoints.map(({name}) => name));
+    context.chart.commitChanges();
 
     await polymerFlush();
     context.index = 0;
@@ -234,6 +241,7 @@ benchmark({
         context.container.appendChild(chart);
         chart.setSeriesData(name, data);
         chart.setVisibleSeries([name]);
+        chart.commitChanges();
         return chart;
       }
     );
@@ -264,6 +272,7 @@ benchmark({
         chart.style.height = '50px';
         context.container.appendChild(chart);
         chart.setSeriesData(name, data);
+        chart.commitChanges();
         return chart;
       }
     );
@@ -277,12 +286,14 @@ benchmark({
     context.charts.forEach((chart: any) => {
       chart.setVisibleSeries([]);
     });
+    context.chart.commitChanges();
 
     await context.flushAsync();
 
     context.charts.forEach((chart: any, index: number) => {
       chart.setVisibleSeries([context.names[index]]);
     });
+    context.chart.commitChanges();
 
     await context.flushAsync();
   },
@@ -300,6 +311,7 @@ benchmark({
 
     context.chart.setSeriesData('cosine', DATA_POINTS.cosine1k);
     context.chart.setVisibleSeries(['cosine']);
+    context.chart.commitChanges();
     context.chart.smoothingEnabled = true;
     context.even = true;
 
@@ -328,6 +340,7 @@ benchmark({
 
     context.chart.setSeriesData('cosine', DATA_POINTS.cosine100k);
     context.chart.setVisibleSeries(['cosine']);
+    context.chart.commitChanges();
     context.chart.smoothingEnabled = true;
     context.even = true;
 
@@ -358,6 +371,7 @@ benchmark({
 
     context.chart.setSeriesData('cosine', DATA_POINTS.cosine100k);
     context.chart.setVisibleSeries(['cosine']);
+    context.chart.commitChanges();
     context.chart.smoothingEnabled = true;
     context.even = true;
 

diff --git a/tensorboard/components/vz_line_chart2/vz-line-chart2.ts b/tensorboard/components/vz_line_chart2/vz-line-chart2.ts
@@ -338,11 +338,9 @@ namespace vz_line_chart2 {
       }
     },
 
-    /**
-     * Not yet implemented.
-     */
     commitChanges() {
-      // Temporarily rolled back due to PR curves breakage.
+      if (!this._chart) return;
+      this._chart.commitChanges();
     },
 
     /**
@@ -433,6 +431,7 @@ namespace vz_line_chart2 {
           this._chart.setSeriesMetadata(name, this._seriesMetadataCache[name]);
         });
       this._chart.setVisibleSeries(this._visibleSeriesCache);
+      this._chart.commitChanges();
     },
 
     _smoothingChanged: function() {

diff --git a/.../plugins/custom_scalar/tf_custom_scalar_dashboard/tf-custom-scalar-margin-chart-card.html b/.../plugins/custom_scalar/tf_custom_scalar_dashboard/tf-custom-scalar-margin-chart-card.html
@@ -664,6 +664,7 @@
             dataSeries.getData()
           );
         });
+        this.$.loader.commitChanges();
       },
       _computeSeriesNames() {
         const runLookup = new Set(this.runs);

diff --git a/...gins/custom_scalar/tf_custom_scalar_dashboard/tf-custom-scalar-multi-line-chart-card.html b/...gins/custom_scalar/tf_custom_scalar_dashboard/tf-custom-scalar-multi-line-chart-card.html
@@ -382,6 +382,7 @@
         Object.entries(_nameToDataSeries).forEach(([name, series]) => {
           this.$.loader.setSeriesData(name, series.getData());
         });
+        this.$.loader.commitChanges();
       },
       _computeSelectedRunsSet(runs) {
         const mapping = {};

diff --git a/tensorboard/plugins/debugger/tf_debugger_dashboard/tf-debugger-line-chart.html b/tensorboard/plugins/debugger/tf_debugger_dashboard/tf-debugger-line-chart.html
@@ -110,6 +110,7 @@
           seriesData.push({step: xData[i], scalar: yData[i]});
         }
         chart.setSeriesData(this._defaultSeriesName, seriesData);
+        chart.commitChanges();
       },
     });
   </script>

diff --git a/tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-card.html b/tensorboard/plugins/pr_curve/tf_pr_curve_dashboard/tf-pr-curve-card.html
@@ -407,11 +407,15 @@
         for (let i = 0; i < seriesData.length; i++) {
           seriesData[i] = _.mapValues(fieldsToData, (values) => values[i]);
         }
-        this.$$('tf-line-chart-data-loader').setSeriesData(run, seriesData);
+        const loader = this.$$('tf-line-chart-data-loader');
+        loader.setSeriesData(run, seriesData);
+        loader.commitChanges();
       },
       _clearSeriesData(run) {
         // Clears data for a run in the chart.
-        this.$$('tf-line-chart-data-loader').setSeriesData(run, []);
+        const loader = this.$$('tf-line-chart-data-loader');
+        loader.setSeriesData(run, []);
+        loader.commitChanges();
       },
       _updateRunToPrCurveEntry(runToDataOverTime, runToStepCap) {
         const runToEntry = {};

diff --git a/tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-card.html b/tensorboard/plugins/scalar/tf_scalar_dashboard/tf-scalar-card.html
@@ -226,6 +226,7 @@
               const name = this._getSeriesNameFromDatum(datum);
               scalarChart.setSeriesMetadata(name, datum);
               scalarChart.setSeriesData(name, formattedData);
+              scalarChart.commitChanges();
             };
           },
           readOnly: true,