HParams: project table content components #6427

JamesHollyer · 2023-06-09T21:27:25Z

Motivation for features / changes

This is part of the effort to use the data table widget in the Runs table which will allow for lots of code reuse as we add HParam functionality to both tables. To allow more customization in each table this PR pulls all the table content out of the data_table_component and allows users to project it using the new content_row_component and content_cell_component.

Technical description of changes

Created new content_row_component and content_cell_component. The row is extremely basic and basically just wraps the content inside a table-row. The content_cell_component take all the logic for formatting each cell. To customize the content users of this component can simply pass in no datum or a ColumnHeaderType that does not have a specified way to format in the getFormattedDataForColumn function and then put the custom content inside the cell as a child it will be projected in. This is currently shown in the way the ScalarCardDataTable handles the Color column.

Screenshots of UI changes (or N/A)

This should result in no changes to the Scalar Data Table. However, the Runs Data Table is now empty as it has not implemented this new DataTable structure. It is behind a flag though and will be implemented in an upcoming PR.

Detailed steps to verify changes work correctly (as executed by you)

Ran it and clicked around a lot to try breaking.

Alternate designs / implementations considered (or N/A)

We considered some way to allow the data table to be highly configured instead of projecting the content.
I also considered not creating the content_row_component class and requiring parents to add their own div with class=table-row.

rileyajones · 2023-06-09T21:31:34Z

tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts

+    `
+      .row-circle {
+        height: 12px;
+        width: 12px;
+      }
+      .row-circle > span {
+        border-radius: 50%;
+        border: 1px solid rgba(255, 255, 255, 0.4);
+        display: inline-block;
+        height: 10px;
+        width: 10px;
+        vertical-align: middle;
+      }
+    `,


Now that the template has been moved into a standalone file, I would rather see the scss also move to a standalone file

rileyajones · 2023-06-09T21:32:10Z

tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts

        return 0;
      });
    }
+    // console.log(JSON.parse(JSON.stringify(dataTableData)) as TableData[]);


Oops?

Suggested change

// console.log(JSON.parse(JSON.stringify(dataTableData)) as TableData[]);

rileyajones · 2023-06-09T21:32:25Z

tensorboard/webapp/widgets/data_table/content_row_component.ts

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


rileyajones · 2023-06-09T21:32:36Z

tensorboard/webapp/widgets/data_table/content_cell_component.ts

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


Suggested change

