Skip to content

Conversation

@JamesHollyer
Copy link
Contributor

Motivation for features / changes

We are adding lots of functionality to the scalar data table and the runs table. For consistency and reuse we are replacing the table in the runs table with a DataTable widget. This is the first step towards that change.

Technical description of changes

Besides the data all the values which are passed in are hard coded into the runs_table_container. This is done so that we have a data table to work with quickly and unblock some other work. These values will be set up in ngrx in future PRs.

Screenshots of UI changes (or N/A)

We have a plan to restyle this table for the Runs Table. However, that will come later. For now it is just the standard data table which looks like this.
Screenshot 2023-05-17 at 4 13 21 PM

Alternate designs / implementations considered (or N/A)

We considered leaving the runs table and adding all features to both.

let selectSpy: jasmine.Spy;

// To make sure we only return the runs when called with the right props.
selectSpy = spyOn(store, 'select').and.callThrough();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
selectSpy = spyOn(store, 'select').and.callThrough();
const selectSpy = spyOn(store, 'select').and.callThrough();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Comment on lines 3195 to 3196
let selectSpy: jasmine.Spy;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
let selectSpy: jasmine.Spy;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

sortOption$ = this.store.select(getRunSelectorSort);
paginationOption$ = this.store.select(getRunSelectorPaginationOption);
regexFilter$ = this.store.select(getRunSelectorRegexFilter);
HParamsEnabled = new BehaviorSubject<boolean>(false);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does this need to be a BehaviorSubject? It seems much more complicated this way

Suggested change
HParamsEnabled = new BehaviorSubject<boolean>(false);
hparamsEnabled$ = this.store.select(getEnableHparamsInTimeSeries);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't think I could use an observable as a value in this component.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried doing the *ngIf with an async pipe. It did not work. Leaving it as is.

Comment on lines +572 to +598
private getRunTableDataForExperiment(
experimentId: string
): Observable<TableData[]> {
return combineLatest([
this.store.select(getRuns, {experimentId}),
this.store.select(getRunColorMap),
]).pipe(
map(([runs, colorMap]) => {
return runs.map((run) => {
const tableData: TableData = {
id: run.id,
color: colorMap[run.id],
};
this.runsColumns.forEach((column) => {
switch (column.type) {
case ColumnHeaderType.RUN:
tableData[column.name!] = run.name;
break;
default:
break;
}
});
return tableData;
});
})
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be a selector, but I don't mind it being this way for now (until I get my generalized selector checked in).

Could this at least be a lot shorter?

Suggested change
private getRunTableDataForExperiment(
experimentId: string
): Observable<TableData[]> {
return combineLatest([
this.store.select(getRuns, {experimentId}),
this.store.select(getRunColorMap),
]).pipe(
map(([runs, colorMap]) => {
return runs.map((run) => {
const tableData: TableData = {
id: run.id,
color: colorMap[run.id],
};
this.runsColumns.forEach((column) => {
switch (column.type) {
case ColumnHeaderType.RUN:
tableData[column.name!] = run.name;
break;
default:
break;
}
});
return tableData;
});
})
);
}
const runColumnName = this.runColumns.find((column) => column.type === ColumnHeaderType.RUN).name;
return runs.map((run) => ({
id: run.id,
color: colorMap[run.id],
[runColumnName]: run.name
});

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would rather leave it as is. It is more extensible this way for other columns. I definitely see this being moved to a selector. However, the selector would be similar to this structure to accommodate all columns instead of just run.

@JamesHollyer JamesHollyer requested a review from rileyajones May 19, 2023 18:28
@JamesHollyer JamesHollyer merged commit 7fc7f28 into tensorflow:master May 19, 2023
rileyajones added a commit that referenced this pull request May 25, 2023
## Motivation for features / changes
As part the effort to add hparams to time series we have updated the
runs table to use the data table in #6394. Now I am updating the runs
data table to read its columns from the redux state.

## Screenshots of UI changes (or N/A)
N/A (but there will be soon!)

## Alternate designs / implementations considered (or N/A)
We could have reused the existing sorting property in redux and added
keys to it, but I prefered to create a seperate one and remove the
existing property when we remove the old runs table.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants