line chart: draw NaN with triangle #4461

stephanwlee · 2020-12-11T23:48:37Z

This change adds the feature of vz-line-chart of rendering null values
as triangles.

Business logic:

for non-first NaNs: carry the y value from the last non-NaN value
for starting NaNs with non-NaN values in the series: assign y values
from the first non-NaN value.
for all NaNs: assign 0 to y.

tensorboard/webapp/widgets/line_chart_v2/lib/series_line_view.ts

psybuzz · 2020-12-12T01:42:57Z

tensorboard/webapp/widgets/line_chart_v2/lib/series_line_view.ts

+      ] of partitionedPolyline.entries()) {
+        if (type === PartitionType.NUMBER) {
+          if (polyline.length === 2) {
+            if (metadata.aux) continue;


iiuc, this is saying we want to skip drawing the circle because:

An aux series cannot be drawn alone without a corresponding non-aux series in the same view

A single-point aux and non-aux series will want a circle at the exact same position

If these are the only reasons why we can skip, I'm not sure we should do so in SeriesLineView. These 2 assumptions are really true only in this scalar card scenario where "aux" represents smoothing. To keep SeriesLineView agnostic, should we drop this check?

I agree that it has leaky abstraction and ideally, we should make NaN drawing a feature specified in the metadata but I do not yet see a strong reason to do so.

Few things though (since there seems to be some confusion).

aux is the original series (shown in faded color) when smoothing is enabled

vz_line_chart did not show NaNs for aux/original-series and I am merely trying to have the same UX.

vz_line_chart

Gpu line chart

Again, we are kind of are abusing the aux flag but I think there is a balance between flags for every feature vs. simplicity. I expect the API to naturally evolve as we adopt the line chart in more use cases.

Hm, I think I see where you are coming from. By making SeriesLineView aware of the fact that aux lines never draw NaNs, we gain a good amount of simplicity. I think this is reasonable, and keeping L135-136 is fine with me.

However, I don't agree in the number case (non-NaN). Shouldn't all (non-NaN) lines be drawn the same way, whether they are aux or not? If we treat aux+nonNaN the same as nonAux+nonNan, I think we satisfy those goals:

aux is the original series (shown in faded color) when smoothing is enabled
vz_line_chart did not show NaNs for aux/original-series and I am merely trying to have the same UX.

Concretely, I think dropping L111 changes our cases from this:
If line segment is [number && singlePoint && aux] skip
If line segment is [number && non-aux] draw
If line segment is [NaN && aux] skip
If line segment is [NaN && non-aux] draw

Into this:
If line segment is [number] draw
If line segment is [NaN && aux] skip
If line segment is [NaN && non-aux] draw

Is there something we gain by keeping L111, besides rendering fewer points in the single point scenario, that I might be missing?

Ah, got it. I simply decided not to draw the circle for aux because there was no circle for single point in vz_line_chart at all!

I took your advise and remove L111.

psybuzz · 2020-12-12T01:49:27Z

tensorboard/webapp/widgets/line_chart_v2/lib/series_line_view.ts

+  ): Array<{polyline: Float32Array; type: PartitionType}> {
+    if (polyline.length % 2 !== 0) {
+      throw new Error(`Cannot have odd length-ed polyline: ${polyline.length}`);
+    }


I would slightly prefer moving this check closer to the place where the polyline is constructed, or perhaps even better to remove it entirely. Polylines type has a nice comment "Flattened array of 2d coordinates...", and it would be a bit cumbersome to have to require all consumers to check the even-ness of it, imo.

Another idea is to rename Polyline to Polyline2D or something that readers and users of it would instantly know that it's safe to assume that it is even.

Leaning not making the change. We have plenty of those invariant checks but as a dev who jumps around a lot, I think it makes sense to have the same invariant in multiple places if it is not type checkable since it makes reading the code much easier.

In that case, can we move this check to the calling function redraw() ~L103? Its function body relies on the invariant (in the loop around L137), so having the check there helps me realize that everything in redraw() operates with the validated assumption.

tensorboard/webapp/widgets/line_chart_v2/lib/series_line_view.ts

This change adds the feature of vz-line-chart of rendering null values as triangles. Business logic: - for non-first NaNs: carry the y value from the last non-NaN value - for starting NaNs with non-NaN values in the series: assign y values from the first non-NaN value. - for all NaNs: assign 0 to y.

psybuzz

Overall looks good

psybuzz · 2020-12-14T22:06:45Z

tensorboard/webapp/widgets/line_chart_v2/lib/series_line_view.ts

+  ): Array<{polyline: Float32Array; type: PartitionType}> {
+    if (polyline.length % 2 !== 0) {
+      throw new Error(`Cannot have odd length-ed polyline: ${polyline.length}`);
+    }


In that case, can we move this check to the calling function redraw() ~L103? Its function body relies on the invariant (in the loop around L137), so having the check there helps me realize that everything in redraw() operates with the validated assumption.

psybuzz · 2020-12-14T22:27:53Z

tensorboard/webapp/widgets/line_chart_v2/lib/series_line_view.ts

+      ] of partitionedPolyline.entries()) {
+        if (type === PartitionType.NUMBER) {
+          if (polyline.length === 2) {
+            if (metadata.aux) continue;


Hm, I think I see where you are coming from. By making SeriesLineView aware of the fact that aux lines never draw NaNs, we gain a good amount of simplicity. I think this is reasonable, and keeping L135-136 is fine with me.

However, I don't agree in the number case (non-NaN). Shouldn't all (non-NaN) lines be drawn the same way, whether they are aux or not? If we treat aux+nonNaN the same as nonAux+nonNan, I think we satisfy those goals:

aux is the original series (shown in faded color) when smoothing is enabled
vz_line_chart did not show NaNs for aux/original-series and I am merely trying to have the same UX.

Concretely, I think dropping L111 changes our cases from this:
If line segment is [number && singlePoint && aux] skip
If line segment is [number && non-aux] draw
If line segment is [NaN && aux] skip
If line segment is [NaN && non-aux] draw

Into this:
If line segment is [number] draw
If line segment is [NaN && aux] skip
If line segment is [NaN && non-aux] draw

Is there something we gain by keeping L111, besides rendering fewer points in the single point scenario, that I might be missing?

psybuzz

Thanks for the changes, lgtm!

google-cla bot added the cla: yes label Dec 11, 2020

stephanwlee force-pushed the lc_draw_null branch from ba4e254 to b89ff3c Compare December 11, 2020 23:49

stephanwlee requested a review from psybuzz December 11, 2020 23:49

psybuzz reviewed Dec 12, 2020

View reviewed changes

stephanwlee added 2 commits December 14, 2020 13:00

cr address

4e84667

stephanwlee force-pushed the lc_draw_null branch from b89ff3c to 4e84667 Compare December 14, 2020 21:13

stephanwlee requested a review from psybuzz December 14, 2020 21:20

psybuzz reviewed Dec 14, 2020

View reviewed changes

cr more

8055216

stephanwlee requested a review from psybuzz December 15, 2020 00:28

psybuzz approved these changes Dec 15, 2020

View reviewed changes

stephanwlee merged commit aa8dbfd into tensorflow:master Dec 15, 2020

stephanwlee deleted the lc_draw_null branch December 15, 2020 05:11

ronigober mentioned this pull request Jun 14, 2022

draw NaN with triangle lanpa/tensorboardX#668

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

line chart: draw NaN with triangle #4461

line chart: draw NaN with triangle #4461

stephanwlee commented Dec 11, 2020

psybuzz Dec 12, 2020

stephanwlee Dec 14, 2020

psybuzz Dec 14, 2020

stephanwlee Dec 15, 2020

psybuzz Dec 12, 2020

stephanwlee Dec 14, 2020

psybuzz Dec 14, 2020

psybuzz left a comment

psybuzz Dec 14, 2020

psybuzz Dec 14, 2020

psybuzz left a comment

line chart: draw NaN with triangle #4461

line chart: draw NaN with triangle #4461

Conversation

stephanwlee commented Dec 11, 2020

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

psybuzz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

psybuzz left a comment

Choose a reason for hiding this comment