[tensor-widget] Add support for hierarchical menu and image view of tensor values #2729

caisq · 2019-10-04T21:03:41Z

Motivation for features / changes
- Continue developing TensorWidget
Technical description of changes
- Add menu.ts: Include FlatMenu and Menu classes.
- Add a default menu item to TensorWdigetImpl, one that allows user to switch between text and image representations of numeric tensors.
- Add colormap.ts: Include the base class ColorMap and a basic implementation GrayscaleColorMap. The latter is used for image rendering of numeric and boolean tensor values.
- Add minimum and maximum values to BooleanOrNumericTensorNumericSummary
- Rename terminology: health pill --> numeric summary
- Update demo.ts.
Screenshots of UI changes

davidsoergel · 2019-10-07T15:35:53Z

tensorboard/components/tensor_widget/health-pill-types.ts

+  /** Maximum of all finite values. */
+  maximum: number | boolean;
+}
+
 // TODO(cais): Add sub-interfaces of `BaseTensorHealthPill` for other tensor


Nit: update 'HealthPill' in this comment, maybe also in the filename, and search the rest of the code for any other instances

davidsoergel · 2019-10-07T15:37:16Z

tensorboard/plugins/debugger/tf_debugger_dashboard/tf-debugger-dashboard.html

@@ -178,6 +178,7 @@
              id="tensorValueMultiView"
              continue-to-callback="[[_createContinueToCallback()]]"
              tensor-name-clicked="[[_createNodeClickedHandler()]]"
+              get-health-pill="[[_createGetHealthPill()]]"


get-numeric-summary, _createGetNumericSummary()

This part belongs to the existing debugger plugin code, which has called it "health-pill" for a while. For the sake of consistency, I'd like to avoid changing the terminology here.

davidsoergel · 2019-10-07T15:37:31Z

tensorboard/plugins/debugger/tf_debugger_dashboard/tf-tensor-value-multi-view.html

@@ -52,6 +52,8 @@
          type: Number,
          value: 0,
        },
+
+        getHealthPill: Function,


davidsoergel · 2019-10-07T15:39:45Z

tensorboard/plugins/debugger/tf_debugger_dashboard/tf-tensor-value-view.html

@@ -307,6 +311,31 @@
                  .catch((err) => reject(err));
              });
            },
+            getNumericSummary: async () => {
+              return new Promise((resolve, reject) => {


Avoid mixing async/await with explicit Promise/callback style-- it's confusing. Since this function is already async, you can throw an error instead of reject(), and return instead of resolve().

I agree with what you said in principle. But unfortunately this doesn't work in this Polymer code when I tried it. So I'll keep this style for now. Hopefully it won't be a problem when the debugger plugin v2 is written in Angular or other more modern frameworks.

davidsoergel · 2019-10-07T15:44:27Z

tensorboard/components/tensor_widget/demo/demo.ts

+            maximum = value;
+          }
+        }
+        (numericSummary as BooleanOrNumericTensorNumericSummary).minimum = minimum;


Can numericSummary have this detailed type in the first place, avoiding the need for the cast? Will there be any other concrete types?

davidsoergel · 2019-10-07T15:49:40Z

tensorboard/components/tensor_widget/tensor-widget-impl.ts

+  private menu: Menu | null = null;
+
+  // Value render mode.
+  protected valueRenderMode: 'text' | 'image';


nit: my instinct is to use an enum here (maybe with string values, e.g. TEXT = 'text'). On the other hand, I don't know why I think that, and this also works fine.

Done changing it to an enum.

davidsoergel · 2019-10-07T15:58:51Z

tensorboard/components/tensor_widget/menu.ts

@@ -0,0 +1,278 @@
+/* Copyright 2019 The TensorFlow Authors. All Rights Reserved.


File comment: this revives my concern about integrating TensorWidget into a framework-based app. I.e., we'll want to make the look & feel match https://material.angular.io/components/menu. We can commit this anyway and deal with the Angular (etc.?) integration when the time comes.

One important behavior difference is that these menus disappear when you stop hovering on them, whereas Material (and MacOS, etc.) menus persist until you click elsewhere. That may be worth fixing here in any case.

I'm still not convinced the value of adding a dependency overweighs the benefit of increased package size and build complexity. I've fixed the menu mouseleave callback issue you pointed out below. I guess we can leave this as an open point. If further UI widget needs come up in the future, we can revisit it.

davidsoergel · 2019-10-07T16:02:02Z

tensorboard/components/tensor_widget/menu.ts

+        }
+        menuItem.classList.add('tensor-widget-dim-dropdown-menu-item-active');
+      });
+      menuItem.addEventListener('mouseleave', () => {


Ideally mouseleave should be a no-op, and this logic should trigger on a click anywhere outside the menu. Maybe that requires a transparent page-sized element to catch the clicks? I don't know how Angular Material components handle this case. I did a React version previously; I don't remember how it worked but let me know if it would help to look it up.

Done. Made mouseleave a no-op as you suggested.

caisq

Thanks for the review, @davidsoergel

caisq · 2019-10-09T13:07:58Z

tensorboard/components/tensor_widget/health-pill-types.ts

+  /** Maximum of all finite values. */
+  maximum: number | boolean;
+}
+
 // TODO(cais): Add sub-interfaces of `BaseTensorHealthPill` for other tensor


caisq · 2019-10-09T13:09:26Z

