[tensor-widget] Add slicing control to support 3D+ tensors #2565

caisq · 2019-08-16T16:57:59Z

Motivation for features / changes
- Continue developing TensorWidget
Technical description of changes
- Add a slicing control UI, included in the newly created file silcing-control.ts
- This UI allows the user to move between slices and to flexibly change which dimensions of a 3D+ tensor are sliced and which are viewed in the value table.
- The dimension selection is supported by an ad hoc dropdown menu.
Screenshots of UI changes

The dropdown menu for swapping a viewing dimension with a slicing one:

The div for a slicing dimension becomes a number input box when clicked.

Co-author: @bileschi

bileschi

Some high level comments. No big deal and definitely not a blocker. Consider adding bugs to track.

The slice-spec drop-downs don't close nicely if you open one and then click outside of it. To close it you must first click on it.
The 'swap with x' functionality is difficult to keep track of. I find myself wishing there were a clearer way to know what I'm looking at. Some sort of highlighting of the 'shape' field in the top bar may help. Perhaps a way to click on the shape itself to set which direction is 'rows' and which direction is 'columns', and leave the others to be slices. It can still be constrained such that the 'rows' dim must be before the 'columns' dim.
There's a bit more space for the tensor name in the top left. Consider giving a few more characters up there.

bileschi · 2019-08-19T16:21:19Z

tensorboard/components/tensor_widget/shape-utils.ts

