Skip to content

Commit

Permalink
Partially revert "improve rerender of vz_line_chart (#3524)" (#3552)
Browse files Browse the repository at this point in the history
Summary:
This reverts commit 27d0023, then
reinstates stubs for the new method definitions due to Google-internal
code that expects them to exist.

Stopgap for #3551 pending real fix.

Test Plan:
Opening the PR curves dashboard and sliding one of the step sliders now
correctly renders the PR curve instead of showing just the final datum.

wchargin-branch: revert-3524
  • Loading branch information
wchargin authored Apr 27, 2020
1 parent b929fae commit 46b6e61
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 53 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -219,8 +219,11 @@
this.$.chart.setSeriesMetadata(name, metadata);
},

/**
* Not yet implemented.
*/
commitChanges() {
this.$.chart.commitChanges();
// Temporarily rolled back due to PR curves breakage.
},

redraw() {
Expand Down
81 changes: 33 additions & 48 deletions tensorboard/components/vz_line_chart2/line-chart.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ 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;
Expand Down Expand Up @@ -138,6 +139,10 @@ 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;
Expand Down Expand Up @@ -317,6 +322,16 @@ 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;
Expand Down Expand Up @@ -818,53 +833,32 @@ namespace vz_line_chart2 {
}

/**
* 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.
* Update the selected series on the chart.
*/
public setVisibleSeries(names: string[]) {
this.disableChanges();
names = names.sort();
names.reverse(); // draw first series on top
this.seriesNames = names;
}

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();

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);

if (this.smoothingEnabled) {
this.smoothLinePlot.datasets(this.datasets);
}
if (this.marginAreaPlot) {
this.marginAreaPlot.datasets(this.datasets);
}
this.updateSpecialDatasets();
}

this.measureBBoxAndMaybeInvalidateLayoutInRaf();
this.dirtyDatasets.clear();
/**
* Not yet implemented.
*/
public commitChanges() {
// Temporarily rolled back due to PR curves breakage.
}

/**
Expand All @@ -891,30 +885,21 @@ namespace vz_line_chart2 {
}

/**
* 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.
* Sets the data of a series on the chart.
*/
public setSeriesData(name: string, data: vz_chart_helpers.ScalarDatum[]) {
this.disableChanges();
this.getDataset(name).data(data);
this.dirtyDatasets.add(name);
this.measureBBoxAndMaybeInvalidateLayoutInRaf();
}

/**
* 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.
* Sets the metadata of a series on the chart.
*/
public setSeriesMetadata(name: string, meta: any) {
this.disableChanges();
this.getDataset(name).metadata({
...this.getDataset(name).metadata(),
const newMeta = Object.assign({}, this.getDataset(name).metadata(), {
meta,
});
this.dirtyDatasets.add(name);
this.getDataset(name).metadata(newMeta);
}

public smoothingUpdate(weight: number) {
Expand Down
7 changes: 4 additions & 3 deletions tensorboard/components/vz_line_chart2/vz-line-chart2.ts
Original file line number Diff line number Diff line change
Expand Up @@ -338,9 +338,11 @@ namespace vz_line_chart2 {
}
},

/**
* Not yet implemented.
*/
commitChanges() {
if (!this._chart) return;
this._chart.commitChanges();
// Temporarily rolled back due to PR curves breakage.
},

/**
Expand Down Expand Up @@ -431,7 +433,6 @@ namespace vz_line_chart2 {
this._chart.setSeriesMetadata(name, this._seriesMetadataCache[name]);
});
this._chart.setVisibleSeries(this._visibleSeriesCache);
this._chart.commitChanges();
},

_smoothingChanged: function() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,6 @@
const name = this._getSeriesNameFromDatum(datum);
scalarChart.setSeriesMetadata(name, datum);
scalarChart.setSeriesData(name, formattedData);
scalarChart.commitChanges();
};
},
readOnly: true,
Expand Down

0 comments on commit 46b6e61

Please sign in to comment.