tensorboard/components/tensor_widget/menu.ts

@@ -0,0 +1,278 @@
+/* Copyright 2019 The TensorFlow Authors. All Rights Reserved.


I'm still not convinced the value of adding a dependency overweighs the benefit of increased package size and build complexity. I've fixed the menu mouseleave callback issue you pointed out below. I guess we can leave this as an open point. If further UI widget needs come up in the future, we can revisit it.

caisq · 2019-10-09T13:20:17Z

tensorboard/components/tensor_widget/tensor-widget-impl.ts

+  private menu: Menu | null = null;
+
+  // Value render mode.
+  protected valueRenderMode: 'text' | 'image';


Done changing it to an enum.

caisq · 2019-10-09T13:21:16Z

tensorboard/plugins/debugger/tf_debugger_dashboard/tf-debugger-dashboard.html

@@ -178,6 +178,7 @@
              id="tensorValueMultiView"
              continue-to-callback="[[_createContinueToCallback()]]"
              tensor-name-clicked="[[_createNodeClickedHandler()]]"
+              get-health-pill="[[_createGetHealthPill()]]"


This part belongs to the existing debugger plugin code, which has called it "health-pill" for a while. For the sake of consistency, I'd like to avoid changing the terminology here.

caisq · 2019-10-09T13:21:21Z

tensorboard/plugins/debugger/tf_debugger_dashboard/tf-tensor-value-multi-view.html

@@ -52,6 +52,8 @@
          type: Number,
          value: 0,
        },
+
+        getHealthPill: Function,


caisq · 2019-10-09T13:25:57Z

tensorboard/plugins/debugger/tf_debugger_dashboard/tf-tensor-value-view.html

@@ -307,6 +311,31 @@
                  .catch((err) => reject(err));
              });
            },
+            getNumericSummary: async () => {
+              return new Promise((resolve, reject) => {


I agree with what you said in principle. But unfortunately this doesn't work in this Polymer code when I tried it. So I'll keep this style for now. Hopefully it won't be a problem when the debugger plugin v2 is written in Angular or other more modern frameworks.

caisq · 2019-10-09T14:31:37Z

tensorboard/components/tensor_widget/menu.ts

+        }
+        menuItem.classList.add('tensor-widget-dim-dropdown-menu-item-active');
+      });
+      menuItem.addEventListener('mouseleave', () => {


Done. Made mouseleave a no-op as you suggested.

ticks

davidsoergel

LGTM, thanks for the fixes!

caisq added 6 commits October 2, 2019 17:34

[tensor-widget] Add Menu

cf1599c

save

6524ccb

save

5edecba

save

e638715

Added colormap

d721c65

Added basic image support

a7091e1

googlebot added the cla: yes label Oct 4, 2019

caisq added 5 commits October 4, 2019 17:05

Save

d4349f2

Add some doc strings

228d070

Tweak some doc strings

839ccd4

Fix empty menu; add zoom-in / zoom-out options

b589e54

Adapt TensorView implementation in the debugger v1 plugin

49f4572

caisq requested a review from davidsoergel October 5, 2019 03:44

davidsoergel reviewed Oct 7, 2019

View reviewed changes

caisq added 2 commits October 9, 2019 09:21

Save

f381663

Fix mouse leave behavior in menus

154afc4

caisq commented Oct 9, 2019

View reviewed changes

caisq requested a review from davidsoergel October 9, 2019 14:36

caisq added 3 commits October 9, 2019 10:49

Dynamically determine if indices should be displayed on the ruler ticks

8e4e371

Refactor code for calculating whether indices show be shown on ruler

ebca888

ticks

Save

f0f8773

davidsoergel approved these changes Oct 9, 2019

View reviewed changes

caisq merged commit 185c0c4 into tensorflow:master Oct 9, 2019

caisq deleted the tensor-widget-menu branch October 9, 2019 20:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tensor-widget] Add support for hierarchical menu and image view of tensor values #2729

[tensor-widget] Add support for hierarchical menu and image view of tensor values #2729

caisq commented Oct 4, 2019 •

edited

Loading

davidsoergel Oct 7, 2019

caisq Oct 9, 2019

davidsoergel Oct 7, 2019

caisq Oct 9, 2019

davidsoergel Oct 7, 2019

caisq Oct 9, 2019

davidsoergel Oct 7, 2019

caisq Oct 9, 2019

davidsoergel Oct 7, 2019

caisq Oct 9, 2019

davidsoergel Oct 7, 2019

caisq Oct 9, 2019

davidsoergel Oct 7, 2019

caisq Oct 9, 2019

davidsoergel Oct 7, 2019

caisq Oct 9, 2019

caisq left a comment

caisq Oct 9, 2019

caisq Oct 9, 2019

caisq Oct 9, 2019

caisq Oct 9, 2019

caisq Oct 9, 2019

caisq Oct 9, 2019

caisq Oct 9, 2019

davidsoergel left a comment

		@@ -0,0 +1,278 @@
		/* Copyright 2019 The TensorFlow Authors. All Rights Reserved.

[tensor-widget] Add support for hierarchical menu and image view of tensor values #2729

[tensor-widget] Add support for hierarchical menu and image view of tensor values #2729

Conversation

caisq commented Oct 4, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

caisq 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

Choose a reason for hiding this comment

davidsoergel left a comment

Choose a reason for hiding this comment

caisq commented Oct 4, 2019 •

edited

Loading