+ * @return `true` if and only if the slicing dimensions and/or the viewing
+ *   dimensions differ between the `spec0` and `spec1`.
+ */
+export function dimensionsDiffer(


Consider the inverse for readability:

compatibleSlicingSpec: which is true if viewing slicingSpec matches except for the {horizontal/vertical}range.

.. and the slice index

I renamed to to areSlicingSpecsCompatible for better clarity.

bileschi · 2019-08-19T16:21:24Z

tensorboard/components/tensor_widget/shape-utils.ts

+  spec0: TensorViewSlicingSpec,
+  spec1: TensorViewSlicingSpec
+): boolean {
+  if (spec0.viewingDims[0] !== spec1.viewingDims[0]) {


Is this the canonical way to do this even if the length of the constituent arrays may be less than 1?

If the length is less than 1, then [0] will give you undefined, which works for the logic here.

bileschi · 2019-08-19T16:24:01Z

tensorboard/components/tensor_widget/shape-utils-test.ts

+    expect(dimensionsDiffer(spec0, spec1)).to.be.false;
+  });
+
+  it('Differen slicing and viewing dimensions are captured', () => {


nit differenT

bileschi · 2019-08-19T16:29:02Z

tensorboard/components/tensor_widget/slicing-control.ts

+  private slicingSpec: TensorViewSlicingSpec;
+
+  // Constituent UI components.
+  // private rootDiv: HTMLDivElement;


Is this commented line supposed to be here? Vestigial?

Removed. Thanks for the catch.

bileschi · 2019-08-19T16:37:43Z

tensorboard/components/tensor_widget/slicing-control.ts

+    private readonly shape: Shape,
+    private readonly onSlicngSpecChange?: OnSlicingSpecChangeCallback
+  ) {
+    this.rank = this.shape.length;


This seems like it could become a problem, as it is now possible to have the rank != this.shape.length, if someone changes one but not the other. Would it be better to replace all occurrences of this.rank with this.shape.length , which isn't that much longer??

This goes back to how the tensor widget works. If the underlying tensor's rank changes, the UI widget won't reflect that until its render() method is called again. However, when the render() method is called, it'll always create a new SlicingControl instance, which will reflect the up-to-date rank.

bileschi · 2019-08-19T16:48:03Z

tensorboard/components/tensor_widget/slicing-control.ts

+   */
+  render(slicingSpec?: TensorViewSlicingSpec) {
+    if (slicingSpec != null) {
+      this.slicingSpec = JSON.parse(JSON.stringify(slicingSpec));


What does this stringify-parse loop accomplish?

This is a way to deep-copy the object. See the accepted answer in https://stackoverflow.com/questions/122102/what-is-the-most-efficient-way-to-deep-clone-an-object-in-javascript

We want to avoid mutating the input argument slicingSpec.

Sure, but note jeresig’s comment and the actual benchmarks showing that
stringify–parse is (as should be expected) the slowest way to copy:
https://stackoverflow.com/a/5344074/

We already have a lodash dependency; could you please use _.cloneDeep
instead?

The object being deep-copied here is tiny. This method is not called frequently. If the performance becomes and issue in the future, we can consider using lodash. But for now, I think the benefit of one fewer dependency outweighs the tiny performance gain. (TensorWidget is intended to be usable as a standalone package)

bileschi · 2019-08-19T16:55:29Z

tensorboard/components/tensor_widget/slicing-control.ts

+            this.slicingSpec.slicingDimsAndIndices[
+              slicingDims.indexOf(i)
+            ].index = newIndex;
+            dimControl.textContent = `${newIndex}/${dimSize}`;


consider removing the '../${dimSize} here and elsewhere. IMO it adds visual noise and the maximum dimension value is already available from the shape field above.

compare

vs.

or even without the arrows, since row-then-column order is constrained:

bileschi · 2019-08-19T17:02:10Z

tensorboard/components/tensor_widget/slicing-control.ts

+   */
+  private renderDropdownMenuItems(
+    dropdown: HTMLDivElement,
+    top: number,


consider using a coord.XY pattern here, to collect the two values with similar meaning & use into a common object.

/**

Represents a position in a planar coordinate frame.
*/
export interface XYCoordinates {
x: number;
y: number;
}

The top and left arg names correspond nicely to the return type of getBoundingClientRect(). I'd argue that creating a Coordinate-like interface or struct only adds confusion, because coordinates are usually x and y, but in the HTML coordinate system, things are called left, right, top and bottom. It would be confusing if force associate left with x and top with y; it'll also be confusing if you let the members of the interface be called left and top and so forth.

caisq · 2019-08-20T02:45:55Z

@bileschi Thanks for the very helpful review. Listed you as a co-author in this PR if that's okay with you.

Regarding the point of the dropdown menu being hard to get rid of, realize that the dropdown menu has a 'mouseleave' listener that closes itself if the mouse cursor enters and the leaves it, even without clicking. But to address the problem you pointed out, I added an extra 'mouseleave' listener, i.e., to the slicing control bar, such that if the mouse goes out of that region, the dropdown menu goes away by itself.
Regarding the point that it's hard to keep track of which dimensions are being swapped in the dropdown menu, I added hover-based visual highlighting for the dropdown menu. For example, see the screenshot below. When you hover the mouse cursor on a menu item of the dropdown, the dimension with which it'll be swapped with will be highlighted by a color outline and bold font weight.

tensorboard/components/tensor_widget/shape-utils.ts

caisq · 2019-08-20T03:00:00Z

Regarding the point that there is some extra space for tensor name, note that the top-right part of the widget is reserved for health pill, which hasn't been implemented yet, but will be soon.

caisq · 2019-08-20T03:12:59Z

Added commas to the slicing control bar, so that it looks like a Python/numpy slicing string, which IMO facilitates understanding of the UI.

Screenshot:

wchargin · 2019-08-20T03:28:15Z

tensorboard/components/tensor_widget/slicing-control.ts

+              Math.floor(dimSize) != dimSize
+            ) {
+              // Reject invalid value.
+              dimInput.value = `${this.slicingSpec.slicingDimsAndIndices[slicingDims.indexOf(i)].index}`;


Please use String(foo) instead of `${foo}`.

Done throughout this file.

wchargin · 2019-08-20T03:29:18Z

tensorboard/components/tensor_widget/slicing-control.ts

+              slicingDims.indexOf(i)
+            ].index = newIndex;
+            dimControl.textContent = `${newIndex}/${dimSize}`;
+            this.onSlicngSpecChange(this.slicingSpec);


s/Slicng/Slicing/?

Fixed. Thanks for the catch.

wchargin · 2019-08-20T03:33:57Z

tensorboard/components/tensor_widget/slicing-control.ts

+   */
+  render(slicingSpec?: TensorViewSlicingSpec) {
+    if (slicingSpec != null) {
+      this.slicingSpec = JSON.parse(JSON.stringify(slicingSpec));


Sure, but note jeresig’s comment and the actual benchmarks showing that
stringify–parse is (as should be expected) the slowest way to copy:
https://stackoverflow.com/a/5344074/

We already have a lodash dependency; could you please use _.cloneDeep
instead?

caisq added 8 commits August 15, 2019 13:55

[tensor-widget] Add slicing control to support 3D+ tensors

3036f80

Checkpoint

8bc5f0e

Fix slicing

867beba

Add 4D example in demo.ts and demo.html

7d07035

Checkpoint

8904959

Checkpoint

cc87647

Working checkpoint; apply prettier

61989f5

Add unit tests

d807cae

googlebot added the cla: yes label Aug 16, 2019

Improve doc strings

e7e579d

caisq requested a review from bileschi August 16, 2019 18:43

bileschi reviewed Aug 19, 2019

View reviewed changes

bileschi approved these changes Aug 19, 2019

View reviewed changes

Address some comments

4d46975

bileschi approved these changes Aug 20, 2019

View reviewed changes

tensorboard/components/tensor_widget/shape-utils.ts Outdated Show resolved Hide resolved

Address comments

6b82cf2

Add commas to slicing control

b7ac3bd

wchargin reviewed Aug 20, 2019

View reviewed changes

caisq added 3 commits August 19, 2019 23:46

Address William's comments

4899220

Apply yarn fix-lint

74bafe0

Fix a bug in the allowed swaps in the slicing control

59a17cf

caisq merged commit ae205a7 into tensorflow:master Aug 20, 2019

caisq deleted the tensor-widget-3f branch August 20, 2019 13:51

[tensor-widget] Add slicing control to support 3D+ tensors #2565

[tensor-widget] Add slicing control to support 3D+ tensors #2565

Uh oh!

Conversation

caisq commented Aug 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bileschi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caisq commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

caisq commented Aug 20, 2019

Uh oh!

caisq commented Aug 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

caisq commented Aug 16, 2019 •

edited

Loading

caisq commented Aug 20, 2019 •

edited

Loading