/* Copyright 2022 The TensorFlow Authors. All Rights Reserved.

/* Copyright 2023 The TensorFlow Authors. All Rights Reserved.

rileyajones · 2023-06-09T21:33:06Z

tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts

+      }
+      .row-circle > span {
+        border-radius: 50%;
+        border: 1px solid rgba(255, 255, 255, 0.4);


How does this look in dark mode?

This style is copied so it looks the same as before. I checked it and I think it looks just fine in dark mode.

rileyajones · 2023-06-09T21:35:07Z

tensorboard/webapp/widgets/data_table/data_table_component.ng.html

-      </ng-container>
-    </div>
-  </ng-container>
+  <ng-content select="[content]"></ng-content>


This is a thing of beauty

rileyajones · 2023-06-09T21:35:31Z

tensorboard/webapp/widgets/data_table/data_table_component.scss


-  .header,
-  .row {
+  .header {


Is this style still being used?

Yes, the header class is used as the "row" for the headers.

rileyajones · 2023-06-09T21:35:56Z

tensorboard/webapp/widgets/data_table/data_table_test.ts

 }

-describe('data table', () => {
+fdescribe('data table', () => {


Suggested change

fdescribe('data table', () => {

describe('data table', () => {

I was still working on the tests. Should be fixed now.

rileyajones · 2023-06-09T21:36:44Z

tensorboard/webapp/widgets/data_table/content_cell_component.ts

+  template: `
+    <ng-container [ngSwitch]="header.type">
+      <div *ngSwitchCase="ColumnHeaderType.VALUE_CHANGE" class="cell">
+        <ng-container
+          *ngTemplateOutlet="arrow; context: {$implicit: datum}"
+        ></ng-container>
+        {{ getFormattedDataForColumn() }}
+      </div>
+      <div *ngSwitchCase="ColumnHeaderType.PERCENTAGE_CHANGE" class="cell">
+        <ng-container
+          *ngTemplateOutlet="arrow; context: {$implicit: datum}"
+        ></ng-container>
+        {{ getFormattedDataForColumn() }}
+      </div>
+      <div *ngSwitchDefault class="cell extra-right-padding">
+        {{ getFormattedDataForColumn() }}
+      </div>
+      <ng-content></ng-content>
+    </ng-container>
+    <ng-template #arrow let-value>
+      <mat-icon *ngIf="value >= 0" svgIcon="arrow_upward_24px"></mat-icon>
+      <mat-icon *ngIf="value < 0" svgIcon="arrow_downward_24px"></mat-icon>
+    </ng-template>
+  `,
+  styles: [
+    `
+      :host {
+        display: table-cell;
+        padding: 1px;
+      }
+      .cell {
+        align-items: center;
+        display: flex;
+      }
+
+      .cell mat-icon {
+        height: 12px;
+        width: 12px;
+
+        ::ng-deep path {
+          fill: unset;
+        }
+      }
+
+      .extra-right-padding {
+        padding-right: 1px;
+      }
+    `,
+  ],


At this size I would like to see the template and styles broken out into their own components

rileyajones · 2023-06-09T21:37:05Z

tensorboard/webapp/widgets/data_table/content_cell_component.ts

+        ::ng-deep path {
+          fill: unset;
+        }


This may conflict with Brendas work in #6410

I do not think this will cause problems as all of her changes are in the header.

rileyajones · 2023-06-13T16:54:27Z

tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts

+        expect(secondRowContentCells.length).toEqual(3);
+
+        expect(secondRowContentCells[0].componentInstance.header.name).toEqual(
+          'color'
+        );
+        expect(secondRowContentCells[1].componentInstance.header.name).toEqual(
+          'run'
+        );
+        expect(secondRowContentCells[2].componentInstance.header.name).toEqual(
+          'step'
+        );


This seems like it an excellent use-case for a map

Suggested change

expect(secondRowContentCells.length).toEqual(3);

expect(secondRowContentCells[0].componentInstance.header.name).toEqual(

'color'

);

expect(secondRowContentCells[1].componentInstance.header.name).toEqual(

'run'

);

expect(secondRowContentCells[2].componentInstance.header.name).toEqual(

'step'

);

expect(secondRowContentCells.map((cell) => cell.componentInstance.header.name)).toEqual(['color', 'run', 'step']);

I made a lot of changes here. I moved stuff to before each. Change multiple things to use map like this. I removed the expects for cell header names when testing the data. I also changed the smooth test to just have the expect(find smoothed) to be falsy style tests.

rileyajones · 2023-06-13T16:57:12Z

tensorboard/webapp/widgets/data_table/data_table_component.ng.html

 <div class="data-table">
  <div class="header">
-    <div class="col"></div>
+    <div class=".col"></div>


It looks like this class was removed.

This the '.' character should not be here

Suggested change

<div class=".col"></div>

This is the hard coded header for the color column. There was supposed to be no change on this line. I did mess with it a bit and apparently I accidentally made this change.

Interesting side note. This did not break the table. Apparently CSS is smart enough to use this as a place holder for a cell even when the display is not set to table-cell.

rileyajones · 2023-06-13T17:04:33Z

tensorboard/webapp/runs/views/runs_table/runs_table_test.ts

+    // Currently nothing is passed to the data table from the runs table. This
+    // is because of a data table refactor.
+    // TODO(JamesHollyer): reenable and fix tests once runs table implements new
+    // data table structure.


Because this is hidden behind a feature flag I'm fine with this.
Can you elaborate a bit on how you think this process should happen? Do you intend to update the runs table implementation soon, or will this just be some kind of tech debt to be resolved prior to the launch of the hparams in time series feature?

I plan to implement the projection in the runs table in my next PR. These tests will be enabled and fixed in that PR.

rileyajones · 2023-06-13T22:27:55Z

tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts

 import {
  ChangeDetectorRef,
  Component,
+  DebugElement,


Is this being used?

Not anymore. Good catch.

## Motivation for features / changes In #6422 we started projecting the header components into the data table widget. However, the content was still not projected and the color column was hard coded in. Therefore we left a hard coded color header. The content started being projected in #6427. Therefore we can stop hardcoding this color column. This PR removes the hardcoded header column and starts adding it from the ScalarCardDataTable ## Technical description of changes The color header cannot be sorted, dismissed, or dragged. There was some logic in place for this. However, it was incomplete. This completes that logic and uses it for the color header. We also add tests for that logic which were not in place before. Note: I did some test clean up in the HeaderCell ## Screenshot of changes It still works. <img width="148" alt="Screenshot 2023-06-15 at 5 41 33 AM" src="https://github.com/tensorflow/tensorboard/assets/8672809/0fe6e62e-f79a-46a8-bf60-cad3d87a28d4"> ComponentTests for consistency. --------- Co-authored-by: Riley Jones <78179109+rileyajones@users.noreply.github.com>

#6438) ## Motivation for features / changes We preparation for adding HParam functionality to both the Scalar Table and the Runs Table we have refactored the Data Table to be more flexible. This refactoring was done in #6422 and #6427 and was only implemented in the Scalar Table because the Runs Table only uses the Data Table behind a flag. This PR implements the new Data Table Structure in the Runs Table. ## Technical description of changes Basically just followed the same structure as the Scalar Table. ## Screenshots of UI changes (or N/A) Runs Table specific styling still needs to be done. It currently looks like this. <img width="392" alt="Screenshot 2023-06-15 at 7 21 46 PM" src="https://github.com/tensorflow/tensorboard/assets/8672809/2dd13003-ad93-4b66-89eb-579a497d6708"> We still need to add a column for check boxes and add styling.

## Motivation for features / changes In #6438 we rewrote the Runs Data Table to use the new projection Data Table structure(Created in #6427 and #6422). This allows us to project custom controls such as the run selection check boxes. This PR implements those checkboxes. ## Technical description of changes Pretty straight forward use of projection. The weird part was that the projected checkbox in the header did not fire the "change" event while the header itself had a subscription to a "click" event for sorting. The solution was to subscribe to the "click" event and that seems to work just fine. ## Screenshots of UI changes (or N/A) All selected: <img width="230" alt="Screenshot 2023-06-20 at 2 40 19 PM" src="https://github.com/tensorflow/tensorboard/assets/8672809/164ab4c2-b2bd-47ad-b46d-9dbcf78c976a"> Some selected: <img width="228" alt="Screenshot 2023-06-20 at 2 40 27 PM" src="https://github.com/tensorflow/tensorboard/assets/8672809/0822e363-ac08-45a3-9102-4c1959192c9f"> None selected: <img width="211" alt="Screenshot 2023-06-20 at 2 40 35 PM" src="https://github.com/tensorflow/tensorboard/assets/8672809/9ce0b8aa-182d-4796-a59b-5e05fc655f2e">

rileyajones reviewed Jun 9, 2023

View reviewed changes

JamesHollyer force-pushed the ProjectTableContent branch 2 times, most recently from bdf3f9e to c2be6f7 Compare June 10, 2023 06:52

JamesHollyer added 2 commits June 12, 2023 16:39

HParams: project table content components

19f4822

add tests

edd7475

JamesHollyer force-pushed the ProjectTableContent branch from c2be6f7 to edd7475 Compare June 12, 2023 22:18

JamesHollyer requested a review from rileyajones June 13, 2023 00:04

rileyajones reviewed Jun 13, 2023

View reviewed changes

clean up tests and revert color header change

7a6a560

JamesHollyer requested a review from rileyajones June 13, 2023 19:06

rileyajones approved these changes Jun 13, 2023

View reviewed changes

remove unused import

4319d45

JamesHollyer merged commit fa91799 into tensorflow:master Jun 14, 2023

JamesHollyer mentioned this pull request Jun 14, 2023

HParams: project color header #6434

Merged

JamesHollyer mentioned this pull request Jun 16, 2023

HParam: Implement new Data Table projection architecture in Runs Table #6438

Merged

JamesHollyer mentioned this pull request Jun 20, 2023

HParams: Add selected checkbox column to Runs table #6442

Merged

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

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

	/* Copyright 2022 The TensorFlow Authors. All Rights Reserved.
	/* Copyright 2023 The TensorFlow Authors. All Rights Reserved.

	fdescribe('data table', () => {
	describe('data table', () => {

HParams: project table content components #6427

HParams: project table content components #6427

Uh oh!

Conversation

JamesHollyer commented Jun 9, 2023

Motivation for features / changes

Technical description of changes

Screenshots of UI changes (or N/A)

Detailed steps to verify changes work correctly (as executed by you)

Alternate designs / implementations considered (or N/A)

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants