From 89fc04a3eb5a09d66694c72ff1b167816ee7c441 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Wed, 24 Jan 2024 12:18:53 +0900 Subject: [PATCH 01/22] Adds shared hparam related common selectors --- .../hparams/_redux/hparams_selectors.ts | 6 +- .../hparams/_redux/hparams_selectors_test.ts | 70 +++-- .../webapp/metrics/views/main_view/BUILD | 1 + .../views/main_view/common_selectors.ts | 60 ++++- .../views/main_view/common_selectors_test.ts | 246 ++++++++++++++---- 5 files changed, 299 insertions(+), 84 deletions(-) diff --git a/tensorboard/webapp/hparams/_redux/hparams_selectors.ts b/tensorboard/webapp/hparams/_redux/hparams_selectors.ts index 90c8b360d0..b5312c2cf9 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_selectors.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_selectors.ts @@ -47,8 +47,12 @@ export const getDashboardDefaultHparamFilters = createSelector( ); export const getDashboardDisplayedHparamColumns = createSelector( + getDashboardHparamsAndMetricsSpecs, getHparamsState, - (state) => state.dashboardDisplayedHparamColumns + ({hparams}, state) => { + const hparamSet = new Set(hparams.map(hparam => hparam.name)); + return state.dashboardDisplayedHparamColumns.filter(column => hparamSet.has(column.name)); + } ); export const getDashboardHparamFilterMap = createSelector( diff --git a/tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts b/tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts index cc43c0dfa2..fcdc2e5a6e 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts @@ -15,6 +15,7 @@ limitations under the License. import {ColumnHeaderType} from '../../widgets/data_table/types'; import {DomainType} from '../types'; +import {State} from './types'; import * as selectors from './hparams_selectors'; import { buildHparamSpec, @@ -114,30 +115,63 @@ describe('hparams/_redux/hparams_selectors_test', () => { }); describe('#getDashboardDisplayedHparamColumns', () => { - it('returns dashboard displayed hparam columns', () => { - const fakeColumns = [ - { - type: ColumnHeaderType.HPARAM, - name: 'conv_layers', - displayName: 'Conv Layers', - enabled: true, - }, - { - type: ColumnHeaderType.HPARAM, - name: 'conv_kernel_size', - displayName: 'Conv Kernel Size', - enabled: true, - }, - ]; + it('returns no columns if no hparam specs', () => { const state = buildStateFromHparamsState( buildHparamsState({ - dashboardDisplayedHparamColumns: fakeColumns, + dashboardSpecs: { + hparams: [], + }, + dashboardDisplayedHparamColumns: [ + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + ], }) ); - expect(selectors.getDashboardDisplayedHparamColumns(state)).toEqual( - fakeColumns + expect(selectors.getDashboardDisplayedHparamColumns(state)).toEqual([]); + }); + + it('returns only dashboard displayed hparam columns', () => { + const state = buildStateFromHparamsState( + buildHparamsState({ + dashboardSpecs: { + hparams: [buildHparamSpec({name: 'conv_layers'})], + }, + dashboardDisplayedHparamColumns: [ + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + ], + }) ); + + expect(selectors.getDashboardDisplayedHparamColumns(state)).toEqual([ + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + ]); }); }); }); diff --git a/tensorboard/webapp/metrics/views/main_view/BUILD b/tensorboard/webapp/metrics/views/main_view/BUILD index a0a2f17691..13e2f82ef5 100644 --- a/tensorboard/webapp/metrics/views/main_view/BUILD +++ b/tensorboard/webapp/metrics/views/main_view/BUILD @@ -91,6 +91,7 @@ tf_ts_library( "//tensorboard/webapp/metrics/views:utils", "//tensorboard/webapp/metrics/views/card_renderer:scalar_card_types", "//tensorboard/webapp/runs:types", + "//tensorboard/webapp/runs/store:selectors", "//tensorboard/webapp/runs/views/runs_table:types", "//tensorboard/webapp/util:matcher", "//tensorboard/webapp/util:memoize", diff --git a/tensorboard/webapp/metrics/views/main_view/common_selectors.ts b/tensorboard/webapp/metrics/views/main_view/common_selectors.ts index 20be847c66..c709fefad3 100644 --- a/tensorboard/webapp/metrics/views/main_view/common_selectors.ts +++ b/tensorboard/webapp/metrics/views/main_view/common_selectors.ts @@ -12,8 +12,8 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ -import {createSelector} from '@ngrx/store'; -import {State} from '../../../app_state'; +import { createSelector, Selector } from '@ngrx/store'; +import { State } from '../../../app_state'; import { getCurrentRouteRunSelection, getMetricsHideEmptyCards, @@ -27,12 +27,13 @@ import { getColumnHeadersForCard, getDashboardExperimentNames, } from '../../../selectors'; -import {DeepReadonly} from '../../../util/types'; +import { DeepReadonly } from '../../../util/types'; import { getDashboardMetricsFilterMap, getDashboardHparamsAndMetricsSpecs, getDashboardHparamFilterMap, getDashboardDefaultHparamFilters, + getDashboardDisplayedHparamColumns, } from '../../../hparams/_redux/hparams_selectors'; import { DiscreteFilter, @@ -44,13 +45,14 @@ import { RunTableItem, RunTableExperimentItem, } from '../../../runs/views/runs_table/types'; -import {matchRunToRegex} from '../../../util/matcher'; -import {isSingleRunPlugin, PluginType} from '../../data_source'; -import {getNonEmptyCardIdsWithMetadata, TagMetadata} from '../../store'; -import {compareTagNames} from '../../utils'; -import {CardIdWithMetadata} from '../metrics_view_types'; -import {RouteKind} from '../../../app_routing/types'; -import {memoize} from '../../../util/memoize'; +import { getRunsTableHeaders } from '../../../runs/store/runs_selectors'; +import { matchRunToRegex } from '../../../util/matcher'; +import { isSingleRunPlugin, PluginType } from '../../data_source'; +import { getNonEmptyCardIdsWithMetadata, TagMetadata } from '../../store'; +import { compareTagNames } from '../../utils'; +import { CardIdWithMetadata } from '../metrics_view_types'; +import { RouteKind } from '../../../app_routing/types'; +import { memoize } from '../../../util/memoize'; import { ColumnHeader, ColumnHeaderType, @@ -166,7 +168,7 @@ const utils = { hparamFilters: Map, metricFilters: Map ) { - return runItems.filter(({hparams, metrics}) => { + return runItems.filter(({ hparams, metrics }) => { const hparamMatches = [...hparamFilters.entries()].every( ([hparamName, filter]) => { const value = hparams.get(hparamName); @@ -264,14 +266,14 @@ export const getFilteredRenderableRuns = createSelector( export const getFilteredRenderableRunsIds = createSelector( getFilteredRenderableRuns, (filteredRenderableRuns) => { - return new Set(filteredRenderableRuns.map(({run: {id}}) => id)); + return new Set(filteredRenderableRuns.map(({ run: { id } }) => id)); } ); export const getPotentialHparamColumns = createSelector( getDashboardHparamsAndMetricsSpecs, getExperimentIdsFromRoute, - ({hparams}, experimentIds): ColumnHeader[] => { + ({ hparams }, experimentIds): ColumnHeader[] => { if (!experimentIds) { return []; } @@ -287,10 +289,42 @@ export const getPotentialHparamColumns = createSelector( sortable: true, movable: true, filterable: true, + hidable: true, })); } ); +export const getSelectableColumns = createSelector( + getPotentialHparamColumns, + getDashboardDisplayedHparamColumns, + (potentialColumns, currentColumns) => { + const currentColumnNames = new Set(currentColumns.map(({ name }) => name)); + return potentialColumns.filter((columnHeader) => { + return !currentColumnNames.has(columnHeader.name); + }); + } +); + +/** Returns a list of columns that have been sorted into logical groups. + * + * Column order: | RUN | experimentAlias | HPARAMs | other | +*/ +export const getGroupedColumns = ( + headersSelector: Selector +) => + createSelector( + headersSelector, + getDashboardDisplayedHparamColumns, + (tableHeaders, hparamHeaders): ColumnHeader[] => { + return [ + ...tableHeaders.filter((header) => header.type === 'RUN'), + ...tableHeaders.filter((header) => header.type === 'CUSTOM' && header.name === 'experimentAlias'), + ...hparamHeaders, + ...tableHeaders.filter((header) => header.type !== 'RUN' && !(header.type === 'CUSTOM' && header.name === 'experimentAlias')), + ]; + } + ); + export const getAllPotentialColumnsForCard = memoize((cardId: string) => { return createSelector( getColumnHeadersForCard(cardId), diff --git a/tensorboard/webapp/metrics/views/main_view/common_selectors_test.ts b/tensorboard/webapp/metrics/views/main_view/common_selectors_test.ts index 56e59784be..f8838684fb 100644 --- a/tensorboard/webapp/metrics/views/main_view/common_selectors_test.ts +++ b/tensorboard/webapp/metrics/views/main_view/common_selectors_test.ts @@ -12,7 +12,7 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ -import {RouteKind} from '../../../app_routing'; +import { RouteKind } from '../../../app_routing'; import { buildHparamSpec, buildMetricSpec, @@ -21,25 +21,26 @@ import { buildAppRoutingState, buildStateFromAppRoutingState, } from '../../../app_routing/store/testing'; -import {buildRoute} from '../../../app_routing/testing'; -import {buildExperiment} from '../../../experiments/store/testing'; -import {IntervalFilter, DiscreteFilter} from '../../../hparams/types'; -import {DomainType, Run} from '../../../runs/store/runs_types'; +import { buildRoute } from '../../../app_routing/testing'; +import { buildExperiment } from '../../../experiments/store/testing'; +import { IntervalFilter, DiscreteFilter } from '../../../hparams/types'; +import { DomainType, Run } from '../../../runs/store/runs_types'; +import { getRunsTableHeaders } from '../../../runs/store/runs_selectors'; import { buildRun, buildRunsState, buildStateFromRunsState, } from '../../../runs/store/testing'; -import {RunTableItem} from '../../../runs/views/runs_table/types'; -import {buildMockState} from '../../../testing/utils'; +import { RunTableItem } from '../../../runs/views/runs_table/types'; +import { buildMockState } from '../../../testing/utils'; import { appStateFromMetricsState, buildMetricsSettingsState, buildMetricsState, } from '../../testing'; -import {PluginType} from '../../types'; +import { PluginType } from '../../types'; import * as selectors from './common_selectors'; -import {ColumnHeaderType} from '../card_renderer/scalar_card_types'; +import { ColumnHeaderType } from '../card_renderer/scalar_card_types'; describe('common selectors', () => { let runIds: Record; @@ -55,7 +56,7 @@ describe('common selectors', () => { let state: ReturnType; beforeEach(() => { - runIds = {defaultExperimentId: ['run1', 'run2', 'run3']}; + runIds = { defaultExperimentId: ['run1', 'run2', 'run3'] }; runIdToExpId = { run1: 'defaultExperimentId', run2: 'defaultExperimentId', @@ -142,10 +143,10 @@ describe('common selectors', () => { }, ]; - run1 = buildRun({name: 'run 1'}); - run2 = buildRun({id: '2', name: 'run 2'}); - run3 = buildRun({id: '3', name: 'run 3'}); - run4 = buildRun({id: '4', name: 'run 4'}); + run1 = buildRun({ name: 'run 1' }); + run2 = buildRun({ id: '2', name: 'run 2' }); + run3 = buildRun({ id: '3', name: 'run 3' }); + run4 = buildRun({ id: '4', name: 'run 4' }); state = buildMockState({ runs: { data: { @@ -161,13 +162,41 @@ describe('common selectors', () => { run4, }, } as any, - ui: {} as any, + ui: { + runsTableHeaders: [ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + sortable: true, + removable: false, + movable: false, + filterable: false, + hidable: false, + }, + { + type: ColumnHeaderType.CUSTOM, + name: 'experimentAlias', + displayName: 'Experiment', + enabled: true, + movable: false, + sortable: true, + }, + { + type: ColumnHeaderType.CUSTOM, + name: 'fakeRunsHeader', + displayName: 'Fake Runs Header', + enabled: true, + }, + ] + } as any, }, experiments: { data: { experimentMap: { - exp1: buildExperiment({name: 'experiment1', id: 'exp1'}), - exp2: buildExperiment({name: 'experiment2', id: 'exp2'}), + exp1: buildExperiment({ name: 'experiment1', id: 'exp1' }), + exp2: buildExperiment({ name: 'experiment2', id: 'exp2' }), }, }, }, @@ -182,10 +211,35 @@ describe('common selectors', () => { }, hparams: { dashboardSpecs: { - hparams: [buildHparamSpec({name: 'foo', displayName: 'Foo'})], - metrics: [buildMetricSpec({displayName: 'Bar'})], + hparams: [ + buildHparamSpec({ name: 'conv_layers', displayName: 'Conv Layers' }), + buildHparamSpec({ + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + }), + buildHparamSpec({ + name: 'dense_layers', + displayName: 'Dense Layers', + }), + buildHparamSpec({ name: 'dropout', displayName: 'Dropout' }), + ], + metrics: [buildMetricSpec({ displayName: 'Bar' })], }, dashboardSessionGroups: [], + dashboardDisplayedHparamColumns: [ + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'dense_layers', + displayName: 'Dense Layers', + enabled: true, + }, + ], } as any, }); }); @@ -715,11 +769,11 @@ describe('common selectors', () => { state.app_routing!.activeRoute!.routeKind = RouteKind.COMPARE_EXPERIMENT; const results = selectors.TEST_ONLY.getRenderableRuns(state); expect(results.length).toEqual(5); - expect(results[0].run).toEqual({...run1, experimentId: 'exp1'}); - expect(results[1].run).toEqual({...run2, experimentId: 'exp1'}); - expect(results[2].run).toEqual({...run2, experimentId: 'exp2'}); - expect(results[3].run).toEqual({...run3, experimentId: 'exp2'}); - expect(results[4].run).toEqual({...run4, experimentId: 'exp2'}); + expect(results[0].run).toEqual({ ...run1, experimentId: 'exp1' }); + expect(results[1].run).toEqual({ ...run2, experimentId: 'exp1' }); + expect(results[2].run).toEqual({ ...run2, experimentId: 'exp2' }); + expect(results[3].run).toEqual({ ...run3, experimentId: 'exp2' }); + expect(results[4].run).toEqual({ ...run4, experimentId: 'exp2' }); }); it('returns empty list when route does not contain experiments', () => { @@ -911,7 +965,7 @@ describe('common selectors', () => { state.runs!.data.regexFilter = 'foo'; state.app_routing!.activeRoute = { routeKind: RouteKind.EXPERIMENT, - params: {experimentIds: 'exp1'}, + params: { experimentIds: 'exp1' }, }; const result = selectors.getFilteredRenderableRuns(state); expect(result).toEqual([]); @@ -933,7 +987,7 @@ describe('common selectors', () => { ).and.callThrough(); state.app_routing!.activeRoute = { routeKind: RouteKind.EXPERIMENT, - params: {experimentIds: 'exp1'}, + params: { experimentIds: 'exp1' }, }; const results = selectors.getFilteredRenderableRuns(state); expect(spy).toHaveBeenCalledOnceWith(results, new Map(), new Map()); @@ -962,6 +1016,15 @@ describe('common selectors', () => { }); describe('getPotentialHparamColumns', () => { + const expectedBooleanFlags = { + enabled: false, + removable: true, + sortable: true, + movable: true, + filterable: true, + hidable: true, + }; + it('returns empty list when there are no experiments', () => { state.app_routing!.activeRoute!.routeKind = RouteKind.EXPERIMENTS; @@ -972,42 +1035,121 @@ describe('common selectors', () => { expect(selectors.getPotentialHparamColumns(state)).toEqual([ { type: ColumnHeaderType.HPARAM, - name: 'foo', - displayName: 'Foo', - enabled: false, - removable: true, - sortable: true, - movable: true, - filterable: true, + name: 'conv_layers', + displayName: 'Conv Layers', + ...expectedBooleanFlags, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + ...expectedBooleanFlags, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'dense_layers', + displayName: 'Dense Layers', + ...expectedBooleanFlags, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'dropout', + displayName: 'Dropout', + ...expectedBooleanFlags, }, ]); }); it('sets name as display name when a display name is not provided', () => { - state.hparams!.dashboardSpecs.hparams.push( - buildHparamSpec({name: 'bar', displayName: ''}) - ); + state.hparams!.dashboardSpecs.hparams = [ + buildHparamSpec({ name: 'conv_layers', displayName: '' }), + ]; + expect(selectors.getPotentialHparamColumns(state)).toEqual([ { type: ColumnHeaderType.HPARAM, - name: 'foo', - displayName: 'Foo', - enabled: false, - removable: true, - sortable: true, - movable: true, - filterable: true, + name: 'conv_layers', + displayName: 'conv_layers', + ...expectedBooleanFlags, }, - { + ]); + }); + }); + + describe('getSelectableColumns', () => { + it('returns the full list of hparam columns if none are currently displayed', () => { + state.hparams!.dashboardDisplayedHparamColumns = []; + + expect(selectors.getSelectableColumns(state)).toEqual([ + jasmine.objectContaining({ type: ColumnHeaderType.HPARAM, - name: 'bar', - displayName: 'bar', - enabled: false, - removable: true, - sortable: true, - movable: true, - filterable: true, - }, + name: 'conv_layers', + displayName: 'Conv Layers', + }), + jasmine.objectContaining({ + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + }), + jasmine.objectContaining({ + type: ColumnHeaderType.HPARAM, + name: 'dense_layers', + displayName: 'Dense Layers', + }), + jasmine.objectContaining({ + type: ColumnHeaderType.HPARAM, + name: 'dropout', + displayName: 'Dropout', + }), + ]); + }); + + it('returns only columns that are not displayed', () => { + expect(selectors.getSelectableColumns(state)).toEqual([ + jasmine.objectContaining({ + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + }), + jasmine.objectContaining({ + type: ColumnHeaderType.HPARAM, + name: 'dropout', + displayName: 'Dropout', + }), + ]); + }); + }); + + describe('getGroupedColumns', () => { + it('returns a grouped list of columns given a list of standard columns', () => { + expect(selectors.getGroupedColumns(getRunsTableHeaders)(state)).toEqual([ + jasmine.objectContaining({ + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + }), + jasmine.objectContaining({ + type: ColumnHeaderType.CUSTOM, + name: 'experimentAlias', + displayName: 'Experiment', + }), + jasmine.objectContaining({ + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }), + jasmine.objectContaining({ + type: ColumnHeaderType.HPARAM, + name: 'dense_layers', + displayName: 'Dense Layers', + enabled: true, + }), + jasmine.objectContaining({ + type: ColumnHeaderType.CUSTOM, + name: 'fakeRunsHeader', + displayName: 'Fake Runs Header', + }), ]); }); }); From bed9604fe0116bab83f5bfda59bbf049a7517d33 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Wed, 24 Jan 2024 21:01:50 +0900 Subject: [PATCH 02/22] Fixes test description --- tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts b/tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts index fcdc2e5a6e..d57be572bb 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts @@ -141,7 +141,7 @@ describe('hparams/_redux/hparams_selectors_test', () => { expect(selectors.getDashboardDisplayedHparamColumns(state)).toEqual([]); }); - it('returns only dashboard displayed hparam columns', () => { + it('returns only hparam columns that have specs', () => { const state = buildStateFromHparamsState( buildHparamsState({ dashboardSpecs: { From 9eef73499564ae35a23fe70d15d90a4715ed037b Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Wed, 24 Jan 2024 21:04:59 +0900 Subject: [PATCH 03/22] Fixes formatting --- .../hparams/_redux/hparams_selectors.ts | 6 +- .../views/main_view/common_selectors.ts | 45 ++++++++------ .../views/main_view/common_selectors_test.ts | 58 +++++++++---------- 3 files changed, 59 insertions(+), 50 deletions(-) diff --git a/tensorboard/webapp/hparams/_redux/hparams_selectors.ts b/tensorboard/webapp/hparams/_redux/hparams_selectors.ts index b5312c2cf9..bd6054e3a5 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_selectors.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_selectors.ts @@ -50,8 +50,10 @@ export const getDashboardDisplayedHparamColumns = createSelector( getDashboardHparamsAndMetricsSpecs, getHparamsState, ({hparams}, state) => { - const hparamSet = new Set(hparams.map(hparam => hparam.name)); - return state.dashboardDisplayedHparamColumns.filter(column => hparamSet.has(column.name)); + const hparamSet = new Set(hparams.map((hparam) => hparam.name)); + return state.dashboardDisplayedHparamColumns.filter((column) => + hparamSet.has(column.name) + ); } ); diff --git a/tensorboard/webapp/metrics/views/main_view/common_selectors.ts b/tensorboard/webapp/metrics/views/main_view/common_selectors.ts index c709fefad3..76354a4ad9 100644 --- a/tensorboard/webapp/metrics/views/main_view/common_selectors.ts +++ b/tensorboard/webapp/metrics/views/main_view/common_selectors.ts @@ -12,8 +12,8 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ -import { createSelector, Selector } from '@ngrx/store'; -import { State } from '../../../app_state'; +import {createSelector, Selector} from '@ngrx/store'; +import {State} from '../../../app_state'; import { getCurrentRouteRunSelection, getMetricsHideEmptyCards, @@ -27,7 +27,7 @@ import { getColumnHeadersForCard, getDashboardExperimentNames, } from '../../../selectors'; -import { DeepReadonly } from '../../../util/types'; +import {DeepReadonly} from '../../../util/types'; import { getDashboardMetricsFilterMap, getDashboardHparamsAndMetricsSpecs, @@ -45,14 +45,14 @@ import { RunTableItem, RunTableExperimentItem, } from '../../../runs/views/runs_table/types'; -import { getRunsTableHeaders } from '../../../runs/store/runs_selectors'; -import { matchRunToRegex } from '../../../util/matcher'; -import { isSingleRunPlugin, PluginType } from '../../data_source'; -import { getNonEmptyCardIdsWithMetadata, TagMetadata } from '../../store'; -import { compareTagNames } from '../../utils'; -import { CardIdWithMetadata } from '../metrics_view_types'; -import { RouteKind } from '../../../app_routing/types'; -import { memoize } from '../../../util/memoize'; +import {getRunsTableHeaders} from '../../../runs/store/runs_selectors'; +import {matchRunToRegex} from '../../../util/matcher'; +import {isSingleRunPlugin, PluginType} from '../../data_source'; +import {getNonEmptyCardIdsWithMetadata, TagMetadata} from '../../store'; +import {compareTagNames} from '../../utils'; +import {CardIdWithMetadata} from '../metrics_view_types'; +import {RouteKind} from '../../../app_routing/types'; +import {memoize} from '../../../util/memoize'; import { ColumnHeader, ColumnHeaderType, @@ -168,7 +168,7 @@ const utils = { hparamFilters: Map, metricFilters: Map ) { - return runItems.filter(({ hparams, metrics }) => { + return runItems.filter(({hparams, metrics}) => { const hparamMatches = [...hparamFilters.entries()].every( ([hparamName, filter]) => { const value = hparams.get(hparamName); @@ -266,14 +266,14 @@ export const getFilteredRenderableRuns = createSelector( export const getFilteredRenderableRunsIds = createSelector( getFilteredRenderableRuns, (filteredRenderableRuns) => { - return new Set(filteredRenderableRuns.map(({ run: { id } }) => id)); + return new Set(filteredRenderableRuns.map(({run: {id}}) => id)); } ); export const getPotentialHparamColumns = createSelector( getDashboardHparamsAndMetricsSpecs, getExperimentIdsFromRoute, - ({ hparams }, experimentIds): ColumnHeader[] => { + ({hparams}, experimentIds): ColumnHeader[] => { if (!experimentIds) { return []; } @@ -298,7 +298,7 @@ export const getSelectableColumns = createSelector( getPotentialHparamColumns, getDashboardDisplayedHparamColumns, (potentialColumns, currentColumns) => { - const currentColumnNames = new Set(currentColumns.map(({ name }) => name)); + const currentColumnNames = new Set(currentColumns.map(({name}) => name)); return potentialColumns.filter((columnHeader) => { return !currentColumnNames.has(columnHeader.name); }); @@ -306,9 +306,9 @@ export const getSelectableColumns = createSelector( ); /** Returns a list of columns that have been sorted into logical groups. - * + * * Column order: | RUN | experimentAlias | HPARAMs | other | -*/ + */ export const getGroupedColumns = ( headersSelector: Selector ) => @@ -318,9 +318,16 @@ export const getGroupedColumns = ( (tableHeaders, hparamHeaders): ColumnHeader[] => { return [ ...tableHeaders.filter((header) => header.type === 'RUN'), - ...tableHeaders.filter((header) => header.type === 'CUSTOM' && header.name === 'experimentAlias'), + ...tableHeaders.filter( + (header) => + header.type === 'CUSTOM' && header.name === 'experimentAlias' + ), ...hparamHeaders, - ...tableHeaders.filter((header) => header.type !== 'RUN' && !(header.type === 'CUSTOM' && header.name === 'experimentAlias')), + ...tableHeaders.filter( + (header) => + header.type !== 'RUN' && + !(header.type === 'CUSTOM' && header.name === 'experimentAlias') + ), ]; } ); diff --git a/tensorboard/webapp/metrics/views/main_view/common_selectors_test.ts b/tensorboard/webapp/metrics/views/main_view/common_selectors_test.ts index f8838684fb..6667e7beea 100644 --- a/tensorboard/webapp/metrics/views/main_view/common_selectors_test.ts +++ b/tensorboard/webapp/metrics/views/main_view/common_selectors_test.ts @@ -12,7 +12,7 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ -import { RouteKind } from '../../../app_routing'; +import {RouteKind} from '../../../app_routing'; import { buildHparamSpec, buildMetricSpec, @@ -21,26 +21,26 @@ import { buildAppRoutingState, buildStateFromAppRoutingState, } from '../../../app_routing/store/testing'; -import { buildRoute } from '../../../app_routing/testing'; -import { buildExperiment } from '../../../experiments/store/testing'; -import { IntervalFilter, DiscreteFilter } from '../../../hparams/types'; -import { DomainType, Run } from '../../../runs/store/runs_types'; -import { getRunsTableHeaders } from '../../../runs/store/runs_selectors'; +import {buildRoute} from '../../../app_routing/testing'; +import {buildExperiment} from '../../../experiments/store/testing'; +import {IntervalFilter, DiscreteFilter} from '../../../hparams/types'; +import {DomainType, Run} from '../../../runs/store/runs_types'; +import {getRunsTableHeaders} from '../../../runs/store/runs_selectors'; import { buildRun, buildRunsState, buildStateFromRunsState, } from '../../../runs/store/testing'; -import { RunTableItem } from '../../../runs/views/runs_table/types'; -import { buildMockState } from '../../../testing/utils'; +import {RunTableItem} from '../../../runs/views/runs_table/types'; +import {buildMockState} from '../../../testing/utils'; import { appStateFromMetricsState, buildMetricsSettingsState, buildMetricsState, } from '../../testing'; -import { PluginType } from '../../types'; +import {PluginType} from '../../types'; import * as selectors from './common_selectors'; -import { ColumnHeaderType } from '../card_renderer/scalar_card_types'; +import {ColumnHeaderType} from '../card_renderer/scalar_card_types'; describe('common selectors', () => { let runIds: Record; @@ -56,7 +56,7 @@ describe('common selectors', () => { let state: ReturnType; beforeEach(() => { - runIds = { defaultExperimentId: ['run1', 'run2', 'run3'] }; + runIds = {defaultExperimentId: ['run1', 'run2', 'run3']}; runIdToExpId = { run1: 'defaultExperimentId', run2: 'defaultExperimentId', @@ -143,10 +143,10 @@ describe('common selectors', () => { }, ]; - run1 = buildRun({ name: 'run 1' }); - run2 = buildRun({ id: '2', name: 'run 2' }); - run3 = buildRun({ id: '3', name: 'run 3' }); - run4 = buildRun({ id: '4', name: 'run 4' }); + run1 = buildRun({name: 'run 1'}); + run2 = buildRun({id: '2', name: 'run 2'}); + run3 = buildRun({id: '3', name: 'run 3'}); + run4 = buildRun({id: '4', name: 'run 4'}); state = buildMockState({ runs: { data: { @@ -189,14 +189,14 @@ describe('common selectors', () => { displayName: 'Fake Runs Header', enabled: true, }, - ] + ], } as any, }, experiments: { data: { experimentMap: { - exp1: buildExperiment({ name: 'experiment1', id: 'exp1' }), - exp2: buildExperiment({ name: 'experiment2', id: 'exp2' }), + exp1: buildExperiment({name: 'experiment1', id: 'exp1'}), + exp2: buildExperiment({name: 'experiment2', id: 'exp2'}), }, }, }, @@ -212,7 +212,7 @@ describe('common selectors', () => { hparams: { dashboardSpecs: { hparams: [ - buildHparamSpec({ name: 'conv_layers', displayName: 'Conv Layers' }), + buildHparamSpec({name: 'conv_layers', displayName: 'Conv Layers'}), buildHparamSpec({ name: 'conv_kernel_size', displayName: 'Conv Kernel Size', @@ -221,9 +221,9 @@ describe('common selectors', () => { name: 'dense_layers', displayName: 'Dense Layers', }), - buildHparamSpec({ name: 'dropout', displayName: 'Dropout' }), + buildHparamSpec({name: 'dropout', displayName: 'Dropout'}), ], - metrics: [buildMetricSpec({ displayName: 'Bar' })], + metrics: [buildMetricSpec({displayName: 'Bar'})], }, dashboardSessionGroups: [], dashboardDisplayedHparamColumns: [ @@ -769,11 +769,11 @@ describe('common selectors', () => { state.app_routing!.activeRoute!.routeKind = RouteKind.COMPARE_EXPERIMENT; const results = selectors.TEST_ONLY.getRenderableRuns(state); expect(results.length).toEqual(5); - expect(results[0].run).toEqual({ ...run1, experimentId: 'exp1' }); - expect(results[1].run).toEqual({ ...run2, experimentId: 'exp1' }); - expect(results[2].run).toEqual({ ...run2, experimentId: 'exp2' }); - expect(results[3].run).toEqual({ ...run3, experimentId: 'exp2' }); - expect(results[4].run).toEqual({ ...run4, experimentId: 'exp2' }); + expect(results[0].run).toEqual({...run1, experimentId: 'exp1'}); + expect(results[1].run).toEqual({...run2, experimentId: 'exp1'}); + expect(results[2].run).toEqual({...run2, experimentId: 'exp2'}); + expect(results[3].run).toEqual({...run3, experimentId: 'exp2'}); + expect(results[4].run).toEqual({...run4, experimentId: 'exp2'}); }); it('returns empty list when route does not contain experiments', () => { @@ -965,7 +965,7 @@ describe('common selectors', () => { state.runs!.data.regexFilter = 'foo'; state.app_routing!.activeRoute = { routeKind: RouteKind.EXPERIMENT, - params: { experimentIds: 'exp1' }, + params: {experimentIds: 'exp1'}, }; const result = selectors.getFilteredRenderableRuns(state); expect(result).toEqual([]); @@ -987,7 +987,7 @@ describe('common selectors', () => { ).and.callThrough(); state.app_routing!.activeRoute = { routeKind: RouteKind.EXPERIMENT, - params: { experimentIds: 'exp1' }, + params: {experimentIds: 'exp1'}, }; const results = selectors.getFilteredRenderableRuns(state); expect(spy).toHaveBeenCalledOnceWith(results, new Map(), new Map()); @@ -1062,7 +1062,7 @@ describe('common selectors', () => { it('sets name as display name when a display name is not provided', () => { state.hparams!.dashboardSpecs.hparams = [ - buildHparamSpec({ name: 'conv_layers', displayName: '' }), + buildHparamSpec({name: 'conv_layers', displayName: ''}), ]; expect(selectors.getPotentialHparamColumns(state)).toEqual([ From f72b2a2d45efa8f9c8dfbce7b1fae2fd88c68a4a Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Thu, 25 Jan 2024 18:29:01 +0900 Subject: [PATCH 04/22] Changes reorder column events to use source and destination --- tensorboard/webapp/hparams/_redux/BUILD | 2 + .../webapp/hparams/_redux/hparams_reducers.ts | 31 +- .../hparams/_redux/hparams_reducers_test.ts | 107 +----- tensorboard/webapp/metrics/internal_types.ts | 7 +- tensorboard/webapp/metrics/store/BUILD | 1 + .../webapp/metrics/store/metrics_reducers.ts | 102 ++---- .../metrics/store/metrics_reducers_test.ts | 330 ++---------------- .../webapp/metrics/views/card_renderer/BUILD | 2 + .../scalar_card_component.ng.html | 1 - .../card_renderer/scalar_card_component.ts | 8 +- .../card_renderer/scalar_card_container.ts | 31 +- .../scalar_card_data_table.ng.html | 4 +- .../card_renderer/scalar_card_data_table.ts | 20 +- .../views/card_renderer/scalar_card_test.ts | 123 ++----- .../right_pane/scalar_column_editor/BUILD | 2 + .../scalar_column_editor_component.ng.html | 8 +- .../scalar_column_editor_component.ts | 51 ++- .../scalar_column_editor_container.ts | 33 +- .../scalar_column_editor_test.ts | 175 ++++++---- .../persistent_settings_data_source.ts | 6 +- .../persistent_settings_data_source_test.ts | 42 +++ .../runs/views/runs_table/runs_data_table.ts | 3 +- tensorboard/webapp/widgets/data_table/BUILD | 23 ++ .../data_table/data_table_component.ts | 33 +- .../widgets/data_table/data_table_test.ts | 39 +-- .../webapp/widgets/data_table/types.ts | 2 +- .../webapp/widgets/data_table/utils.ts | 54 +++ .../webapp/widgets/data_table/utils_test.ts | 142 ++++++++ 28 files changed, 601 insertions(+), 781 deletions(-) create mode 100644 tensorboard/webapp/widgets/data_table/utils.ts create mode 100644 tensorboard/webapp/widgets/data_table/utils_test.ts diff --git a/tensorboard/webapp/hparams/_redux/BUILD b/tensorboard/webapp/hparams/_redux/BUILD index 2b3608bd5b..15a565d974 100644 --- a/tensorboard/webapp/hparams/_redux/BUILD +++ b/tensorboard/webapp/hparams/_redux/BUILD @@ -67,6 +67,7 @@ tf_ts_library( "//tensorboard/webapp/hparams:_types", "//tensorboard/webapp/runs/actions", "//tensorboard/webapp/widgets/data_table:types", + "//tensorboard/webapp/widgets/data_table:utils", "@npm//@ngrx/store", ], ) @@ -174,6 +175,7 @@ tf_ts_library( "//tensorboard/webapp/util:types", "//tensorboard/webapp/webapp_data_source:http_client_testing", "//tensorboard/webapp/widgets/data_table:types", + "//tensorboard/webapp/widgets/data_table:utils", "@npm//@ngrx/effects", "@npm//@ngrx/store", "@npm//@types/jasmine", diff --git a/tensorboard/webapp/hparams/_redux/hparams_reducers.ts b/tensorboard/webapp/hparams/_redux/hparams_reducers.ts index 883de392c6..d152199bac 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_reducers.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_reducers.ts @@ -13,7 +13,10 @@ See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ import {Action, ActionReducer, createReducer, on} from '@ngrx/store'; -import {ColumnHeader, Side} from '../../widgets/data_table/types'; +import { + Side, +} from '../../widgets/data_table/types'; +import {DataTableUtils} from '../../widgets/data_table/utils'; import * as actions from './hparams_actions'; import {HparamsState} from './types'; @@ -141,28 +144,12 @@ const reducer: ActionReducer = createReducer( actions.dashboardHparamColumnOrderChanged, (state, {source, destination, side}) => { const {dashboardDisplayedHparamColumns: columns} = state; - const sourceIndex = columns.findIndex( - (column: ColumnHeader) => column.name === source.name + const newColumns = DataTableUtils.moveColumn( + columns, + source, + destination, + side ); - let destinationIndex = columns.findIndex( - (column: ColumnHeader) => column.name === destination.name - ); - if (sourceIndex === -1 || sourceIndex === destinationIndex) { - return state; - } - if (destinationIndex === -1) { - // Use side as a backup to determine source position if destination isn't found. - if (side !== undefined) { - destinationIndex = side === Side.LEFT ? 0 : columns.length - 1; - } else { - return state; - } - } - - const newColumns = [...columns]; - newColumns.splice(sourceIndex, 1); - newColumns.splice(destinationIndex, 0, source); - return { ...state, dashboardDisplayedHparamColumns: newColumns, diff --git a/tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts b/tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts index d671f762dc..895b8f6170 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts @@ -18,6 +18,7 @@ import * as actions from './hparams_actions'; import {reducers} from './hparams_reducers'; import {buildHparamSpec, buildHparamsState, buildMetricSpec} from './testing'; import {ColumnHeaderType, Side} from '../../widgets/data_table/types'; +import {DataTableUtils} from '../../widgets/data_table/utils'; describe('hparams/_redux/hparams_reducers_test', () => { describe('hparamsFetchSessionGroupsSucceeded', () => { @@ -594,104 +595,15 @@ describe('hparams/_redux/hparams_reducers_test', () => { }, ]; - it('does nothing if source is not found', () => { + it('moves source to destination using moveColumn', () => { const state = buildHparamsState({ dashboardDisplayedHparamColumns: fakeColumns, }); - const state2 = reducers( - state, - actions.dashboardHparamColumnOrderChanged({ - source: { - type: ColumnHeaderType.HPARAM, - name: 'nonexistent_param', - displayName: 'Nonexistent param', - enabled: false, - }, - destination: { - type: ColumnHeaderType.HPARAM, - name: 'conv_kernel_size', - displayName: 'Conv Kernel Size', - enabled: true, - }, - side: Side.LEFT, - }) - ); - - expect(state2.dashboardDisplayedHparamColumns).toEqual(fakeColumns); - }); - - it('does nothing if source equals dest', () => { - const state = buildHparamsState({ - dashboardDisplayedHparamColumns: fakeColumns, - }); - const state2 = reducers( - state, - actions.dashboardHparamColumnOrderChanged({ - source: { - type: ColumnHeaderType.HPARAM, - name: 'conv_kernel_size', - displayName: 'Conv Kernel Size', - enabled: false, - }, - destination: { - type: ColumnHeaderType.HPARAM, - name: 'conv_kernel_size', - displayName: 'Conv Kernel Size', - enabled: true, - }, - side: Side.LEFT, - }) - ); - - expect(state2.dashboardDisplayedHparamColumns).toEqual(fakeColumns); - }); + const moveColumnSpy = spyOn( + DataTableUtils, + 'moveColumn' + ).and.callThrough(); - [ - { - testDesc: 'to front if side is left', - side: Side.LEFT, - expectedResult: [ - fakeColumns[1], - fakeColumns[0], - ...fakeColumns.slice(2), - ], - }, - { - testDesc: 'to back if side is right', - side: Side.RIGHT, - expectedResult: [ - fakeColumns[0], - ...fakeColumns.slice(2), - fakeColumns[1], - ], - }, - ].forEach(({testDesc, side, expectedResult}) => { - it(`if destination not found, moves source ${testDesc}`, () => { - const state = buildHparamsState({ - dashboardDisplayedHparamColumns: fakeColumns, - }); - const state2 = reducers( - state, - actions.dashboardHparamColumnOrderChanged({ - source: fakeColumns[1], - destination: { - type: ColumnHeaderType.HPARAM, - name: 'nonexistent param', - displayName: 'Nonexistent param', - enabled: true, - }, - side, - }) - ); - - expect(state2.dashboardDisplayedHparamColumns).toEqual(expectedResult); - }); - }); - - it('swaps source and destination positions if destination is found', () => { - const state = buildHparamsState({ - dashboardDisplayedHparamColumns: fakeColumns, - }); const state2 = reducers( state, actions.dashboardHparamColumnOrderChanged({ @@ -701,6 +613,13 @@ describe('hparams/_redux/hparams_reducers_test', () => { }) ); + // Edge cases are tested by moveColumn tests. + expect(moveColumnSpy).toHaveBeenCalledWith( + fakeColumns, + fakeColumns[1], + fakeColumns[0], + Side.LEFT + ); expect(state2.dashboardDisplayedHparamColumns).toEqual([ fakeColumns[1], fakeColumns[0], diff --git a/tensorboard/webapp/metrics/internal_types.ts b/tensorboard/webapp/metrics/internal_types.ts index d73a6c12e3..c664baa7e4 100644 --- a/tensorboard/webapp/metrics/internal_types.ts +++ b/tensorboard/webapp/metrics/internal_types.ts @@ -16,8 +16,8 @@ import {TimeSelection} from '../widgets/card_fob/card_fob_types'; import {HistogramMode} from '../widgets/histogram/histogram_types'; import { ColumnHeader, - ColumnHeaderType, DataTableMode, + ReorderColumnEvent, } from '../widgets/data_table/types'; export {HistogramMode, TimeSelection}; @@ -94,15 +94,14 @@ export interface URLDeserializedState { }; } -export interface HeaderEditInfo { +export interface HeaderEditInfo extends ReorderColumnEvent { dataTableMode: DataTableMode; - headers: ColumnHeader[]; } export interface HeaderToggleInfo { header: ColumnHeader; cardId?: CardId; - dataTableMode?: DataTableMode; + dataTableMode?: DataTableMode | undefined; } export const SCALARS_SMOOTHING_MIN = 0; diff --git a/tensorboard/webapp/metrics/store/BUILD b/tensorboard/webapp/metrics/store/BUILD index 8db5c763f9..88f8bce1fe 100644 --- a/tensorboard/webapp/metrics/store/BUILD +++ b/tensorboard/webapp/metrics/store/BUILD @@ -33,6 +33,7 @@ tf_ts_library( "//tensorboard/webapp/util:types", "//tensorboard/webapp/widgets/card_fob:types", "//tensorboard/webapp/widgets/data_table:types", + "//tensorboard/webapp/widgets/data_table:utils", "//tensorboard/webapp/widgets/line_chart_v2/lib:public_types", "@npm//@ngrx/store", ], diff --git a/tensorboard/webapp/metrics/store/metrics_reducers.ts b/tensorboard/webapp/metrics/store/metrics_reducers.ts index d2f899c2c2..db43c38914 100644 --- a/tensorboard/webapp/metrics/store/metrics_reducers.ts +++ b/tensorboard/webapp/metrics/store/metrics_reducers.ts @@ -46,11 +46,7 @@ import { URLDeserializedState, } from '../types'; import {groupCardIdWithMetdata} from '../utils'; -import { - ColumnHeader, - ColumnHeaderType, - DataTableMode, -} from '../../widgets/data_table/types'; +import {ColumnHeaderType, DataTableMode} from '../../widgets/data_table/types'; import { buildOrReturnStateWithPinnedCopy, buildOrReturnStateWithUnresolvedImportedPins, @@ -82,6 +78,7 @@ import { TimeSeriesData, TimeSeriesLoadable, } from './metrics_types'; +import {DataTableUtils} from '../../widgets/data_table/utils'; function buildCardMetadataList(tagMetadata: TagMetadata): CardMetadata[] { const results: CardMetadata[] = []; @@ -1434,34 +1431,30 @@ const reducer = createReducer( tableEditorSelectedTab: tab, }; }), - on(actions.dataTableColumnEdited, (state, {dataTableMode, headers}) => { - const enabledNewHeaders: ColumnHeader[] = []; - const disabledNewHeaders: ColumnHeader[] = []; - - // All enabled headers appear above all disabled headers. - headers.forEach((header) => { - if (header.enabled) { - enabledNewHeaders.push(header); - } else { - disabledNewHeaders.push(header); + on( + actions.dataTableColumnEdited, + (state, {source, destination, side, dataTableMode}) => { + let headers = + dataTableMode === DataTableMode.RANGE + ? [...state.rangeSelectionHeaders] + : [...state.singleSelectionHeaders]; + headers = DataTableUtils.moveColumn(headers, source, destination, side); + + if (dataTableMode === DataTableMode.RANGE) { + return { + ...state, + rangeSelectionHeaders: headers, + }; } - }); - - if (dataTableMode === DataTableMode.RANGE) { return { ...state, - rangeSelectionHeaders: enabledNewHeaders.concat(disabledNewHeaders), + singleSelectionHeaders: headers, }; } - - return { - ...state, - singleSelectionHeaders: enabledNewHeaders.concat(disabledNewHeaders), - }; - }), + ), on( actions.dataTableColumnToggled, - (state, {dataTableMode, header, cardId}) => { + (state, {dataTableMode, header: toggledHeader, cardId}) => { const {cardStateMap, rangeSelectionEnabled, linkedTimeEnabled} = state; const rangeEnabled = cardId ? cardRangeSelectionEnabled( @@ -1471,32 +1464,17 @@ const reducer = createReducer( cardId ) : dataTableMode === DataTableMode.RANGE; - const targetedHeaders = rangeEnabled ? state.rangeSelectionHeaders : state.singleSelectionHeaders; - const currentToggledHeaderIndex = targetedHeaders.findIndex( - (element) => element.name === header.name - ); - - // If the header is being enabled it goes at the bottom of the currently - // enabled headers. If it is being disabled it goes to the top of the - // currently disabled headers. - let newToggledHeaderIndex = getEnabledCount(targetedHeaders); - if (targetedHeaders[currentToggledHeaderIndex].enabled) { - newToggledHeaderIndex--; - } - const newHeaders = moveHeader( - currentToggledHeaderIndex, - newToggledHeaderIndex, - targetedHeaders - ); - - newHeaders[newToggledHeaderIndex] = { - ...newHeaders[newToggledHeaderIndex], - enabled: !newHeaders[newToggledHeaderIndex].enabled, - }; + const newHeaders = targetedHeaders.map((header) => { + const newHeader = {...header}; + if (header.name === toggledHeader.name) { + newHeader.enabled = !newHeader.enabled; + } + return newHeader; + }); if (rangeEnabled) { return { @@ -1504,7 +1482,6 @@ const reducer = createReducer( rangeSelectionHeaders: newHeaders, }; } - return { ...state, singleSelectionHeaders: newHeaders, @@ -1583,30 +1560,3 @@ function buildTagToRuns(runTagInfo: {[run: string]: string[]}) { } return tagToRuns; } - -/** - * Returns a copy of the headers array with item at sourceIndex moved to - * destinationIndex. - */ -function moveHeader( - sourceIndex: number, - destinationIndex: number, - headers: ColumnHeader[] -) { - const newHeaders = [...headers]; - // Delete from original location - newHeaders.splice(sourceIndex, 1); - // Insert at destinationIndex. - newHeaders.splice(destinationIndex, 0, headers[sourceIndex]); - return newHeaders; -} - -function getEnabledCount(headers: ColumnHeader[]) { - let count = 0; - headers.forEach((header) => { - if (header.enabled) { - count++; - } - }); - return count; -} diff --git a/tensorboard/webapp/metrics/store/metrics_reducers_test.ts b/tensorboard/webapp/metrics/store/metrics_reducers_test.ts index ba42d51e0b..c4538580ca 100644 --- a/tensorboard/webapp/metrics/store/metrics_reducers_test.ts +++ b/tensorboard/webapp/metrics/store/metrics_reducers_test.ts @@ -1602,39 +1602,19 @@ describe('metrics reducers', () => { const nextState = reducers( beforeState, actions.dataTableColumnEdited({ + source: { + type: ColumnHeaderType.END_VALUE, + name: 'endValue', + displayName: 'End Value', + enabled: true, + }, + destination: { + type: ColumnHeaderType.START_VALUE, + name: 'startValue', + displayName: 'Start Value', + enabled: true, + }, dataTableMode: DataTableMode.RANGE, - headers: [ - { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - enabled: true, - }, - { - type: ColumnHeaderType.END_VALUE, - name: 'endValue', - displayName: 'End Value', - enabled: true, - }, - { - type: ColumnHeaderType.START_VALUE, - name: 'startValue', - displayName: 'Start Value', - enabled: true, - }, - { - type: ColumnHeaderType.MIN_VALUE, - name: 'minValue', - displayName: 'Min', - enabled: false, - }, - { - type: ColumnHeaderType.MAX_VALUE, - name: 'maxValue', - displayName: 'Max', - enabled: false, - }, - ], }) ); @@ -1763,33 +1743,19 @@ describe('metrics reducers', () => { const nextState = reducers( beforeState, actions.dataTableColumnEdited({ + source: { + type: ColumnHeaderType.STEP, + name: 'step', + displayName: 'Step', + enabled: true, + }, + destination: { + type: ColumnHeaderType.VALUE, + name: 'value', + displayName: 'Value', + enabled: true, + }, dataTableMode: DataTableMode.SINGLE, - headers: [ - { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - enabled: true, - }, - { - type: ColumnHeaderType.STEP, - name: 'step', - displayName: 'Step', - enabled: true, - }, - { - type: ColumnHeaderType.VALUE, - name: 'value', - displayName: 'Value', - enabled: true, - }, - { - type: ColumnHeaderType.RELATIVE_TIME, - name: 'relativeTime', - displayName: 'Relative', - enabled: false, - }, - ], }) ); @@ -1852,115 +1818,6 @@ describe('metrics reducers', () => { }, ]); }); - - it('ensures ordering keeps enabled headers first', () => { - const beforeState = buildMetricsState({ - rangeSelectionHeaders: [ - { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - enabled: true, - }, - { - type: ColumnHeaderType.START_VALUE, - name: 'startValue', - displayName: 'Start Value', - enabled: true, - }, - { - type: ColumnHeaderType.END_VALUE, - name: 'endValue', - displayName: 'End Value', - enabled: true, - }, - { - type: ColumnHeaderType.MIN_VALUE, - name: 'minValue', - displayName: 'Min', - enabled: false, - }, - { - type: ColumnHeaderType.MAX_VALUE, - name: 'maxValue', - displayName: 'Max', - enabled: false, - }, - ], - }); - - const nextState = reducers( - beforeState, - actions.dataTableColumnEdited({ - dataTableMode: DataTableMode.RANGE, - headers: [ - { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - enabled: true, - }, - { - type: ColumnHeaderType.MAX_VALUE, - name: 'maxValue', - displayName: 'Max', - enabled: false, - }, - { - type: ColumnHeaderType.START_VALUE, - name: 'startValue', - displayName: 'Start Value', - enabled: true, - }, - { - type: ColumnHeaderType.END_VALUE, - name: 'endValue', - displayName: 'End Value', - enabled: true, - }, - { - type: ColumnHeaderType.MIN_VALUE, - name: 'minValue', - displayName: 'Min', - enabled: false, - }, - ], - }) - ); - - expect(nextState.rangeSelectionHeaders).toEqual([ - { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - enabled: true, - }, - { - type: ColumnHeaderType.START_VALUE, - name: 'startValue', - displayName: 'Start Value', - enabled: true, - }, - { - type: ColumnHeaderType.END_VALUE, - name: 'endValue', - displayName: 'End Value', - enabled: true, - }, - { - type: ColumnHeaderType.MAX_VALUE, - name: 'maxValue', - displayName: 'Max', - enabled: false, - }, - { - type: ColumnHeaderType.MIN_VALUE, - name: 'minValue', - displayName: 'Min', - enabled: false, - }, - ]); - }); }); describe('dataTableColumnToggled', () => { @@ -2029,55 +1886,7 @@ describe('metrics reducers', () => { }); }); - it('moves header down to the disabled headers when toggling to disabled with data table mode input', () => { - const nextState = reducers( - beforeState, - actions.dataTableColumnToggled({ - dataTableMode: DataTableMode.RANGE, - header: { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - enabled: false, - }, - }) - ); - - expect(nextState.rangeSelectionHeaders).toEqual([ - { - type: ColumnHeaderType.START_VALUE, - name: 'startValue', - displayName: 'Start Value', - enabled: true, - }, - { - type: ColumnHeaderType.END_VALUE, - name: 'endValue', - displayName: 'End Value', - enabled: true, - }, - { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - enabled: false, - }, - { - type: ColumnHeaderType.MIN_VALUE, - name: 'minValue', - displayName: 'Min', - enabled: false, - }, - { - type: ColumnHeaderType.MAX_VALUE, - name: 'maxValue', - displayName: 'Max', - enabled: false, - }, - ]); - }); - - it('moves header up to the enabled headers when toggling to enabled with data table mode input', () => { + it('only changes range selection headers when dataTableMode is RANGE', () => { const nextState = reducers( beforeState, actions.dataTableColumnToggled({ @@ -2110,66 +1919,18 @@ describe('metrics reducers', () => { displayName: 'End Value', enabled: true, }, - { - type: ColumnHeaderType.MAX_VALUE, - name: 'maxValue', - displayName: 'Max', - enabled: true, - }, { type: ColumnHeaderType.MIN_VALUE, name: 'minValue', displayName: 'Min', enabled: false, }, - ]); - }); - - it('only changes range selection headers when dataTableMode is RANGE', () => { - const nextState = reducers( - beforeState, - actions.dataTableColumnToggled({ - dataTableMode: DataTableMode.RANGE, - header: { - type: ColumnHeaderType.MAX_VALUE, - name: 'maxValue', - displayName: 'Max', - enabled: true, - }, - }) - ); - - expect(nextState.rangeSelectionHeaders).toEqual([ - { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - enabled: true, - }, - { - type: ColumnHeaderType.START_VALUE, - name: 'startValue', - displayName: 'Start Value', - enabled: true, - }, - { - type: ColumnHeaderType.END_VALUE, - name: 'endValue', - displayName: 'End Value', - enabled: true, - }, { type: ColumnHeaderType.MAX_VALUE, name: 'maxValue', displayName: 'Max', enabled: true, }, - { - type: ColumnHeaderType.MIN_VALUE, - name: 'minValue', - displayName: 'Min', - enabled: false, - }, ]); expect(nextState.singleSelectionHeaders).toEqual([ @@ -2275,34 +2036,6 @@ describe('metrics reducers', () => { ]); }); - it('moves header down to the disabled headers when column is removed with card id input', () => { - beforeState = { - ...beforeState, - cardStateMap: { - card1: { - rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, - }, - }, - }; - - const nextState = reducers( - beforeState, - actions.dataTableColumnToggled({ - cardId: 'card1', - header: { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - enabled: true, - }, - }) - ); - - expect( - nextState.rangeSelectionHeaders.map((header) => header.enabled) - ).toEqual([true, true, false, false, false]); - }); - it('only changes range selection headers when given card has rangeSelectionOverride ENABLED', () => { beforeState = { ...beforeState, @@ -2345,20 +2078,19 @@ describe('metrics reducers', () => { displayName: 'End Value', enabled: true, }, - { - type: ColumnHeaderType.MAX_VALUE, - name: 'maxValue', - displayName: 'Max', - enabled: true, - }, { type: ColumnHeaderType.MIN_VALUE, name: 'minValue', displayName: 'Min', enabled: false, }, + { + type: ColumnHeaderType.MAX_VALUE, + name: 'maxValue', + displayName: 'Max', + enabled: true, + }, ]); - expect(nextState.singleSelectionHeaders).toEqual([ { type: ColumnHeaderType.RUN, diff --git a/tensorboard/webapp/metrics/views/card_renderer/BUILD b/tensorboard/webapp/metrics/views/card_renderer/BUILD index 180c83eb0f..d70762c9fe 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/BUILD +++ b/tensorboard/webapp/metrics/views/card_renderer/BUILD @@ -302,6 +302,7 @@ tf_ng_module( ":scalar_card_types", ":utils", "//tensorboard/webapp/metrics:types", + "//tensorboard/webapp/runs:types", "//tensorboard/webapp/widgets/card_fob:types", "//tensorboard/webapp/widgets/data_table", "//tensorboard/webapp/widgets/data_table:types", @@ -347,6 +348,7 @@ tf_ng_module( "//tensorboard/webapp/metrics/views:utils", "//tensorboard/webapp/metrics/views/main_view:common_selectors", "//tensorboard/webapp/runs/store:types", + "//tensorboard/webapp/runs:types", "//tensorboard/webapp/types", "//tensorboard/webapp/types:ui", "//tensorboard/webapp/util:value_formatter", diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html index 11113af833..8d457246da 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html @@ -202,7 +202,6 @@ [hparamsEnabled]="hparamsEnabled" (sortDataBy)="sortDataBy($event)" (editColumnHeaders)="editColumnHeaders.emit($event)" - (removeColumn)="removeColumn.emit($event)" > diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts index 74e2390576..3ec6e070a5 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts @@ -44,7 +44,11 @@ import { TooltipDatum, } from '../../../widgets/line_chart_v2/types'; import {CardState} from '../../store'; -import {HeaderEditInfo, TooltipSort, XAxisType} from '../../types'; +import { + HeaderEditInfo, + TooltipSort, + XAxisType, +} from '../../types'; import { MinMaxStep, ScalarCardDataSeries, @@ -114,10 +118,8 @@ export class ScalarCardComponent { @Output() onDataTableSorting = new EventEmitter(); @Output() editColumnHeaders = new EventEmitter(); @Output() openTableEditMenuToMode = new EventEmitter(); - @Output() removeColumn = new EventEmitter(); @Output() onLineChartZoom = new EventEmitter(); - @Output() onCardStateChanged = new EventEmitter>(); // Line chart may not exist when was never visible (*ngIf). diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts index abbd9d7421..013ca742c7 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts @@ -58,6 +58,7 @@ import { getRunColorMap, getCurrentRouteRunSelection, getColumnHeadersForCard, + getDashboardRunsToHparams, } from '../../../selectors'; import {DataLoadState} from '../../../types/data'; import { @@ -71,7 +72,6 @@ import {ScaleType} from '../../../widgets/line_chart_v2/types'; import { cardViewBoxChanged, dataTableColumnEdited, - dataTableColumnToggled, metricsCardFullSizeToggled, metricsCardStateUpdated, sortingDataTable, @@ -93,13 +93,7 @@ import { getMetricsXAxisType, RunToSeries, } from '../../store'; -import { - CardId, - CardMetadata, - HeaderEditInfo, - HeaderToggleInfo, - XAxisType, -} from '../../types'; +import {CardId, CardMetadata, HeaderEditInfo, XAxisType} from '../../types'; import {getFilteredRenderableRunsIds} from '../main_view/common_selectors'; import {CardRenderer} from '../metrics_view_types'; import {getTagDisplayName} from '../utils'; @@ -150,11 +144,6 @@ function areSeriesEqual( }); } -function isMinMaxStepValid(minMax: MinMaxStep | undefined): boolean { - if (!minMax) return false; - return !(minMax.minStep === -Infinity && minMax.maxStep === Infinity); -} - @Component({ selector: 'scalar-card', template: ` @@ -196,7 +185,6 @@ function isMinMaxStepValid(minMax: MinMaxStep | undefined): boolean { (editColumnHeaders)="editColumnHeaders($event)" (onCardStateChanged)="onCardStateChanged($event)" (openTableEditMenuToMode)="openTableEditMenuToMode($event)" - (removeColumn)="onRemoveColumn($event)" > `, styles: [ @@ -686,15 +674,18 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { ); } - editColumnHeaders(headerEditInfo: HeaderEditInfo) { - this.store.dispatch(dataTableColumnEdited(headerEditInfo)); + editColumnHeaders({ + source, + destination, + side, + dataTableMode, + }: HeaderEditInfo) { + this.store.dispatch( + dataTableColumnEdited({source, destination, side, dataTableMode}) + ); } openTableEditMenuToMode(tableMode: DataTableMode) { this.store.dispatch(metricsSlideoutMenuOpened({mode: tableMode})); } - - onRemoveColumn(header: ColumnHeader) { - this.store.dispatch(dataTableColumnToggled({header, cardId: this.cardId})); - } } diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html index 9f797a6da6..902bdb9d1b 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html @@ -18,10 +18,8 @@ diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts index e73194447a..15e3194bd7 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts @@ -34,6 +34,7 @@ import { TableData, SortingInfo, SortingOrder, + ReorderColumnEvent, } from '../../../widgets/data_table/types'; import {isDatumVisible} from './utils'; @@ -55,9 +56,6 @@ export class ScalarCardDataTable { @Output() sortDataBy = new EventEmitter(); @Output() editColumnHeaders = new EventEmitter(); - @Output() removeColumn = new EventEmitter<{ - headerType: ColumnHeaderType; - }>(); ColumnHeaderType = ColumnHeaderType; @@ -292,12 +290,18 @@ export class ScalarCardDataTable { return makeValueSortable(point[header.name]); } - orderColumns(headers: ColumnHeader[]) { + private getDataTableMode(): DataTableMode { + return this.stepOrLinkedTimeSelection.end + ? DataTableMode.RANGE + : DataTableMode.SINGLE; + } + + onOrderColumns({source, destination, side}: ReorderColumnEvent) { this.editColumnHeaders.emit({ - headers: headers, - dataTableMode: this.stepOrLinkedTimeSelection.end - ? DataTableMode.RANGE - : DataTableMode.SINGLE, + source, + destination, + side, + dataTableMode: this.getDataTableMode(), }); } } diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts index bbe739df71..d761390179 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts @@ -117,13 +117,14 @@ import { ColumnHeader, ColumnHeaderType, DataTableMode, + ReorderColumnEvent, + Side, SortingOrder, } from '../../../widgets/data_table/types'; import {VisLinkedTimeSelectionWarningModule} from './vis_linked_time_selection_warning_module'; import {Extent} from '../../../widgets/line_chart_v2/lib/public_types'; import {provideMockTbStore} from '../../../testing/utils'; import * as commonSelectors from '../main_view/common_selectors'; -import {CardFeatureOverride} from '../../store/metrics_types'; import {ContentCellComponent} from '../../../widgets/data_table/content_cell_component'; import {ContentRowComponent} from '../../../widgets/data_table/content_row_component'; import {HeaderCellComponent} from '../../../widgets/data_table/header_cell_component'; @@ -4430,26 +4431,29 @@ describe('scalar card', () => { const scalarCardDataTable = fixture.debugElement.query( By.directive(ScalarCardDataTable) ); - - const headers = [ - { + const reorderColumnEvent: ReorderColumnEvent = { + source: { type: ColumnHeaderType.RUN, name: 'run', displayName: 'Run', enabled: true, }, - { + destination: { type: ColumnHeaderType.VALUE, name: 'value', displayName: 'Value', enabled: true, }, - ]; - scalarCardDataTable.componentInstance.orderColumns(headers); + side: Side.RIGHT, + }; + + scalarCardDataTable.componentInstance.onOrderColumns( + reorderColumnEvent + ); expect(dispatchedActions).toEqual([ dataTableColumnEdited({ - headers, + ...reorderColumnEvent, dataTableMode: DataTableMode.SINGLE, }), ]); @@ -4474,116 +4478,33 @@ describe('scalar card', () => { const scalarCardDataTable = fixture.debugElement.query( By.directive(ScalarCardDataTable) ); - - const headers = [ - { + const reorderColumnEvent: ReorderColumnEvent = { + source: { type: ColumnHeaderType.RUN, name: 'run', displayName: 'Run', enabled: true, }, - { + destination: { type: ColumnHeaderType.VALUE, name: 'value', displayName: 'Value', enabled: true, }, - ]; - scalarCardDataTable.componentInstance.orderColumns(headers); + side: Side.RIGHT, + }; + + scalarCardDataTable.componentInstance.onOrderColumns( + reorderColumnEvent + ); expect(dispatchedActions).toEqual([ dataTableColumnEdited({ - headers, + ...reorderColumnEvent, dataTableMode: DataTableMode.RANGE, }), ]); })); - - it('emits dataTableColumnToggled when onRemoveColumn is called with range selection disabled', fakeAsync(() => { - store.overrideSelector(getSingleSelectionHeaders, [ - { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - enabled: true, - }, - { - type: ColumnHeaderType.VALUE, - name: 'value', - displayName: 'Value', - enabled: false, - }, - ]); - store.overrideSelector(getCardStateMap, { - card1: { - rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_DISABLED, - }, - }); - const fixture = createComponent('card1'); - fixture.detectChanges(); - - fixture.componentInstance.onRemoveColumn({ - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - enabled: true, - }); - - expect(dispatchedActions).toEqual([ - dataTableColumnToggled({ - cardId: 'card1', - header: { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - enabled: true, - }, - }), - ]); - })); - - it('emits dataTableColumnToggled when onRemoveColumn is called with range selection enabled', fakeAsync(() => { - store.overrideSelector(getRangeSelectionHeaders, [ - { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - enabled: true, - }, - { - type: ColumnHeaderType.MIN_VALUE, - name: 'minValue', - displayName: 'Min Value', - enabled: true, - }, - ]); - store.overrideSelector(getCardStateMap, { - card1: { - rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, - }, - }); - const fixture = createComponent('card1'); - fixture.detectChanges(); - - fixture.componentInstance.onRemoveColumn({ - type: ColumnHeaderType.MIN_VALUE, - name: 'minValue', - displayName: 'Min Value', - enabled: true, - }); - - expect(dispatchedActions).toEqual([ - dataTableColumnToggled({ - cardId: 'card1', - header: { - type: ColumnHeaderType.MIN_VALUE, - name: 'minValue', - displayName: 'Min Value', - enabled: true, - }, - }), - ]); - })); }); }); diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/BUILD b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/BUILD index 4de20c5ee4..4262e2e897 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/BUILD +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/BUILD @@ -27,10 +27,12 @@ tf_ng_module( "//tensorboard/webapp:app_state", "//tensorboard/webapp:selectors", "//tensorboard/webapp/angular:expect_angular_material_checkbox", + "//tensorboard/webapp/angular:expect_angular_material_icon", "//tensorboard/webapp/angular:expect_angular_material_tabs", "//tensorboard/webapp/metrics:types", "//tensorboard/webapp/metrics/actions", "//tensorboard/webapp/metrics/store", + "//tensorboard/webapp/metrics/views/main_view:common_selectors", "//tensorboard/webapp/widgets/data_table:data_table_header", "//tensorboard/webapp/widgets/data_table:types", "@npm//@angular/common", diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ng.html b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ng.html index fc0f611463..976ec07edf 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ng.html +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ng.html @@ -21,16 +21,16 @@ (selectedTabChange)="tabChange($event)" > - + > - + > diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts index b17aa03896..50aeb4af09 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts @@ -24,28 +24,15 @@ import { import {MatTabChangeEvent} from '@angular/material/tabs'; import { ColumnHeader, - ColumnHeaderType, DataTableMode, + Side, } from '../../../../widgets/data_table/types'; +import {HeaderEditInfo} from '../../../types'; const preventDefault = (e: MouseEvent) => { e.preventDefault(); }; -// Move the item at sourceIndex to destinationIndex -const moveHeader = ( - sourceIndex: number, - destinationIndex: number, - headers: ColumnHeader[] -) => { - const newHeaders = [...headers]; - // Delete from original location - newHeaders.splice(sourceIndex, 1); - // Insert at destinationIndex. - newHeaders.splice(destinationIndex, 0, headers[sourceIndex]); - return newHeaders; -}; - const getIndexOfColumn = (column: ColumnHeader, headers: ColumnHeader[]) => { return headers.findIndex((header) => { return header.name === column.name; @@ -72,10 +59,7 @@ export class ScalarColumnEditorComponent implements OnDestroy { @Input() singleHeaders!: ColumnHeader[]; @Input() selectedTab!: DataTableMode; - @Output() onScalarTableColumnEdit = new EventEmitter<{ - dataTableMode: DataTableMode; - headers: ColumnHeader[]; - }>(); + @Output() onScalarTableColumnEdit = new EventEmitter(); @Output() onScalarTableColumnToggled = new EventEmitter<{ dataTableMode: DataTableMode; header: ColumnHeader; @@ -92,6 +76,13 @@ export class ScalarColumnEditorComponent implements OnDestroy { ); } + getHeaderByName( + headers: ColumnHeader[], + name: string + ): ColumnHeader | undefined { + return headers.find((header) => header.name === name); + } + tabChange(event: MatTabChangeEvent) { const newMode = event.index === 0 ? DataTableMode.SINGLE : DataTableMode.RANGE; @@ -108,14 +99,20 @@ export class ScalarColumnEditorComponent implements OnDestroy { return; } const headers = this.getHeadersForMode(dataTableMode); - this.onScalarTableColumnEdit.emit({ - dataTableMode: dataTableMode, - headers: moveHeader( - getIndexOfColumn(this.draggingHeader, headers), - getIndexOfColumn(this.highlightedHeader, headers), - headers - ), - }); + const source = this.getHeaderByName(headers, this.draggingHeader.name); + const destination = this.getHeaderByName( + headers, + this.highlightedHeader.name + ); + if (source && destination && source !== destination) { + this.onScalarTableColumnEdit.emit({ + source, + destination, + side: this.highlightEdge === Edge.TOP ? Side.LEFT : Side.RIGHT, + dataTableMode, + }); + } + this.draggingHeader = undefined; this.highlightedHeader = undefined; this.hostElement.nativeElement.removeEventListener( diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts index 22136ad63a..58dcaa9f6f 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts @@ -27,7 +27,15 @@ import { getTableEditorSelectedTab, } from '../../../store/metrics_selectors'; import {HeaderEditInfo, HeaderToggleInfo} from '../../../types'; -import {DataTableMode} from '../../../../widgets/data_table/types'; +import { + ColumnHeader, + DataTableMode, +} from '../../../../widgets/data_table/types'; +import {map} from 'rxjs'; + +function headersWithoutRuns(headers: ColumnHeader[]) { + return headers.filter((header) => header.type !== 'RUN'); +} @Component({ selector: 'metrics-scalar-column-editor', @@ -48,16 +56,27 @@ import {DataTableMode} from '../../../../widgets/data_table/types'; export class ScalarColumnEditorContainer { constructor(private readonly store: Store) {} - readonly singleHeaders$ = this.store.select(getSingleSelectionHeaders); - readonly rangeHeaders$ = this.store.select(getRangeSelectionHeaders); + readonly singleHeaders$ = this.store + .select(getSingleSelectionHeaders) + .pipe(map(headersWithoutRuns)); + readonly rangeHeaders$ = this.store + .select(getRangeSelectionHeaders) + .pipe(map(headersWithoutRuns)); readonly selectedTab$ = this.store.select(getTableEditorSelectedTab); - onScalarTableColumnToggled(toggleInfo: HeaderToggleInfo) { - this.store.dispatch(dataTableColumnToggled(toggleInfo)); + onScalarTableColumnToggled({dataTableMode, header}: HeaderToggleInfo) { + this.store.dispatch(dataTableColumnToggled({dataTableMode, header})); } - onScalarTableColumnEdit(editInfo: HeaderEditInfo) { - this.store.dispatch(dataTableColumnEdited(editInfo)); + onScalarTableColumnEdit({ + source, + destination, + side, + dataTableMode, + }: HeaderEditInfo) { + this.store.dispatch( + dataTableColumnEdited({source, destination, side, dataTableMode}) + ); } onScalarTableColumnEditorClosed() { diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_test.ts b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_test.ts index 6731d9aa6a..ba5bee7d45 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_test.ts +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_test.ts @@ -39,6 +39,7 @@ import { import { ColumnHeaderType, DataTableMode, + Side, } from '../../../../widgets/data_table/types'; import {DataTableHeaderModule} from '../../../../widgets/data_table/data_table_header_module'; import {ScalarColumnEditorComponent} from './scalar_column_editor_component'; @@ -101,9 +102,9 @@ describe('scalar column editor', () => { it('renders single selection headers when selectedTab is set to SINGLE', fakeAsync(() => { store.overrideSelector(getSingleSelectionHeaders, [ { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', + type: ColumnHeaderType.SMOOTHED, + name: 'smoothed', + displayName: 'Smoothed', enabled: true, }, { @@ -121,16 +122,16 @@ describe('scalar column editor', () => { ); expect(headerElements.length).toEqual(2); - expect(headerElements[0].nativeElement.innerText).toEqual('Run'); + expect(headerElements[0].nativeElement.innerText).toEqual('Smoothed'); expect(headerElements[1].nativeElement.innerText).toEqual('Value'); })); it('renders range selection headers when selectedTab is set to RANGE', fakeAsync(() => { store.overrideSelector(getRangeSelectionHeaders, [ { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', + type: ColumnHeaderType.SMOOTHED, + name: 'smoothed', + displayName: 'Smoothed', enabled: true, }, { @@ -148,16 +149,62 @@ describe('scalar column editor', () => { ); expect(headerElements.length).toEqual(2); - expect(headerElements[0].nativeElement.innerText).toEqual('Run'); + expect(headerElements[0].nativeElement.innerText).toEqual('Smoothed'); expect(headerElements[1].nativeElement.innerText).toEqual('Value'); })); + [ + { + testDesc: 'for singleSelectionHeaders', + selector: getSingleSelectionHeaders, + mode: DataTableMode.SINGLE, + }, + { + testDesc: 'for rangeSelectionHeaders', + selector: getRangeSelectionHeaders, + mode: DataTableMode.RANGE, + }, + ].forEach(({testDesc, selector, mode}) => { + it(`hides the runs column ${testDesc}`, fakeAsync(() => { + store.overrideSelector(selector, [ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.SMOOTHED, + name: 'smoothed', + displayName: 'Smoothed', + enabled: true, + }, + { + type: ColumnHeaderType.VALUE, + name: 'value', + displayName: 'Value', + enabled: true, + }, + ]); + const fixture = createComponent(); + + switchTabs(fixture, mode); + const headerElements = fixture.debugElement.queryAll( + By.css('.header-list-item') + ); + + expect(headerElements.length).toEqual(2); + expect(headerElements[0].nativeElement.innerText).toEqual('Smoothed'); + expect(headerElements[1].nativeElement.innerText).toEqual('Value'); + })); + }); + it('checkboxes reflect enabled state', fakeAsync(() => { store.overrideSelector(getSingleSelectionHeaders, [ { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', + type: ColumnHeaderType.SMOOTHED, + name: 'smoothed', + displayName: 'Smoothed', enabled: true, }, { @@ -173,7 +220,7 @@ describe('scalar column editor', () => { const checkboxes = fixture.debugElement.queryAll(By.css('mat-checkbox')); expect(checkboxes.length).toEqual(2); - expect(checkboxes[0].nativeElement.innerText).toEqual('Run'); + expect(checkboxes[0].nativeElement.innerText).toEqual('Smoothed'); expect( checkboxes[0].nativeElement.attributes.getNamedItem('ng-reflect-checked') .value @@ -197,9 +244,9 @@ describe('scalar column editor', () => { it('dispatches dataTableColumnToggled action with singe selection when checkbox is clicked', fakeAsync(() => { store.overrideSelector(getSingleSelectionHeaders, [ { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', + type: ColumnHeaderType.SMOOTHED, + name: 'smoothed', + displayName: 'Smoothed', enabled: true, }, { @@ -222,9 +269,9 @@ describe('scalar column editor', () => { dataTableColumnToggled({ dataTableMode: DataTableMode.SINGLE, header: { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', + type: ColumnHeaderType.SMOOTHED, + name: 'smoothed', + displayName: 'Smoothed', enabled: true, }, }) @@ -278,12 +325,12 @@ describe('scalar column editor', () => { }); }); - it('dispatches dataTableColumnEdited action with singe selection when header is dragged', fakeAsync(() => { + it('dispatches dataTableColumnEdited action with single selection when header is dragged', fakeAsync(() => { store.overrideSelector(getSingleSelectionHeaders, [ { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', + type: ColumnHeaderType.SMOOTHED, + name: 'smoothed', + displayName: 'Smoothed', enabled: true, }, { @@ -313,27 +360,20 @@ describe('scalar column editor', () => { expect(dispatchedActions[0]).toEqual( dataTableColumnEdited({ + source: { + type: ColumnHeaderType.SMOOTHED, + name: 'smoothed', + displayName: 'Smoothed', + enabled: true, + }, + destination: { + type: ColumnHeaderType.VALUE, + name: 'value', + displayName: 'Value', + enabled: true, + }, + side: Side.RIGHT, dataTableMode: DataTableMode.SINGLE, - headers: [ - { - type: ColumnHeaderType.VALUE, - name: 'value', - displayName: 'Value', - enabled: true, - }, - { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - enabled: true, - }, - { - type: ColumnHeaderType.STEP, - name: 'step', - displayName: 'Step', - enabled: true, - }, - ], }) ); })); @@ -341,9 +381,9 @@ describe('scalar column editor', () => { it('dispatches dataTableColumnEdited action with range selection when header is dragged', fakeAsync(() => { store.overrideSelector(getRangeSelectionHeaders, [ { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', + type: ColumnHeaderType.SMOOTHED, + name: 'smoothed', + displayName: 'Smoothed', enabled: true, }, { @@ -373,27 +413,20 @@ describe('scalar column editor', () => { expect(dispatchedActions[0]).toEqual( dataTableColumnEdited({ + source: { + type: ColumnHeaderType.MAX_VALUE, + name: 'maxValue', + displayName: 'Max', + enabled: true, + }, + destination: { + type: ColumnHeaderType.SMOOTHED, + name: 'smoothed', + displayName: 'Smoothed', + enabled: true, + }, + side: Side.LEFT, dataTableMode: DataTableMode.RANGE, - headers: [ - { - type: ColumnHeaderType.MAX_VALUE, - name: 'maxValue', - displayName: 'Max', - enabled: true, - }, - { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - enabled: true, - }, - { - type: ColumnHeaderType.MIN_VALUE, - name: 'minValue', - displayName: 'Min', - enabled: true, - }, - ], }) ); })); @@ -401,9 +434,9 @@ describe('scalar column editor', () => { it('highlights item with bottom edge when dragging below item being dragged', fakeAsync(() => { store.overrideSelector(getRangeSelectionHeaders, [ { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', + type: ColumnHeaderType.SMOOTHED, + name: 'smoothed', + displayName: 'Smoothed', enabled: true, }, { @@ -437,9 +470,9 @@ describe('scalar column editor', () => { it('highlights item with top edge when dragging above item being dragged', fakeAsync(() => { store.overrideSelector(getRangeSelectionHeaders, [ { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', + type: ColumnHeaderType.SMOOTHED, + name: 'smoothed', + displayName: 'Smoothed', enabled: true, }, { diff --git a/tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source.ts b/tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source.ts index 1f268d4ff7..612756f7ae 100644 --- a/tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source.ts +++ b/tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source.ts @@ -235,7 +235,8 @@ export class OSSSettingsConverter extends SettingsConverter< if ( Array.isArray(backendSettings.singleSelectionHeaders) && // If the settings stored in the backend are invalid, reset back to default. - backendSettings.singleSelectionHeaders[0].name !== undefined + backendSettings.singleSelectionHeaders[0].name !== undefined && + backendSettings.singleSelectionHeaders[0].type === 'RUN' ) { updateScalarContextMenuOptions(backendSettings.singleSelectionHeaders); settings.singleSelectionHeaders = backendSettings.singleSelectionHeaders; @@ -244,7 +245,8 @@ export class OSSSettingsConverter extends SettingsConverter< if ( Array.isArray(backendSettings.rangeSelectionHeaders) && // If the settings stored in the backend are invalid, reset back to default. - backendSettings.rangeSelectionHeaders[0].name !== undefined + backendSettings.rangeSelectionHeaders[0].name !== undefined && + backendSettings.rangeSelectionHeaders[0].type === 'RUN' ) { updateScalarContextMenuOptions(backendSettings.rangeSelectionHeaders); settings.rangeSelectionHeaders = backendSettings.rangeSelectionHeaders; diff --git a/tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source_test.ts b/tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source_test.ts index 1dccb63410..959c02ca83 100644 --- a/tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source_test.ts +++ b/tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source_test.ts @@ -300,6 +300,27 @@ describe('persistent_settings data_source test', () => { expect(actual).toEqual({}); }); + it('resets singleSelectionEnabled if runs header is not first', async () => { + getItemSpy.withArgs(TEST_ONLY.GLOBAL_LOCAL_STORAGE_KEY).and.returnValue( + JSON.stringify({ + singleSelectionHeaders: [ + { + type: ColumnHeaderType.VALUE, + enabled: false, + }, + { + type: ColumnHeaderType.RUN, + enabled: true, + }, + ], + }) + ); + + const actual = await firstValueFrom(dataSource.getSettings()); + + expect(actual).toEqual({}); + }); + it('resets rangeSelectionEnabled if old ColumnHeader is stored', async () => { getItemSpy.withArgs(TEST_ONLY.GLOBAL_LOCAL_STORAGE_KEY).and.returnValue( JSON.stringify({ @@ -321,6 +342,27 @@ describe('persistent_settings data_source test', () => { expect(actual).toEqual({}); }); + it('resets rangeSelectionEnabled if runs header is not first', async () => { + getItemSpy.withArgs(TEST_ONLY.GLOBAL_LOCAL_STORAGE_KEY).and.returnValue( + JSON.stringify({ + rangeSelectionHeaders: [ + { + type: ColumnHeaderType.MIN_VALUE, + enabled: true, + }, + { + type: ColumnHeaderType.RUN, + enabled: true, + }, + ], + }) + ); + + const actual = await firstValueFrom(dataSource.getSettings()); + + expect(actual).toEqual({}); + }); + it('properly converts dashboardDisplayedHparamColumns', async () => { getItemSpy.withArgs(TEST_ONLY.GLOBAL_LOCAL_STORAGE_KEY).and.returnValue( JSON.stringify({ diff --git a/tensorboard/webapp/runs/views/runs_table/runs_data_table.ts b/tensorboard/webapp/runs/views/runs_table/runs_data_table.ts index d6cb200fe5..953190195c 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_data_table.ts +++ b/tensorboard/webapp/runs/views/runs_table/runs_data_table.ts @@ -27,6 +27,7 @@ import { FilterAddedEvent, DiscreteFilter, IntervalFilter, + ReorderColumnEvent, } from '../../../widgets/data_table/types'; @Component({ selector: 'runs-data-table', @@ -48,7 +49,7 @@ export class RunsDataTable { ColumnHeaderType = ColumnHeaderType; @Output() sortDataBy = new EventEmitter(); - @Output() orderColumns = new EventEmitter(); + @Output() orderColumns = new EventEmitter(); @Output() onSelectionToggle = new EventEmitter(); @Output() onAllSelectionToggle = new EventEmitter(); @Output() onRegexFilterChange = new EventEmitter(); diff --git a/tensorboard/webapp/widgets/data_table/BUILD b/tensorboard/webapp/widgets/data_table/BUILD index c43110f490..043e98805a 100644 --- a/tensorboard/webapp/widgets/data_table/BUILD +++ b/tensorboard/webapp/widgets/data_table/BUILD @@ -164,6 +164,29 @@ tf_ts_library( ], ) +tf_ts_library( + name = "utils", + srcs = [ + "utils.ts", + ], + deps = [ + ":types", + ], +) + +tf_ts_library( + name = "utils_test", + testonly = True, + srcs = [ + "utils_test.ts", + ], + deps = [ + ":types", + ":utils", + "@npm//@types/jasmine", + ], +) + tf_ts_library( name = "data_table_test", testonly = True, diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ts b/tensorboard/webapp/widgets/data_table/data_table_component.ts index c4c2b5ed91..dea6d2080a 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ts +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ts @@ -34,6 +34,8 @@ import { IntervalFilter, SortingInfo, SortingOrder, + ReorderColumnEvent, + Side, } from './types'; import {HeaderCellComponent} from './header_cell_component'; import {Subscription} from 'rxjs'; @@ -42,11 +44,6 @@ import {ColumnSelectorComponent} from './column_selector_component'; import {ContentCellComponent} from './content_cell_component'; import {RangeValues} from '../range_input/types'; -export enum Side { - RIGHT, - LEFT, -} - const preventDefault = function (e: MouseEvent) { e.preventDefault(); }; @@ -79,7 +76,7 @@ export class DataTableComponent implements OnDestroy, AfterContentInit { filterColumn: ColumnHeader | undefined = undefined; @Output() sortDataBy = new EventEmitter(); - @Output() orderColumns = new EventEmitter(); + @Output() orderColumns = new EventEmitter(); @Output() removeColumn = new EventEmitter(); @Output() addColumn = new EventEmitter<{ header: ColumnHeader; @@ -181,13 +178,16 @@ export class DataTableComponent implements OnDestroy, AfterContentInit { if (!this.draggingHeaderName || !this.highlightedColumnName) { return; } + const source = this.getHeaderByName(this.draggingHeaderName); + const destination = this.getHeaderByName(this.highlightedColumnName); + if (source && destination && source !== destination) { + this.orderColumns.emit({ + source, + destination, + side: this.highlightSide, + }); + } - this.orderColumns.emit( - this.moveHeader( - this.getIndexOfHeaderWithName(this.draggingHeaderName!), - this.getIndexOfHeaderWithName(this.highlightedColumnName!) - ) - ); this.draggingHeaderName = undefined; this.highlightedColumnName = undefined; document.removeEventListener('dragover', preventDefault); @@ -197,7 +197,10 @@ export class DataTableComponent implements OnDestroy, AfterContentInit { } dragEnter(header: ColumnHeader) { - if (!this.draggingHeaderName) { + if ( + !this.draggingHeaderName || + this.getIndexOfHeaderWithName(header.name) === -1 + ) { return; } if ( @@ -239,6 +242,10 @@ export class DataTableComponent implements OnDestroy, AfterContentInit { }; } + getHeaderByName(name: string): ColumnHeader | undefined { + return this.headers.find((header) => header.name === name); + } + getIndexOfHeaderWithName(name: string) { return this.headers.findIndex((element) => { return name === element.name; diff --git a/tensorboard/webapp/widgets/data_table/data_table_test.ts b/tensorboard/webapp/widgets/data_table/data_table_test.ts index 0ed17276ee..0356f9c76a 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_test.ts +++ b/tensorboard/webapp/widgets/data_table/data_table_test.ts @@ -26,8 +26,9 @@ import { DiscreteFilter, IntervalFilter, DomainType, + Side, } from './types'; -import {DataTableComponent, Side} from './data_table_component'; +import {DataTableComponent} from './data_table_component'; import {DataTableModule} from './data_table_module'; import {HeaderCellComponent} from './header_cell_component'; import {NoopAnimationsModule} from '@angular/platform-browser/animations'; @@ -447,26 +448,21 @@ describe('data table', () => { ).toBe(true); headerElements[1].query(By.css('.cell')).triggerEventHandler('dragend'); - expect(orderColumnsSpy).toHaveBeenCalledOnceWith([ - { + expect(orderColumnsSpy).toHaveBeenCalledOnceWith({ + source: { type: ColumnHeaderType.RUN, name: 'run', displayName: 'Run', enabled: true, }, - { + destination: { type: ColumnHeaderType.VALUE, name: 'value', displayName: 'Value', enabled: true, }, - { - type: ColumnHeaderType.STEP, - name: 'step', - displayName: 'Step', - enabled: true, - }, - ]); + side: Side.LEFT, + }); }); it('emits orderColumns with new order when dragged right', () => { @@ -516,26 +512,21 @@ describe('data table', () => { ).toBe(true); headerElements[1].query(By.css('.cell')).triggerEventHandler('dragend'); - expect(orderColumnsSpy).toHaveBeenCalledOnceWith([ - { - type: ColumnHeaderType.VALUE, - name: 'value', - displayName: 'Value', + expect(orderColumnsSpy).toHaveBeenCalledOnceWith({ + source: { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', enabled: true, }, - { + destination: { type: ColumnHeaderType.STEP, name: 'step', displayName: 'Step', enabled: true, }, - { - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - enabled: true, - }, - ]); + side: Side.RIGHT, + }); }); it('does not show add button when there are no selectable columns', () => { diff --git a/tensorboard/webapp/widgets/data_table/types.ts b/tensorboard/webapp/widgets/data_table/types.ts index 7b004c0a00..f3cbd40a96 100644 --- a/tensorboard/webapp/widgets/data_table/types.ts +++ b/tensorboard/webapp/widgets/data_table/types.ts @@ -121,7 +121,7 @@ export enum Side { export interface ReorderColumnEvent { source: ColumnHeader; destination: ColumnHeader; - side?: Side | undefined; + side?: Side | undefined; // Only used when destination is not found. } export interface AddColumnEvent { diff --git a/tensorboard/webapp/widgets/data_table/utils.ts b/tensorboard/webapp/widgets/data_table/utils.ts new file mode 100644 index 0000000000..ef75f466a9 --- /dev/null +++ b/tensorboard/webapp/widgets/data_table/utils.ts @@ -0,0 +1,54 @@ +/* Copyright 2024 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +==============================================================================*/ + +import {ColumnHeader, Side} from './types'; + +/** + * Returns a new copy of the column headers with source moved to the index of destination. + * Returns the original headers if the move is invalid. + */ +function moveColumn( + columns: ColumnHeader[], + source: ColumnHeader, + destination: ColumnHeader, + side?: Side +): ColumnHeader[] { + const sourceIndex = columns.findIndex( + (column: ColumnHeader) => column.name === source.name + ); + let destinationIndex = columns.findIndex( + (column: ColumnHeader) => column.name === destination.name + ); + if (sourceIndex === -1 || sourceIndex === destinationIndex) { + return columns; + } + if (destinationIndex === -1) { + // Use side as a backup to determine source position if destination isn't found. + if (side !== undefined) { + destinationIndex = side === Side.LEFT ? 0 : columns.length - 1; + } else { + return columns; + } + } + + const newColumns = [...columns]; + newColumns.splice(sourceIndex, 1); + newColumns.splice(destinationIndex, 0, source); + return newColumns; +} + +export const DataTableUtils = { + moveColumn, +}; diff --git a/tensorboard/webapp/widgets/data_table/utils_test.ts b/tensorboard/webapp/widgets/data_table/utils_test.ts new file mode 100644 index 0000000000..b03af4e6d5 --- /dev/null +++ b/tensorboard/webapp/widgets/data_table/utils_test.ts @@ -0,0 +1,142 @@ +/* Copyright 2024 The TensorFlow Authors. All Rights Reserved. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +==============================================================================*/ + +import {ColumnHeaderType, Side} from './types'; +import {DataTableUtils} from './utils'; + +describe('data table utils', () => { + describe('moveColumn', () => { + const fakeColumns = [ + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'dense_layers', + displayName: 'Dense Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'dropout', + displayName: 'Dropout', + enabled: true, + }, + ]; + + it('returns original headers if source is not found', () => { + const moveResult = DataTableUtils.moveColumn( + fakeColumns, + { + type: ColumnHeaderType.HPARAM, + name: 'nonexistent_param', + displayName: 'Nonexistent param', + enabled: false, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + Side.LEFT + ); + + expect(moveResult).toEqual(fakeColumns); + }); + + it('returns original headers if source equals dest', () => { + const moveResult = DataTableUtils.moveColumn( + fakeColumns, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + Side.LEFT + ); + + expect(moveResult).toEqual(fakeColumns); + }); + + [ + { + testDesc: 'to front if side is left', + side: Side.LEFT, + expectedResult: [ + fakeColumns[1], + fakeColumns[0], + ...fakeColumns.slice(2), + ], + }, + { + testDesc: 'to back if side is right', + side: Side.RIGHT, + expectedResult: [ + fakeColumns[0], + ...fakeColumns.slice(2), + fakeColumns[1], + ], + }, + ].forEach(({testDesc, side, expectedResult}) => { + it(`if destination not found, moves source ${testDesc}`, () => { + const moveResult = DataTableUtils.moveColumn( + fakeColumns, + fakeColumns[1], + { + type: ColumnHeaderType.HPARAM, + name: 'nonexistent param', + displayName: 'Nonexistent param', + enabled: true, + }, + side + ); + + expect(moveResult).toEqual(expectedResult); + }); + }); + + it('swaps source and destination positions if destination is found', () => { + const moveResult = DataTableUtils.moveColumn( + fakeColumns, + fakeColumns[1], + fakeColumns[0], + Side.LEFT + ); + + expect(moveResult).toEqual([ + fakeColumns[1], + fakeColumns[0], + ...fakeColumns.slice(2), + ]); + }); + }); +}); From f3130d0fa7403cdf09b229b1014f5a2ad0e30439 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Thu, 25 Jan 2024 19:30:47 +0900 Subject: [PATCH 05/22] Fixes lint --- tensorboard/webapp/hparams/_redux/hparams_reducers.ts | 4 +--- .../metrics/views/card_renderer/scalar_card_component.ts | 6 +----- 2 files changed, 2 insertions(+), 8 deletions(-) diff --git a/tensorboard/webapp/hparams/_redux/hparams_reducers.ts b/tensorboard/webapp/hparams/_redux/hparams_reducers.ts index d152199bac..5ff2897c77 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_reducers.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_reducers.ts @@ -13,9 +13,7 @@ See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ import {Action, ActionReducer, createReducer, on} from '@ngrx/store'; -import { - Side, -} from '../../widgets/data_table/types'; +import {Side} from '../../widgets/data_table/types'; import {DataTableUtils} from '../../widgets/data_table/utils'; import * as actions from './hparams_actions'; import {HparamsState} from './types'; diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts index 3ec6e070a5..555b47f771 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts @@ -44,11 +44,7 @@ import { TooltipDatum, } from '../../../widgets/line_chart_v2/types'; import {CardState} from '../../store'; -import { - HeaderEditInfo, - TooltipSort, - XAxisType, -} from '../../types'; +import {HeaderEditInfo, TooltipSort, XAxisType} from '../../types'; import { MinMaxStep, ScalarCardDataSeries, From a9c53c39d57ad75db06d7abfd54a8a38747dd553 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Thu, 25 Jan 2024 19:34:29 +0900 Subject: [PATCH 06/22] Runs buildifier --- tensorboard/webapp/metrics/views/card_renderer/BUILD | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tensorboard/webapp/metrics/views/card_renderer/BUILD b/tensorboard/webapp/metrics/views/card_renderer/BUILD index d70762c9fe..af274bd8c0 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/BUILD +++ b/tensorboard/webapp/metrics/views/card_renderer/BUILD @@ -347,8 +347,8 @@ tf_ng_module( "//tensorboard/webapp/metrics/views:types", "//tensorboard/webapp/metrics/views:utils", "//tensorboard/webapp/metrics/views/main_view:common_selectors", - "//tensorboard/webapp/runs/store:types", "//tensorboard/webapp/runs:types", + "//tensorboard/webapp/runs/store:types", "//tensorboard/webapp/types", "//tensorboard/webapp/types:ui", "//tensorboard/webapp/util:value_formatter", From f17fe2290aac8a99537509190f2f8a66acb789b0 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Fri, 26 Jan 2024 16:55:21 +0900 Subject: [PATCH 07/22] Turns groupColumns into a projector function helper --- tensorboard/webapp/BUILD | 1 + tensorboard/webapp/metrics/store/BUILD | 6 + .../webapp/metrics/store/metrics_selectors.ts | 11 + .../metrics/store/metrics_selectors_test.ts | 196 ++++++++++++++++++ tensorboard/webapp/metrics/testing.ts | 4 +- .../views/main_view/common_selectors.ts | 30 +-- .../views/main_view/common_selectors_test.ts | 35 ---- tensorboard/webapp/runs/store/BUILD | 1 + .../webapp/runs/store/runs_selectors.ts | 18 +- .../webapp/runs/store/runs_selectors_test.ts | 90 ++++++++ tensorboard/webapp/testing/BUILD | 2 +- tensorboard/webapp/testing/utils.ts | 2 +- tensorboard/webapp/widgets/data_table/BUILD | 23 ++ .../webapp/widgets/data_table/types.ts | 2 + .../webapp/widgets/data_table/utils.ts | 46 ++++ .../webapp/widgets/data_table/utils_test.ts | 98 +++++++++ 16 files changed, 494 insertions(+), 71 deletions(-) create mode 100644 tensorboard/webapp/widgets/data_table/utils.ts create mode 100644 tensorboard/webapp/widgets/data_table/utils_test.ts diff --git a/tensorboard/webapp/BUILD b/tensorboard/webapp/BUILD index bd2230c338..25e45c27cf 100644 --- a/tensorboard/webapp/BUILD +++ b/tensorboard/webapp/BUILD @@ -301,6 +301,7 @@ tf_ng_web_test_suite( "//tensorboard/webapp/widgets/content_wrapping_input:content_wrapping_input_tests", "//tensorboard/webapp/widgets/custom_modal:custom_modal_test", "//tensorboard/webapp/widgets/data_table:data_table_test", + "//tensorboard/webapp/widgets/data_table:utils_test", "//tensorboard/webapp/widgets/dropdown:dropdown_tests", "//tensorboard/webapp/widgets/experiment_alias:experiment_alias_test", "//tensorboard/webapp/widgets/filter_input:filter_input_test", diff --git a/tensorboard/webapp/metrics/store/BUILD b/tensorboard/webapp/metrics/store/BUILD index 8db5c763f9..349dcfe15b 100644 --- a/tensorboard/webapp/metrics/store/BUILD +++ b/tensorboard/webapp/metrics/store/BUILD @@ -18,6 +18,7 @@ tf_ts_library( "//tensorboard/webapp/app_routing:types", "//tensorboard/webapp/app_routing/actions", "//tensorboard/webapp/core/actions", + "//tensorboard/webapp/hparams/_redux:hparams_selectors", "//tensorboard/webapp/metrics:types", "//tensorboard/webapp/metrics:utils", "//tensorboard/webapp/metrics/actions", @@ -33,6 +34,7 @@ tf_ts_library( "//tensorboard/webapp/util:types", "//tensorboard/webapp/widgets/card_fob:types", "//tensorboard/webapp/widgets/data_table:types", + "//tensorboard/webapp/widgets/data_table:utils", "//tensorboard/webapp/widgets/line_chart_v2/lib:public_types", "@npm//@ngrx/store", ], @@ -97,14 +99,18 @@ tf_ts_library( "//tensorboard/webapp/app_routing:types", "//tensorboard/webapp/app_routing/actions", "//tensorboard/webapp/core/actions", + "//tensorboard/webapp/hparams:testing", + "//tensorboard/webapp/hparams/_redux:types", "//tensorboard/webapp/metrics:test_lib", "//tensorboard/webapp/metrics:types", "//tensorboard/webapp/metrics/actions", "//tensorboard/webapp/metrics/data_source", "//tensorboard/webapp/persistent_settings", "//tensorboard/webapp/routes:testing", + "//tensorboard/webapp/testing:utils", "//tensorboard/webapp/types", "//tensorboard/webapp/util:dom", + "//tensorboard/webapp/util:types", "//tensorboard/webapp/widgets/card_fob:types", "//tensorboard/webapp/widgets/data_table:types", "@npm//@types/jasmine", diff --git a/tensorboard/webapp/metrics/store/metrics_selectors.ts b/tensorboard/webapp/metrics/store/metrics_selectors.ts index 9892241fb5..f01db8e327 100644 --- a/tensorboard/webapp/metrics/store/metrics_selectors.ts +++ b/tensorboard/webapp/metrics/store/metrics_selectors.ts @@ -50,6 +50,8 @@ import { import {ColumnHeader, DataTableMode} from '../../widgets/data_table/types'; import {Extent} from '../../widgets/line_chart_v2/lib/public_types'; import {memoize} from '../../util/memoize'; +import {getDashboardDisplayedHparamColumns} from '../../hparams/_redux/hparams_selectors'; +import {DataTableUtils} from '../../widgets/data_table/utils'; const selectMetricsState = createFeatureSelector(METRICS_FEATURE_KEY); @@ -661,3 +663,12 @@ export const getColumnHeadersForCard = memoize((cardId: string) => { } ); }); + +export const getGroupedHeadersForCard = memoize((cardId: string) => + createSelector( + getColumnHeadersForCard(cardId), + getDashboardDisplayedHparamColumns, + (standardColumns, hparamColumns) => + DataTableUtils.groupColumns([...standardColumns, ...hparamColumns]) + ) +); diff --git a/tensorboard/webapp/metrics/store/metrics_selectors_test.ts b/tensorboard/webapp/metrics/store/metrics_selectors_test.ts index a30cb45a0e..28b63e1a70 100644 --- a/tensorboard/webapp/metrics/store/metrics_selectors_test.ts +++ b/tensorboard/webapp/metrics/store/metrics_selectors_test.ts @@ -33,6 +33,14 @@ import { } from '../../widgets/data_table/types'; import * as selectors from './metrics_selectors'; import {CardFeatureOverride, MetricsState} from './metrics_types'; +import {buildMockState} from '../../testing/utils'; +import { + buildHparamSpec, + buildHparamsState, + buildStateFromHparamsState, +} from '../../hparams/testing'; +import {DeepPartial} from '../../util/types'; +import {HparamsState} from '../../hparams/_redux/types'; describe('metrics selectors', () => { beforeEach(() => { @@ -1745,4 +1753,192 @@ describe('metrics selectors', () => { ).toEqual(rangeSelectionHeaders); }); }); + + describe('getGroupedHeadersForCard', () => { + let singleSelectionHeaders: ColumnHeader[]; + let rangeSelectionHeaders: ColumnHeader[]; + let hparamsState: DeepPartial; + + beforeEach(() => { + singleSelectionHeaders = [ + { + type: ColumnHeaderType.COLOR, + name: 'color', + displayName: 'Color', + enabled: true, + }, + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'My Run name', + enabled: false, + }, + ]; + rangeSelectionHeaders = [ + { + type: ColumnHeaderType.MEAN, + name: 'mean', + displayName: 'Mean', + enabled: true, + }, + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'My Run name', + enabled: false, + }, + ]; + hparamsState = { + dashboardSpecs: { + hparams: [ + buildHparamSpec({name: 'conv_layers'}), + buildHparamSpec({name: 'conv_kernel_size'}), + ], + }, + dashboardDisplayedHparamColumns: [ + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + ], + }; + }); + + it('returns grouped single selection headers when card range selection is disabled', () => { + const state = buildMockState({ + ...appStateFromMetricsState( + buildMetricsState({ + singleSelectionHeaders, + rangeSelectionHeaders, + cardStateMap: { + card1: { + rangeSelectionOverride: + CardFeatureOverride.OVERRIDE_AS_DISABLED, + }, + }, + }) + ), + ...buildStateFromHparamsState(buildHparamsState(hparamsState)), + }); + + expect(selectors.getGroupedHeadersForCard('card1')(state)).toEqual([ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'My Run name', + enabled: false, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + { + type: ColumnHeaderType.COLOR, + name: 'color', + displayName: 'Color', + enabled: true, + }, + ]); + }); + + it('returns grouped range selection headers when card range selection is enabled', () => { + const state = buildMockState({ + ...appStateFromMetricsState( + buildMetricsState({ + singleSelectionHeaders, + rangeSelectionHeaders, + cardStateMap: { + card1: { + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, + }, + }, + }) + ), + ...buildStateFromHparamsState(buildHparamsState(hparamsState)), + }); + + expect(selectors.getGroupedHeadersForCard('card1')(state)).toEqual([ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'My Run name', + enabled: false, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + { + type: ColumnHeaderType.MEAN, + name: 'mean', + displayName: 'Mean', + enabled: true, + }, + ]); + }); + + it('returns grouped range selection headers when global range selection is enabled', () => { + const state = buildMockState({ + ...appStateFromMetricsState( + buildMetricsState({ + singleSelectionHeaders, + rangeSelectionHeaders, + rangeSelectionEnabled: true, + }) + ), + ...buildStateFromHparamsState(buildHparamsState(hparamsState)), + }); + + expect(selectors.getGroupedHeadersForCard('card1')(state)).toEqual([ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'My Run name', + enabled: false, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + { + type: ColumnHeaderType.MEAN, + name: 'mean', + displayName: 'Mean', + enabled: true, + }, + ]); + }); + }); }); diff --git a/tensorboard/webapp/metrics/testing.ts b/tensorboard/webapp/metrics/testing.ts index de33b3b66d..4dc9757d89 100644 --- a/tensorboard/webapp/metrics/testing.ts +++ b/tensorboard/webapp/metrics/testing.ts @@ -28,14 +28,12 @@ import { TimeSeriesRequest, TimeSeriesResponse, } from './data_source'; +import * as selectors from './store/metrics_selectors'; import { MetricsState, METRICS_FEATURE_KEY, TagMetadata, TimeSeriesData, -} from './store'; -import * as selectors from './store/metrics_selectors'; -import { CardStepIndexMetaData, MetricsSettings, RunToSeries, diff --git a/tensorboard/webapp/metrics/views/main_view/common_selectors.ts b/tensorboard/webapp/metrics/views/main_view/common_selectors.ts index 76354a4ad9..0836af7fb4 100644 --- a/tensorboard/webapp/metrics/views/main_view/common_selectors.ts +++ b/tensorboard/webapp/metrics/views/main_view/common_selectors.ts @@ -12,7 +12,7 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ -import {createSelector, Selector} from '@ngrx/store'; +import {createSelector} from '@ngrx/store'; import {State} from '../../../app_state'; import { getCurrentRouteRunSelection, @@ -45,7 +45,6 @@ import { RunTableItem, RunTableExperimentItem, } from '../../../runs/views/runs_table/types'; -import {getRunsTableHeaders} from '../../../runs/store/runs_selectors'; import {matchRunToRegex} from '../../../util/matcher'; import {isSingleRunPlugin, PluginType} from '../../data_source'; import {getNonEmptyCardIdsWithMetadata, TagMetadata} from '../../store'; @@ -305,33 +304,6 @@ export const getSelectableColumns = createSelector( } ); -/** Returns a list of columns that have been sorted into logical groups. - * - * Column order: | RUN | experimentAlias | HPARAMs | other | - */ -export const getGroupedColumns = ( - headersSelector: Selector -) => - createSelector( - headersSelector, - getDashboardDisplayedHparamColumns, - (tableHeaders, hparamHeaders): ColumnHeader[] => { - return [ - ...tableHeaders.filter((header) => header.type === 'RUN'), - ...tableHeaders.filter( - (header) => - header.type === 'CUSTOM' && header.name === 'experimentAlias' - ), - ...hparamHeaders, - ...tableHeaders.filter( - (header) => - header.type !== 'RUN' && - !(header.type === 'CUSTOM' && header.name === 'experimentAlias') - ), - ]; - } - ); - export const getAllPotentialColumnsForCard = memoize((cardId: string) => { return createSelector( getColumnHeadersForCard(cardId), diff --git a/tensorboard/webapp/metrics/views/main_view/common_selectors_test.ts b/tensorboard/webapp/metrics/views/main_view/common_selectors_test.ts index 6667e7beea..11b133f54b 100644 --- a/tensorboard/webapp/metrics/views/main_view/common_selectors_test.ts +++ b/tensorboard/webapp/metrics/views/main_view/common_selectors_test.ts @@ -25,7 +25,6 @@ import {buildRoute} from '../../../app_routing/testing'; import {buildExperiment} from '../../../experiments/store/testing'; import {IntervalFilter, DiscreteFilter} from '../../../hparams/types'; import {DomainType, Run} from '../../../runs/store/runs_types'; -import {getRunsTableHeaders} from '../../../runs/store/runs_selectors'; import { buildRun, buildRunsState, @@ -1119,38 +1118,4 @@ describe('common selectors', () => { ]); }); }); - - describe('getGroupedColumns', () => { - it('returns a grouped list of columns given a list of standard columns', () => { - expect(selectors.getGroupedColumns(getRunsTableHeaders)(state)).toEqual([ - jasmine.objectContaining({ - type: ColumnHeaderType.RUN, - name: 'run', - displayName: 'Run', - }), - jasmine.objectContaining({ - type: ColumnHeaderType.CUSTOM, - name: 'experimentAlias', - displayName: 'Experiment', - }), - jasmine.objectContaining({ - type: ColumnHeaderType.HPARAM, - name: 'conv_layers', - displayName: 'Conv Layers', - enabled: true, - }), - jasmine.objectContaining({ - type: ColumnHeaderType.HPARAM, - name: 'dense_layers', - displayName: 'Dense Layers', - enabled: true, - }), - jasmine.objectContaining({ - type: ColumnHeaderType.CUSTOM, - name: 'fakeRunsHeader', - displayName: 'Fake Runs Header', - }), - ]); - }); - }); }); diff --git a/tensorboard/webapp/runs/store/BUILD b/tensorboard/webapp/runs/store/BUILD index ef603b75ec..c0f9fa24bd 100644 --- a/tensorboard/webapp/runs/store/BUILD +++ b/tensorboard/webapp/runs/store/BUILD @@ -52,6 +52,7 @@ tf_ts_library( "//tensorboard/webapp/types", "//tensorboard/webapp/types:ui", "//tensorboard/webapp/widgets/data_table:types", + "//tensorboard/webapp/widgets/data_table:utils", "@npm//@ngrx/store", ], ) diff --git a/tensorboard/webapp/runs/store/runs_selectors.ts b/tensorboard/webapp/runs/store/runs_selectors.ts index e8cff472f2..5e5d091636 100644 --- a/tensorboard/webapp/runs/store/runs_selectors.ts +++ b/tensorboard/webapp/runs/store/runs_selectors.ts @@ -26,9 +26,13 @@ import { } from './runs_types'; import {createGroupBy} from './utils'; import {getExperimentIdsFromRoute} from '../../app_routing/store/app_routing_selectors'; -import {getDashboardSessionGroups} from '../../hparams/_redux/hparams_selectors'; +import { + getDashboardDisplayedHparamColumns, + getDashboardSessionGroups, +} from '../../hparams/_redux/hparams_selectors'; import {HparamValue, RunToHparamsAndMetrics} from '../../hparams/types'; import {ColumnHeader, SortingInfo} from '../../widgets/data_table/types'; +import {DataTableUtils} from '../../widgets/data_table/utils'; const getRunsState = createFeatureSelector(RUNS_FEATURE_KEY); @@ -301,7 +305,7 @@ export const getColorGroupRegexString = createSelector( ); /** - * Gets the columns to be displayed by the runs table. + * Gets the standard columns to be displayed by the runs table. */ export const getRunsTableHeaders = createSelector( getUiState, @@ -319,3 +323,13 @@ export const getRunsTableSortingInfo = createSelector( return state.sortingInfo; } ); + +/** + * Gets the grouped columns to be displayed by the runs table. + */ +export const getGroupedRunsTableHeaders = createSelector( + getRunsTableHeaders, + getDashboardDisplayedHparamColumns, + (runsTableHeaders, hparamColumns) => + DataTableUtils.groupColumns([...runsTableHeaders, ...hparamColumns]) +); diff --git a/tensorboard/webapp/runs/store/runs_selectors_test.ts b/tensorboard/webapp/runs/store/runs_selectors_test.ts index 01363d880c..3d31b4e72f 100644 --- a/tensorboard/webapp/runs/store/runs_selectors_test.ts +++ b/tensorboard/webapp/runs/store/runs_selectors_test.ts @@ -21,6 +21,7 @@ import { buildSessionGroup, buildStateFromHparamsState, buildHparamsState, + buildHparamSpec, } from '../../hparams/testing'; import {buildMockState} from '../../testing/utils'; import {DataLoadState} from '../../types/data'; @@ -1027,6 +1028,95 @@ describe('runs_selectors', () => { }); }); + describe('#getGroupedRunsTableHeaders', () => { + it('returns runs table headers grouped with other headers', () => { + const state = buildMockState({ + runs: buildRunsState( + {}, + { + runsTableHeaders: [ + { + type: ColumnHeaderType.COLOR, + name: 'color', + displayName: 'Color', + enabled: true, + }, + { + type: ColumnHeaderType.CUSTOM, + name: 'experimentAlias', + displayName: 'Experiment Alias', + enabled: true, + }, + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + ], + } + ), + ...buildStateFromHparamsState( + buildHparamsState({ + dashboardSpecs: { + hparams: [ + buildHparamSpec({name: 'conv_layers'}), + buildHparamSpec({name: 'conv_kernel_size'}), + ], + }, + dashboardDisplayedHparamColumns: [ + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + ], + }) + ), + }); + + expect(selectors.getGroupedRunsTableHeaders(state)).toEqual([ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.CUSTOM, + name: 'experimentAlias', + displayName: 'Experiment Alias', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + { + type: ColumnHeaderType.COLOR, + name: 'color', + displayName: 'Color', + enabled: true, + }, + ]); + }); + }); + describe('#getRunsTableSortingInfo', () => { it('returns the runs data table sorting info', () => { const state = buildMockState({ diff --git a/tensorboard/webapp/testing/BUILD b/tensorboard/webapp/testing/BUILD index 7fa69f955f..1c2722990b 100644 --- a/tensorboard/webapp/testing/BUILD +++ b/tensorboard/webapp/testing/BUILD @@ -83,7 +83,7 @@ tf_ts_library( "//tensorboard/webapp/hparams/_redux:testing", "//tensorboard/webapp/hparams/_redux:types", "//tensorboard/webapp/metrics:test_lib", - "//tensorboard/webapp/metrics/store", + "//tensorboard/webapp/metrics/store:types", "//tensorboard/webapp/notification_center/_redux:testing", "//tensorboard/webapp/notification_center/_redux:types", "//tensorboard/webapp/persistent_settings/_redux:testing", diff --git a/tensorboard/webapp/testing/utils.ts b/tensorboard/webapp/testing/utils.ts index ad63ce8a86..f2c78daad4 100644 --- a/tensorboard/webapp/testing/utils.ts +++ b/tensorboard/webapp/testing/utils.ts @@ -45,7 +45,7 @@ import { buildHparamsState, buildStateFromHparamsState, } from '../hparams/_redux/testing'; -import {METRICS_FEATURE_KEY} from '../metrics/store'; +import {METRICS_FEATURE_KEY} from '../metrics/store/metrics_types'; import {appStateFromMetricsState, buildMetricsState} from '../metrics/testing'; import {NOTIFICATION_FEATURE_KEY} from '../notification_center/_redux/notification_center_types'; import { diff --git a/tensorboard/webapp/widgets/data_table/BUILD b/tensorboard/webapp/widgets/data_table/BUILD index c43110f490..043e98805a 100644 --- a/tensorboard/webapp/widgets/data_table/BUILD +++ b/tensorboard/webapp/widgets/data_table/BUILD @@ -164,6 +164,29 @@ tf_ts_library( ], ) +tf_ts_library( + name = "utils", + srcs = [ + "utils.ts", + ], + deps = [ + ":types", + ], +) + +tf_ts_library( + name = "utils_test", + testonly = True, + srcs = [ + "utils_test.ts", + ], + deps = [ + ":types", + ":utils", + "@npm//@types/jasmine", + ], +) + tf_ts_library( name = "data_table_test", testonly = True, diff --git a/tensorboard/webapp/widgets/data_table/types.ts b/tensorboard/webapp/widgets/data_table/types.ts index 7b004c0a00..a3ed4bf1bd 100644 --- a/tensorboard/webapp/widgets/data_table/types.ts +++ b/tensorboard/webapp/widgets/data_table/types.ts @@ -129,3 +129,5 @@ export interface AddColumnEvent { nextTo?: ColumnHeader | undefined; side?: Side | undefined; } + +export type ColumnGroup = 'RUN' | 'EXPERIMENT_ALIAS' | 'HPARAM' | 'OTHER'; diff --git a/tensorboard/webapp/widgets/data_table/utils.ts b/tensorboard/webapp/widgets/data_table/utils.ts new file mode 100644 index 0000000000..e0d29509ca --- /dev/null +++ b/tensorboard/webapp/widgets/data_table/utils.ts @@ -0,0 +1,46 @@ +/* Copyright 2024 The TensorFlow Authors. All Rights Reserved. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +==============================================================================*/ + +import {ColumnHeader, ColumnGroup} from './types'; + +function columnToGroup(column: ColumnHeader): ColumnGroup { + if (column.type === 'RUN') { + return 'RUN'; + } else if (column.type === 'CUSTOM' && column.name === 'experimentAlias') { + return 'EXPERIMENT_ALIAS'; + } else if (column.type === 'HPARAM') { + return 'HPARAM'; + } else { + return 'OTHER'; + } +} + +/** Sorts columns into predefined groups. + * + * Preserves relative order within groups. + */ +function groupColumns(columns: ColumnHeader[]): ColumnHeader[] { + const headerGroups = new Map([ + ['RUN', []], + ['EXPERIMENT_ALIAS', []], + ['HPARAM', []], + ['OTHER', []], + ]); + columns.forEach((column) => { + headerGroups.get(columnToGroup(column))?.push(column); + }); + return Array.from(headerGroups.values()).flat(); +} + +export const DataTableUtils = { + groupColumns, +}; diff --git a/tensorboard/webapp/widgets/data_table/utils_test.ts b/tensorboard/webapp/widgets/data_table/utils_test.ts new file mode 100644 index 0000000000..4f4ae5c8c3 --- /dev/null +++ b/tensorboard/webapp/widgets/data_table/utils_test.ts @@ -0,0 +1,98 @@ +/* Copyright 2024 The TensorFlow Authors. All Rights Reserved. +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +==============================================================================*/ + +import {ColumnHeaderType} from './types'; +import {DataTableUtils} from './utils'; + +describe('data table utils', () => { + describe('groupColumns', () => { + it('groups columns according to a predefined order', () => { + const inputColumns = [ + { + type: ColumnHeaderType.VALUE, + name: 'value', + displayName: 'Value', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.SMOOTHED, + name: 'smoothed', + displayName: 'Smoothed', + enabled: true, + }, + { + type: ColumnHeaderType.CUSTOM, + name: 'experimentAlias', + displayName: 'Experiment Alias', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + ]; + + expect(DataTableUtils.groupColumns(inputColumns)).toEqual([ + { + type: ColumnHeaderType.RUN, + name: 'run', + displayName: 'Run', + enabled: true, + }, + { + type: ColumnHeaderType.CUSTOM, + name: 'experimentAlias', + displayName: 'Experiment Alias', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + { + type: ColumnHeaderType.VALUE, + name: 'value', + displayName: 'Value', + enabled: true, + }, + { + type: ColumnHeaderType.SMOOTHED, + name: 'smoothed', + displayName: 'Smoothed', + enabled: true, + }, + ]); + }); + }); +}); From 6e45e6eb936e768460b63ede898e22ab0da8c395 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Fri, 26 Jan 2024 19:54:30 +0900 Subject: [PATCH 08/22] Makes runs table use hparam store for hparam columns --- WORKSPACE | 6 +- tensorboard/webapp/hparams/_redux/BUILD | 2 + .../webapp/hparams/_redux/hparams_reducers.ts | 13 +++- .../hparams/_redux/hparams_reducers_test.ts | 73 +++++++++++++++++++ tensorboard/webapp/runs/store/BUILD | 1 + .../webapp/runs/store/runs_selectors.ts | 15 +++- .../webapp/runs/store/runs_selectors_test.ts | 59 ++++++++++++--- .../views/runs_table/runs_data_table.ng.html | 3 - .../runs/views/runs_table/runs_data_table.ts | 6 +- .../views/runs_table/runs_data_table_test.ts | 18 +++-- .../views/runs_table/runs_table_container.ts | 42 +++++------ .../data_table/data_table_component.ts | 12 +-- 12 files changed, 189 insertions(+), 61 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 2b9a407308..f5c97007b7 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -15,13 +15,13 @@ http_archive( load("@bazel_skylib//lib:versions.bzl", "versions") versions.check( - # Keep this version in sync with: - # * The BAZEL environment variable defined in .github/workflows/ci.yml, which is used for CI and nightly builds. - minimum_bazel_version = "4.2.2", # Preemptively assume the next Bazel major version will break us, since historically they do, # and provide a clean error message in that case. Since the maximum version is inclusive rather # than exclusive, we set it to the 999th patch release of the current major version. maximum_bazel_version = "6.999.0", + # Keep this version in sync with: + # * The BAZEL environment variable defined in .github/workflows/ci.yml, which is used for CI and nightly builds. + minimum_bazel_version = "4.2.2", ) http_archive( diff --git a/tensorboard/webapp/hparams/_redux/BUILD b/tensorboard/webapp/hparams/_redux/BUILD index 15a565d974..5073a2c3f0 100644 --- a/tensorboard/webapp/hparams/_redux/BUILD +++ b/tensorboard/webapp/hparams/_redux/BUILD @@ -65,6 +65,7 @@ tf_ts_library( ":types", ":utils", "//tensorboard/webapp/hparams:_types", + "//tensorboard/webapp/persistent_settings", "//tensorboard/webapp/runs/actions", "//tensorboard/webapp/widgets/data_table:types", "//tensorboard/webapp/widgets/data_table:utils", @@ -168,6 +169,7 @@ tf_ts_library( "//tensorboard/webapp/app_routing/actions", "//tensorboard/webapp/core/actions", "//tensorboard/webapp/hparams:types", + "//tensorboard/webapp/persistent_settings", "//tensorboard/webapp/runs/actions", "//tensorboard/webapp/runs/data_source:testing", "//tensorboard/webapp/runs/store:testing", diff --git a/tensorboard/webapp/hparams/_redux/hparams_reducers.ts b/tensorboard/webapp/hparams/_redux/hparams_reducers.ts index 5ff2897c77..eaca5cb3f0 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_reducers.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_reducers.ts @@ -13,8 +13,9 @@ See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ import {Action, ActionReducer, createReducer, on} from '@ngrx/store'; -import {Side} from '../../widgets/data_table/types'; import {DataTableUtils} from '../../widgets/data_table/utils'; +import {persistentSettingsLoaded} from '../../persistent_settings'; +import {Side} from '../../widgets/data_table/types'; import * as actions from './hparams_actions'; import {HparamsState} from './types'; @@ -33,6 +34,16 @@ const initialState: HparamsState = { const reducer: ActionReducer = createReducer( initialState, + on(persistentSettingsLoaded, (state, {partialSettings}) => { + const {dashboardDisplayedHparamColumns: storedColumns} = partialSettings; + if (storedColumns) { + return { + ...state, + dashboardDisplayedHparamColumns: storedColumns, + }; + } + return state; + }), on(actions.hparamsFetchSessionGroupsSucceeded, (state, action) => { const nextDashboardSpecs = action.hparamsAndMetricsSpecs; const nextDashboardSessionGroups = action.sessionGroups; diff --git a/tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts b/tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts index 895b8f6170..6ed102ac59 100644 --- a/tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts +++ b/tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts @@ -19,8 +19,81 @@ import {reducers} from './hparams_reducers'; import {buildHparamSpec, buildHparamsState, buildMetricSpec} from './testing'; import {ColumnHeaderType, Side} from '../../widgets/data_table/types'; import {DataTableUtils} from '../../widgets/data_table/utils'; +import {persistentSettingsLoaded} from '../../persistent_settings'; describe('hparams/_redux/hparams_reducers_test', () => { + describe('#persistentSettingsLoaded', () => { + it('loads dashboardDisplayedHparamColumns from the persistent settings storage', () => { + const state = buildHparamsState({ + dashboardDisplayedHparamColumns: [], + }); + const state2 = reducers( + state, + persistentSettingsLoaded({ + partialSettings: { + dashboardDisplayedHparamColumns: [ + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + ], + }, + }) + ); + + expect(state2.dashboardDisplayedHparamColumns).toEqual([ + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + ]); + }); + + it('does nothing if persistent settings does not contain dashboardDisplayedHparamColumns', () => { + const state = buildHparamsState({ + dashboardDisplayedHparamColumns: [ + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + ], + }); + const state2 = reducers( + state, + persistentSettingsLoaded({ + partialSettings: {}, + }) + ); + + expect(state2.dashboardDisplayedHparamColumns).toEqual([ + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + ]); + }); + }); + describe('hparamsFetchSessionGroupsSucceeded', () => { it('saves action.hparamsAndMetricsSpecs as dashboardSpecs', () => { const state = buildHparamsState({ diff --git a/tensorboard/webapp/runs/store/BUILD b/tensorboard/webapp/runs/store/BUILD index c0f9fa24bd..d8715bc8e8 100644 --- a/tensorboard/webapp/runs/store/BUILD +++ b/tensorboard/webapp/runs/store/BUILD @@ -99,6 +99,7 @@ tf_ts_library( ":testing", ":types", ":utils", + "//tensorboard/webapp:app_state", "//tensorboard/webapp/app_routing:testing", "//tensorboard/webapp/app_routing:types", "//tensorboard/webapp/app_routing/actions", diff --git a/tensorboard/webapp/runs/store/runs_selectors.ts b/tensorboard/webapp/runs/store/runs_selectors.ts index 5e5d091636..2176b4c6ab 100644 --- a/tensorboard/webapp/runs/store/runs_selectors.ts +++ b/tensorboard/webapp/runs/store/runs_selectors.ts @@ -330,6 +330,17 @@ export const getRunsTableSortingInfo = createSelector( export const getGroupedRunsTableHeaders = createSelector( getRunsTableHeaders, getDashboardDisplayedHparamColumns, - (runsTableHeaders, hparamColumns) => - DataTableUtils.groupColumns([...runsTableHeaders, ...hparamColumns]) + (runsTableHeaders, hparamColumns) => { + let columns = [...runsTableHeaders, ...hparamColumns]; + // Override hparam options to match runs table requirements. + columns = columns.map((column) => { + const newColumn = {...column}; + if (column.type === 'HPARAM') { + newColumn.removable = true; + newColumn.hidable = false; + } + return newColumn; + }); + return DataTableUtils.groupColumns(columns); + } ); diff --git a/tensorboard/webapp/runs/store/runs_selectors_test.ts b/tensorboard/webapp/runs/store/runs_selectors_test.ts index 3d31b4e72f..c1e083a69c 100644 --- a/tensorboard/webapp/runs/store/runs_selectors_test.ts +++ b/tensorboard/webapp/runs/store/runs_selectors_test.ts @@ -24,6 +24,7 @@ import { buildHparamSpec, } from '../../hparams/testing'; import {buildMockState} from '../../testing/utils'; +import {State} from '../../app_state'; import {DataLoadState} from '../../types/data'; import {ColumnHeaderType, SortingOrder} from '../../widgets/data_table/types'; import {GroupByKey} from '../types'; @@ -1029,8 +1030,10 @@ describe('runs_selectors', () => { }); describe('#getGroupedRunsTableHeaders', () => { - it('returns runs table headers grouped with other headers', () => { - const state = buildMockState({ + let state: State; + + beforeEach(() => { + state = buildMockState({ runs: buildRunsState( {}, { @@ -1081,38 +1084,70 @@ describe('runs_selectors', () => { }) ), }); + }); + it('returns runs table headers grouped with other headers', () => { expect(selectors.getGroupedRunsTableHeaders(state)).toEqual([ - { + jasmine.objectContaining({ type: ColumnHeaderType.RUN, name: 'run', displayName: 'Run', enabled: true, - }, - { + }), + jasmine.objectContaining({ type: ColumnHeaderType.CUSTOM, name: 'experimentAlias', displayName: 'Experiment Alias', enabled: true, - }, - { + }), + jasmine.objectContaining({ type: ColumnHeaderType.HPARAM, name: 'conv_layers', displayName: 'Conv Layers', enabled: true, - }, - { + }), + jasmine.objectContaining({ type: ColumnHeaderType.HPARAM, name: 'conv_kernel_size', displayName: 'Conv Kernel Size', enabled: true, - }, - { + }), + jasmine.objectContaining({ type: ColumnHeaderType.COLOR, name: 'color', displayName: 'Color', enabled: true, - }, + }), + ]); + }); + + it('sets the hparam column context options for the runs table', () => { + expect(selectors.getGroupedRunsTableHeaders(state)).toEqual([ + jasmine.objectContaining({ + type: ColumnHeaderType.RUN, + }), + jasmine.objectContaining({ + type: ColumnHeaderType.CUSTOM, + }), + jasmine.objectContaining({ + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + removable: true, + hidable: false, + }), + jasmine.objectContaining({ + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + removable: true, + hidable: false, + }), + jasmine.objectContaining({ + type: ColumnHeaderType.COLOR, + }), ]); }); }); diff --git a/tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html b/tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html index 31e6552043..d91853e2c0 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html +++ b/tensorboard/webapp/runs/views/runs_table/runs_data_table.ng.html @@ -28,7 +28,6 @@ (); - @Output() addColumn = new EventEmitter<{ - header: ColumnHeader; - index?: number | undefined; - }>(); + @Output() addColumn = new EventEmitter(); @Output() removeColumn = new EventEmitter(); @Output() onSelectionDblClick = new EventEmitter(); @Output() addFilter = new EventEmitter(); diff --git a/tensorboard/webapp/runs/views/runs_table/runs_data_table_test.ts b/tensorboard/webapp/runs/views/runs_table/runs_data_table_test.ts index 0de6406a5d..a4d3bc30e2 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_data_table_test.ts +++ b/tensorboard/webapp/runs/views/runs_table/runs_data_table_test.ts @@ -140,21 +140,22 @@ describe('runs_data_table', () => { ).toBeTruthy(); }); - it('projects enabled headers plus color and selected column', () => { + it('projects headers plus color and selected column', () => { const fixture = createComponent({}); const dataTable = fixture.debugElement.query( By.directive(DataTableComponent) ); const headers = dataTable.queryAll(By.directive(HeaderCellComponent)); - expect(headers.length).toBe(4); + expect(headers.length).toBe(5); expect(headers[0].componentInstance.header.name).toEqual('selected'); expect(headers[1].componentInstance.header.name).toEqual('run'); - expect(headers[2].componentInstance.header.name).toEqual('other_header'); - expect(headers[3].componentInstance.header.name).toEqual('color'); + expect(headers[2].componentInstance.header.name).toEqual('disabled_header'); + expect(headers[3].componentInstance.header.name).toEqual('other_header'); + expect(headers[4].componentInstance.header.name).toEqual('color'); }); - it('projects content for each enabled header, selected, and color column', () => { + it('projects content for each header, selected, and color column', () => { const fixture = createComponent({ data: [{id: 'runid', run: 'run name', color: 'red', other_header: 'foo'}], }); @@ -163,11 +164,12 @@ describe('runs_data_table', () => { ); const cells = dataTable.queryAll(By.directive(ContentCellComponent)); - expect(cells.length).toBe(4); + expect(cells.length).toBe(5); expect(cells[0].componentInstance.header.name).toEqual('selected'); expect(cells[1].componentInstance.header.name).toEqual('run'); - expect(cells[2].componentInstance.header.name).toEqual('other_header'); - expect(cells[3].componentInstance.header.name).toEqual('color'); + expect(cells[2].componentInstance.header.name).toEqual('disabled_header'); + expect(cells[3].componentInstance.header.name).toEqual('other_header'); + expect(cells[4].componentInstance.header.name).toEqual('color'); }); describe('color column', () => { diff --git a/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts b/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts index 6bf276aef5..a9fbb8aacf 100644 --- a/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts +++ b/tensorboard/webapp/runs/views/runs_table/runs_table_container.ts @@ -22,7 +22,6 @@ import { import {createSelector, Store} from '@ngrx/store'; import {combineLatest, Observable, of, Subject} from 'rxjs'; import { - combineLatestWith, distinctUntilChanged, filter, map, @@ -44,13 +43,15 @@ import { getRunSelectorRegexFilter, getRunsLoadState, getRunsTableFullScreen, - getRunsTableHeaders, getRunsTableSortingInfo, + getGroupedRunsTableHeaders, } from '../../../selectors'; import {DataLoadState, LoadState} from '../../../types/data'; import { + AddColumnEvent, ColumnHeader, FilterAddedEvent, + ReorderColumnEvent, SortingInfo, TableData, } from '../../../widgets/data_table/types'; @@ -59,9 +60,6 @@ import { runPageSelectionToggled, runSelectionToggled, runSelectorRegexFilterChanged, - runsTableHeaderAdded, - runsTableHeaderOrderChanged, - runsTableHeaderRemoved, runsTableSortingInfoChanged, singleRunSelected, } from '../../actions'; @@ -70,7 +68,7 @@ import {RunsTableColumn, RunTableItem} from './types'; import { getCurrentColumnFilters, getFilteredRenderableRuns, - getPotentialHparamColumns, + getSelectableColumns, } from '../../../metrics/views/main_view/common_selectors'; import {runsTableFullScreenToggled} from '../../../core/actions'; import {sortTableDataItems} from './sorting_utils'; @@ -143,18 +141,9 @@ export class RunsTableContainer implements OnInit, OnDestroy { @Input() showHparamsAndMetrics = false; regexFilter$ = this.store.select(getRunSelectorRegexFilter); - runsColumns$ = this.store.select(getRunsTableHeaders); + runsColumns$ = this.store.select(getGroupedRunsTableHeaders); runsTableFullScreen$ = this.store.select(getRunsTableFullScreen); - - selectableColumns$ = this.store.select(getPotentialHparamColumns).pipe( - combineLatestWith(this.runsColumns$), - map(([potentialColumns, currentColumns]) => { - const currentColumnNames = new Set(currentColumns.map(({name}) => name)); - return potentialColumns.filter((columnHeader) => { - return !currentColumnNames.has(columnHeader.name); - }); - }) - ); + selectableColumns$ = this.store.select(getSelectableColumns); columnFilters$ = this.store.select(getCurrentColumnFilters); @@ -332,19 +321,26 @@ export class RunsTableContainer implements OnInit, OnDestroy { this.store.dispatch(runsTableFullScreenToggled()); } - addColumn({header, index}: {header: ColumnHeader; index: number}) { - header.enabled = true; + addColumn({column, nextTo, side}: AddColumnEvent) { this.store.dispatch( - runsTableHeaderAdded({header: {...header, enabled: true}, index}) + hparamsActions.dashboardHparamColumnAdded({ + column, + nextTo, + side, + }) ); } removeColumn(header: ColumnHeader) { - this.store.dispatch(runsTableHeaderRemoved({header})); + this.store.dispatch( + hparamsActions.dashboardHparamColumnRemoved({column: header}) + ); } - orderColumns(newHeaderOrder: ColumnHeader[]) { - this.store.dispatch(runsTableHeaderOrderChanged({newHeaderOrder})); + orderColumns(event: ReorderColumnEvent) { + this.store.dispatch( + hparamsActions.dashboardHparamColumnOrderChanged(event) + ); } addHparamFilter(event: FilterAddedEvent) { diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ts b/tensorboard/webapp/widgets/data_table/data_table_component.ts index dea6d2080a..09224b3c3b 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ts +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ts @@ -36,6 +36,7 @@ import { SortingOrder, ReorderColumnEvent, Side, + AddColumnEvent, } from './types'; import {HeaderCellComponent} from './header_cell_component'; import {Subscription} from 'rxjs'; @@ -78,10 +79,7 @@ export class DataTableComponent implements OnDestroy, AfterContentInit { @Output() sortDataBy = new EventEmitter(); @Output() orderColumns = new EventEmitter(); @Output() removeColumn = new EventEmitter(); - @Output() addColumn = new EventEmitter<{ - header: ColumnHeader; - index?: number | undefined; - }>(); + @Output() addColumn = new EventEmitter(); @Output() addFilter = new EventEmitter(); @ViewChild('columnSelectorModal', {static: false}) @@ -347,7 +345,11 @@ export class DataTableComponent implements OnDestroy, AfterContentInit { } onColumnAdded(header: ColumnHeader) { - this.addColumn.emit({header, index: this.getInsertIndex()}); + this.addColumn.emit({ + column: header, + nextTo: this.contextMenuHeader, + side: this.insertColumnTo, + }); } openFilterMenu(event: MouseEvent, header: ColumnHeader) { From ec93a93e6affa51ccf63cac6094f09f70812d199 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Sat, 27 Jan 2024 13:28:33 +0900 Subject: [PATCH 09/22] Changes ColumnGroup to enum --- .../webapp/widgets/data_table/types.ts | 7 +++++- .../webapp/widgets/data_table/utils.ts | 22 ++++++++++--------- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/tensorboard/webapp/widgets/data_table/types.ts b/tensorboard/webapp/widgets/data_table/types.ts index a3ed4bf1bd..cf12975277 100644 --- a/tensorboard/webapp/widgets/data_table/types.ts +++ b/tensorboard/webapp/widgets/data_table/types.ts @@ -130,4 +130,9 @@ export interface AddColumnEvent { side?: Side | undefined; } -export type ColumnGroup = 'RUN' | 'EXPERIMENT_ALIAS' | 'HPARAM' | 'OTHER'; +export enum ColumnGroup { + RUN = 'RUN', + EXPERIMENT_ALIAS = 'EXPERIMENT_ALIAS', + HPARAM = 'HPARAM', + OTHER = 'OTHER' +} \ No newline at end of file diff --git a/tensorboard/webapp/widgets/data_table/utils.ts b/tensorboard/webapp/widgets/data_table/utils.ts index e0d29509ca..3a64980dcb 100644 --- a/tensorboard/webapp/widgets/data_table/utils.ts +++ b/tensorboard/webapp/widgets/data_table/utils.ts @@ -14,26 +14,28 @@ import {ColumnHeader, ColumnGroup} from './types'; function columnToGroup(column: ColumnHeader): ColumnGroup { if (column.type === 'RUN') { - return 'RUN'; + return ColumnGroup.RUN; } else if (column.type === 'CUSTOM' && column.name === 'experimentAlias') { - return 'EXPERIMENT_ALIAS'; + return ColumnGroup.EXPERIMENT_ALIAS; } else if (column.type === 'HPARAM') { - return 'HPARAM'; + return ColumnGroup.HPARAM; } else { - return 'OTHER'; + return ColumnGroup.OTHER; } } -/** Sorts columns into predefined groups. +/** + * Sorts columns into predefined groups. * - * Preserves relative order within groups. + * Preserves relative column order within groups. */ function groupColumns(columns: ColumnHeader[]): ColumnHeader[] { + // Using Map ensures that keys preserve order. const headerGroups = new Map([ - ['RUN', []], - ['EXPERIMENT_ALIAS', []], - ['HPARAM', []], - ['OTHER', []], + [ColumnGroup.RUN, []], + [ColumnGroup.EXPERIMENT_ALIAS, []], + [ColumnGroup.HPARAM, []], + [ColumnGroup.OTHER, []], ]); columns.forEach((column) => { headerGroups.get(columnToGroup(column))?.push(column); From c17894d8c7cebd1a16a9d85189ac18a4bfa38a1b Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Sat, 27 Jan 2024 13:44:37 +0900 Subject: [PATCH 10/22] Applies comments --- .../views/card_renderer/scalar_card_container.ts | 11 ++--------- .../scalar_column_editor_component.ts | 16 +++------------- .../scalar_column_editor_container.ts | 15 ++++----------- 3 files changed, 9 insertions(+), 33 deletions(-) diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts index 013ca742c7..888eeb7e96 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts @@ -674,15 +674,8 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { ); } - editColumnHeaders({ - source, - destination, - side, - dataTableMode, - }: HeaderEditInfo) { - this.store.dispatch( - dataTableColumnEdited({source, destination, side, dataTableMode}) - ); + editColumnHeaders(headerEditInfo: HeaderEditInfo) { + this.store.dispatch(dataTableColumnEdited(headerEditInfo)); } openTableEditMenuToMode(tableMode: DataTableMode) { diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts index 50aeb4af09..02ffd555fd 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts @@ -76,13 +76,6 @@ export class ScalarColumnEditorComponent implements OnDestroy { ); } - getHeaderByName( - headers: ColumnHeader[], - name: string - ): ColumnHeader | undefined { - return headers.find((header) => header.name === name); - } - tabChange(event: MatTabChangeEvent) { const newMode = event.index === 0 ? DataTableMode.SINGLE : DataTableMode.RANGE; @@ -99,12 +92,9 @@ export class ScalarColumnEditorComponent implements OnDestroy { return; } const headers = this.getHeadersForMode(dataTableMode); - const source = this.getHeaderByName(headers, this.draggingHeader.name); - const destination = this.getHeaderByName( - headers, - this.highlightedHeader.name - ); - if (source && destination && source !== destination) { + const source = {...this.draggingHeader}; + const destination = {...this.highlightedHeader}; + if (source && destination && source.name !== destination.name) { this.onScalarTableColumnEdit.emit({ source, destination, diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts index 58dcaa9f6f..11c282e92b 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts @@ -64,19 +64,12 @@ export class ScalarColumnEditorContainer { .pipe(map(headersWithoutRuns)); readonly selectedTab$ = this.store.select(getTableEditorSelectedTab); - onScalarTableColumnToggled({dataTableMode, header}: HeaderToggleInfo) { - this.store.dispatch(dataTableColumnToggled({dataTableMode, header})); + onScalarTableColumnToggled(toggleInfo: HeaderToggleInfo) { + this.store.dispatch(dataTableColumnToggled(toggleInfo)); } - onScalarTableColumnEdit({ - source, - destination, - side, - dataTableMode, - }: HeaderEditInfo) { - this.store.dispatch( - dataTableColumnEdited({source, destination, side, dataTableMode}) - ); + onScalarTableColumnEdit(editInfo: HeaderEditInfo) { + this.store.dispatch(dataTableColumnEdited(editInfo)); } onScalarTableColumnEditorClosed() { From ff8c096016494b7024349caa6ded705cba68cdda Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Sat, 27 Jan 2024 16:04:43 +0900 Subject: [PATCH 11/22] Applies comments --- WORKSPACE | 6 +++--- tensorboard/webapp/runs/store/runs_selectors.ts | 3 +-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index f5c97007b7..2b9a407308 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -15,13 +15,13 @@ http_archive( load("@bazel_skylib//lib:versions.bzl", "versions") versions.check( + # Keep this version in sync with: + # * The BAZEL environment variable defined in .github/workflows/ci.yml, which is used for CI and nightly builds. + minimum_bazel_version = "4.2.2", # Preemptively assume the next Bazel major version will break us, since historically they do, # and provide a clean error message in that case. Since the maximum version is inclusive rather # than exclusive, we set it to the 999th patch release of the current major version. maximum_bazel_version = "6.999.0", - # Keep this version in sync with: - # * The BAZEL environment variable defined in .github/workflows/ci.yml, which is used for CI and nightly builds. - minimum_bazel_version = "4.2.2", ) http_archive( diff --git a/tensorboard/webapp/runs/store/runs_selectors.ts b/tensorboard/webapp/runs/store/runs_selectors.ts index 2176b4c6ab..87ed5f73d9 100644 --- a/tensorboard/webapp/runs/store/runs_selectors.ts +++ b/tensorboard/webapp/runs/store/runs_selectors.ts @@ -331,9 +331,8 @@ export const getGroupedRunsTableHeaders = createSelector( getRunsTableHeaders, getDashboardDisplayedHparamColumns, (runsTableHeaders, hparamColumns) => { - let columns = [...runsTableHeaders, ...hparamColumns]; // Override hparam options to match runs table requirements. - columns = columns.map((column) => { + const columns = [...runsTableHeaders, ...hparamColumns].map((column) => { const newColumn = {...column}; if (column.type === 'HPARAM') { newColumn.removable = true; From 999bffc6917e1dc5fb68b0faabfff62bba6cd17e Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Sun, 28 Jan 2024 16:52:03 +0900 Subject: [PATCH 12/22] Adds hparams to scalar tables --- tensorboard/webapp/metrics/actions/index.ts | 12 +- .../webapp/metrics/store/metrics_selectors.ts | 14 +- .../metrics/store/metrics_selectors_test.ts | 96 +++- .../webapp/metrics/views/card_renderer/BUILD | 6 + .../scalar_card_component.ng.html | 6 + .../card_renderer/scalar_card_component.ts | 18 +- .../card_renderer/scalar_card_container.ts | 93 +++- .../scalar_card_data_table.ng.html | 6 +- .../card_renderer/scalar_card_data_table.ts | 29 +- .../views/card_renderer/scalar_card_test.ts | 423 +++++++++++++++++- .../views/main_view/card_grid_component.scss | 2 +- .../webapp/runs/store/runs_selectors.ts | 3 +- tensorboard/webapp/runs/types.ts | 10 +- .../custom_modal/custom_modal_component.ts | 9 +- .../data_table/data_table_component.ng.html | 8 + .../data_table/data_table_component.ts | 13 +- .../widgets/data_table/data_table_test.ts | 87 +++- 17 files changed, 765 insertions(+), 70 deletions(-) diff --git a/tensorboard/webapp/metrics/actions/index.ts b/tensorboard/webapp/metrics/actions/index.ts index a7f8f52d10..5fd1e16eee 100644 --- a/tensorboard/webapp/metrics/actions/index.ts +++ b/tensorboard/webapp/metrics/actions/index.ts @@ -29,12 +29,15 @@ import { HeaderEditInfo, HeaderToggleInfo, HistogramMode, - MinMaxStep, PluginType, TooltipSort, XAxisType, } from '../types'; -import {SortingInfo, DataTableMode} from '../../widgets/data_table/types'; +import { + SortingInfo, + DataTableMode, + ColumnHeader, +} from '../../widgets/data_table/types'; import {Extent} from '../../widgets/line_chart_v2/lib/public_types'; export const metricsSettingsPaneClosed = createAction( @@ -244,6 +247,11 @@ export const dataTableColumnToggled = createAction( props() ); +export const dataTableColumnAdded = createAction( + '[Metrics] Data table column added in edit menu', + props<{header: ColumnHeader}>() +); + export const stepSelectorToggled = createAction( '[Metrics] Time Selector Enable Toggle', props<{ diff --git a/tensorboard/webapp/metrics/store/metrics_selectors.ts b/tensorboard/webapp/metrics/store/metrics_selectors.ts index f01db8e327..8d821c1512 100644 --- a/tensorboard/webapp/metrics/store/metrics_selectors.ts +++ b/tensorboard/webapp/metrics/store/metrics_selectors.ts @@ -668,7 +668,17 @@ export const getGroupedHeadersForCard = memoize((cardId: string) => createSelector( getColumnHeadersForCard(cardId), getDashboardDisplayedHparamColumns, - (standardColumns, hparamColumns) => - DataTableUtils.groupColumns([...standardColumns, ...hparamColumns]) + (standardColumns, hparamColumns) => { + // Override hparam options to match scalar card table requirements. + const columns = [...standardColumns, ...hparamColumns].map((column) => { + const newColumn = {...column}; + if (column.type === 'HPARAM') { + newColumn.removable = false; + newColumn.hidable = true; + } + return newColumn; + }); + return DataTableUtils.groupColumns(columns); + } ) ); diff --git a/tensorboard/webapp/metrics/store/metrics_selectors_test.ts b/tensorboard/webapp/metrics/store/metrics_selectors_test.ts index 28b63e1a70..1c7a3b81ec 100644 --- a/tensorboard/webapp/metrics/store/metrics_selectors_test.ts +++ b/tensorboard/webapp/metrics/store/metrics_selectors_test.ts @@ -1830,30 +1830,30 @@ describe('metrics selectors', () => { }); expect(selectors.getGroupedHeadersForCard('card1')(state)).toEqual([ - { + jasmine.objectContaining({ type: ColumnHeaderType.RUN, name: 'run', displayName: 'My Run name', enabled: false, - }, - { + }), + jasmine.objectContaining({ type: ColumnHeaderType.HPARAM, name: 'conv_layers', displayName: 'Conv Layers', enabled: true, - }, - { + }), + jasmine.objectContaining({ type: ColumnHeaderType.HPARAM, name: 'conv_kernel_size', displayName: 'Conv Kernel Size', enabled: true, - }, - { + }), + jasmine.objectContaining({ type: ColumnHeaderType.COLOR, name: 'color', displayName: 'Color', enabled: true, - }, + }), ]); }); @@ -1874,30 +1874,30 @@ describe('metrics selectors', () => { }); expect(selectors.getGroupedHeadersForCard('card1')(state)).toEqual([ - { + jasmine.objectContaining({ type: ColumnHeaderType.RUN, name: 'run', displayName: 'My Run name', enabled: false, - }, - { + }), + jasmine.objectContaining({ type: ColumnHeaderType.HPARAM, name: 'conv_layers', displayName: 'Conv Layers', enabled: true, - }, - { + }), + jasmine.objectContaining({ type: ColumnHeaderType.HPARAM, name: 'conv_kernel_size', displayName: 'Conv Kernel Size', enabled: true, - }, - { + }), + jasmine.objectContaining({ type: ColumnHeaderType.MEAN, name: 'mean', displayName: 'Mean', enabled: true, - }, + }), ]); }); @@ -1914,31 +1914,79 @@ describe('metrics selectors', () => { }); expect(selectors.getGroupedHeadersForCard('card1')(state)).toEqual([ - { + jasmine.objectContaining({ type: ColumnHeaderType.RUN, name: 'run', displayName: 'My Run name', enabled: false, - }, - { + }), + jasmine.objectContaining({ type: ColumnHeaderType.HPARAM, name: 'conv_layers', displayName: 'Conv Layers', enabled: true, - }, - { + }), + jasmine.objectContaining({ type: ColumnHeaderType.HPARAM, name: 'conv_kernel_size', displayName: 'Conv Kernel Size', enabled: true, - }, - { + }), + jasmine.objectContaining({ type: ColumnHeaderType.MEAN, name: 'mean', displayName: 'Mean', enabled: true, - }, + }), ]); }); + + [ + { + testDesc: 'for single selection', + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_DISABLED, + }, + { + testDesc: 'for range selection', + rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED, + }, + ].forEach(({testDesc, rangeSelectionOverride}) => { + it(`sets proper context menu options for hparam columns ${testDesc}`, () => { + const state = buildMockState({ + ...appStateFromMetricsState( + buildMetricsState({ + singleSelectionHeaders, + rangeSelectionHeaders, + cardStateMap: { + card1: { + rangeSelectionOverride: + CardFeatureOverride.OVERRIDE_AS_ENABLED, + }, + }, + }) + ), + ...buildStateFromHparamsState(buildHparamsState(hparamsState)), + }); + + expect(selectors.getGroupedHeadersForCard('card1')(state)).toEqual([ + jasmine.objectContaining({ + type: ColumnHeaderType.RUN, + }), + jasmine.objectContaining({ + type: ColumnHeaderType.HPARAM, + removable: false, + hidable: true, + }), + jasmine.objectContaining({ + type: ColumnHeaderType.HPARAM, + removable: false, + hidable: true, + }), + jasmine.objectContaining({ + type: ColumnHeaderType.MEAN, + }), + ]); + }); + }); }); }); diff --git a/tensorboard/webapp/metrics/views/card_renderer/BUILD b/tensorboard/webapp/metrics/views/card_renderer/BUILD index af274bd8c0..3e1dbb2d98 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/BUILD +++ b/tensorboard/webapp/metrics/views/card_renderer/BUILD @@ -340,6 +340,8 @@ tf_ng_module( "//tensorboard/webapp/angular:expect_angular_material_progress_spinner", "//tensorboard/webapp/experiments:types", "//tensorboard/webapp/feature_flag/store", + "//tensorboard/webapp/hparams", + "//tensorboard/webapp/hparams:types", "//tensorboard/webapp/metrics:types", "//tensorboard/webapp/metrics/actions", "//tensorboard/webapp/metrics/data_source", @@ -461,6 +463,9 @@ tf_ts_library( "//tensorboard/webapp/angular:expect_angular_platform_browser_animations", "//tensorboard/webapp/angular:expect_ngrx_store_testing", "//tensorboard/webapp/experiments:types", + "//tensorboard/webapp/hparams/_redux:hparams_actions", + "//tensorboard/webapp/hparams/_redux:hparams_selectors", + "//tensorboard/webapp/hparams/_redux:types", "//tensorboard/webapp/metrics:test_lib", "//tensorboard/webapp/metrics:types", "//tensorboard/webapp/metrics/actions", @@ -468,6 +473,7 @@ tf_ts_library( "//tensorboard/webapp/metrics/store", "//tensorboard/webapp/metrics/store:types", "//tensorboard/webapp/metrics/views/main_view:common_selectors", + "//tensorboard/webapp/runs/store:selectors", "//tensorboard/webapp/runs/store:testing", "//tensorboard/webapp/runs/store:types", "//tensorboard/webapp/testing:mat_icon", diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html index 8d457246da..0eaffad40c 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html @@ -200,8 +200,14 @@ [columnCustomizationEnabled]="columnCustomizationEnabled" [smoothingEnabled]="smoothingEnabled" [hparamsEnabled]="hparamsEnabled" + [columnFilters]="columnFilters" + [runToHparams]="runToHparams" + [selectableColumns]="selectableColumns" (sortDataBy)="sortDataBy($event)" (editColumnHeaders)="editColumnHeaders.emit($event)" + (addColumn)="addColumn.emit($event)" + (hideColumn)="hideColumn.emit($event)" + (addFilter)="addFilter.emit($event)" > diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts index 555b47f771..df0e0e9cf8 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts @@ -44,7 +44,12 @@ import { TooltipDatum, } from '../../../widgets/line_chart_v2/types'; import {CardState} from '../../store'; -import {HeaderEditInfo, TooltipSort, XAxisType} from '../../types'; +import { + HeaderEditInfo, + HeaderToggleInfo, + TooltipSort, + XAxisType, +} from '../../types'; import { MinMaxStep, ScalarCardDataSeries, @@ -56,8 +61,13 @@ import { DataTableMode, SortingInfo, SortingOrder, + DiscreteFilter, + IntervalFilter, + FilterAddedEvent, + AddColumnEvent, } from '../../../widgets/data_table/types'; import {isDatumVisible, TimeSelectionView} from './utils'; +import {RunToHparams} from '../../../runs/types'; type ScalarTooltipDatum = TooltipDatum< ScalarCardSeriesMetadata & { @@ -102,6 +112,9 @@ export class ScalarCardComponent { @Input() columnHeaders!: ColumnHeader[]; @Input() rangeEnabled!: boolean; @Input() hparamsEnabled?: boolean; + @Input() columnFilters!: Map; + @Input() selectableColumns!: ColumnHeader[]; + @Input() runToHparams!: RunToHparams[]; @Output() onFullSizeToggle = new EventEmitter(); @Output() onPinClicked = new EventEmitter(); @@ -114,6 +127,9 @@ export class ScalarCardComponent { @Output() onDataTableSorting = new EventEmitter(); @Output() editColumnHeaders = new EventEmitter(); @Output() openTableEditMenuToMode = new EventEmitter(); + @Output() addColumn = new EventEmitter(); + @Output() hideColumn = new EventEmitter(); + @Output() addFilter = new EventEmitter(); @Output() onLineChartZoom = new EventEmitter(); @Output() onCardStateChanged = new EventEmitter>(); diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts index 013ca742c7..49b714f9b6 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts @@ -37,6 +37,7 @@ import { } from 'rxjs/operators'; import {State} from '../../../app_state'; import {ExperimentAlias} from '../../../experiments/types'; +import {actions as hparamsActions} from '../../../hparams'; import { getEnableHparamsInTimeSeries, getForceSvgFeatureFlag, @@ -57,8 +58,8 @@ import { getRun, getRunColorMap, getCurrentRouteRunSelection, - getColumnHeadersForCard, getDashboardRunsToHparams, + getGroupedHeadersForCard, } from '../../../selectors'; import {DataLoadState} from '../../../types/data'; import { @@ -78,6 +79,7 @@ import { stepSelectorToggled, timeSelectionChanged, metricsSlideoutMenuOpened, + dataTableColumnToggled, } from '../../actions'; import {PluginType, ScalarStepDatum} from '../../data_source'; import { @@ -93,8 +95,19 @@ import { getMetricsXAxisType, RunToSeries, } from '../../store'; -import {CardId, CardMetadata, HeaderEditInfo, XAxisType} from '../../types'; -import {getFilteredRenderableRunsIds} from '../main_view/common_selectors'; +import { + CardId, + CardMetadata, + HeaderEditInfo, + HeaderToggleInfo, + XAxisType, +} from '../../types'; +import {RunToHparams} from '../../../runs/types'; +import { + getFilteredRenderableRunsIds, + getCurrentColumnFilters, + getSelectableColumns, +} from '../main_view/common_selectors'; import {CardRenderer} from '../metrics_view_types'; import {getTagDisplayName} from '../utils'; import {DataDownloadDialogContainer} from './data_download_dialog_container'; @@ -111,12 +124,15 @@ import { ColumnHeader, DataTableMode, SortingInfo, + FilterAddedEvent, + AddColumnEvent, } from '../../../widgets/data_table/types'; import { maybeClipTimeSelectionView, partitionSeries, TimeSelectionView, } from './utils'; +import {RunToHparamsAndMetrics} from '../../../hparams/types'; type ScalarCardMetadata = CardMetadata & { plugin: PluginType.SCALARS; @@ -174,6 +190,9 @@ function areSeriesEqual( [columnHeaders]="columnHeaders$ | async" [rangeEnabled]="rangeEnabled$ | async" [hparamsEnabled]="hparamsEnabled$ | async" + [columnFilters]="columnFilters$ | async" + [runToHparams]="runToHparams$ | async" + [selectableColumns]="selectableColumns$ | async" (onFullSizeToggle)="onFullSizeToggle()" (onPinClicked)="pinStateChanged.emit($event)" observeIntersection @@ -185,6 +204,9 @@ function areSeriesEqual( (editColumnHeaders)="editColumnHeaders($event)" (onCardStateChanged)="onCardStateChanged($event)" (openTableEditMenuToMode)="openTableEditMenuToMode($event)" + (addColumn)="onAddColumn($event)" + (hideColumn)="onHideColumn($event)" + (addFilter)="addHparamFilter($event)" > `, styles: [ @@ -224,6 +246,9 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { cardState$?: Observable>; rangeEnabled$?: Observable; hparamsEnabled$?: Observable; + columnFilters$ = this.store.select(getCurrentColumnFilters); + runToHparams$?: Observable; + selectableColumns$?: Observable; onVisibilityChange({visible}: {visible: boolean}) { this.isVisible = visible; @@ -459,7 +484,7 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { ); this.columnHeaders$ = this.store.select( - getColumnHeadersForCard(this.cardId) + getGroupedHeadersForCard(this.cardId) ); this.chartMetadataMap$ = partitionedSeries$.pipe( @@ -588,6 +613,23 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { ); this.hparamsEnabled$ = this.store.select(getEnableHparamsInTimeSeries); + + this.runToHparams$ = this.store.select(getDashboardRunsToHparams).pipe( + map((runToHparamsAndMetrics: RunToHparamsAndMetrics): RunToHparams => { + const runToHparams: RunToHparams = {}; + for (const [runName, {hparams}] of Object.entries( + runToHparamsAndMetrics + )) { + runToHparams[runName] = {}; + for (const {name: hparamName, value} of hparams) { + runToHparams[runName][hparamName] = value; + } + } + return runToHparams; + }) + ); + + this.selectableColumns$ = this.store.select(getSelectableColumns); } ngOnDestroy() { @@ -680,12 +722,49 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { side, dataTableMode, }: HeaderEditInfo) { - this.store.dispatch( - dataTableColumnEdited({source, destination, side, dataTableMode}) - ); + if (source.type === 'HPARAM') { + this.store.dispatch( + hparamsActions.dashboardHparamColumnOrderChanged({ + source, + destination, + side, + }) + ); + } else { + this.store.dispatch( + dataTableColumnEdited({source, destination, side, dataTableMode}) + ); + } } openTableEditMenuToMode(tableMode: DataTableMode) { this.store.dispatch(metricsSlideoutMenuOpened({mode: tableMode})); } + + onAddColumn(addColumnEvent: AddColumnEvent) { + this.store.dispatch( + hparamsActions.dashboardHparamColumnAdded(addColumnEvent) + ); + } + + onHideColumn({header, dataTableMode}: HeaderToggleInfo) { + if (header.type === 'HPARAM') { + this.store.dispatch( + hparamsActions.dashboardHparamColumnToggled({column: header}) + ); + } else { + this.store.dispatch( + dataTableColumnToggled({header, cardId: this.cardId, dataTableMode}) + ); + } + } + + addHparamFilter(event: FilterAddedEvent) { + this.store.dispatch( + hparamsActions.dashboardHparamFilterAdded({ + name: event.name, + filter: event.value, + }) + ); + } } diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html index 902bdb9d1b..62d61eacd8 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ng.html @@ -18,8 +18,13 @@ @@ -28,7 +33,6 @@ [header]="header" [sortingInfo]="sortingInfo" [hparamsEnabled]="hparamsEnabled" - disableContextMenu="true" > diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts index 15e3194bd7..4c184723bc 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_data_table.ts @@ -21,11 +21,13 @@ import { } from '@angular/core'; import {TimeSelection} from '../../../widgets/card_fob/card_fob_types'; import {findClosestIndex} from '../../../widgets/line_chart_v2/sub_view/line_chart_interactive_utils'; -import {HeaderEditInfo} from '../../types'; +import {HeaderEditInfo, HeaderToggleInfo} from '../../types'; +import {RunToHparams} from '../../../runs/types'; import { ScalarCardDataSeries, ScalarCardPoint, ScalarCardSeriesMetadataMap, + SmoothedSeriesMetadata, } from './scalar_card_types'; import { ColumnHeader, @@ -34,7 +36,11 @@ import { TableData, SortingInfo, SortingOrder, + DiscreteFilter, + IntervalFilter, + FilterAddedEvent, ReorderColumnEvent, + AddColumnEvent, } from '../../../widgets/data_table/types'; import {isDatumVisible} from './utils'; @@ -53,9 +59,15 @@ export class ScalarCardDataTable { @Input() columnCustomizationEnabled!: boolean; @Input() smoothingEnabled!: boolean; @Input() hparamsEnabled?: boolean; + @Input() columnFilters!: Map; + @Input() selectableColumns!: ColumnHeader[]; + @Input() runToHparams!: RunToHparams; @Output() sortDataBy = new EventEmitter(); @Output() editColumnHeaders = new EventEmitter(); + @Output() hideColumn = new EventEmitter(); + @Output() addColumn = new EventEmitter(); + @Output() addFilter = new EventEmitter(); ColumnHeaderType = ColumnHeaderType; @@ -69,6 +81,7 @@ export class ScalarCardDataTable { }, ].concat(this.columnHeaders); } + getMinPointInRange( points: ScalarCardPoint[], startPointIndex: number, @@ -251,6 +264,16 @@ export class ScalarCardDataTable { selectedStepData[header.name] = closestEndPoint.value - closestStartPoint.value; continue; + case ColumnHeaderType.HPARAM: + let runId: string; + if ((metadata as SmoothedSeriesMetadata).originalSeriesId) { + runId = (metadata as SmoothedSeriesMetadata).originalSeriesId; + } else { + runId = metadata.id; + } + selectedStepData[header.name] = + this.runToHparams?.[runId]?.[header.name] ?? ''; + continue; default: continue; } @@ -304,6 +327,10 @@ export class ScalarCardDataTable { dataTableMode: this.getDataTableMode(), }); } + + onHideColumn(header: ColumnHeader) { + this.hideColumn.emit({header, dataTableMode: this.getDataTableMode()}); + } } function makeValueSortable( diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts index d761390179..74716eaf32 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts @@ -114,9 +114,13 @@ import { SeriesType, } from './scalar_card_types'; import { + AddColumnEvent, ColumnHeader, ColumnHeaderType, DataTableMode, + DomainType, + FilterAddedEvent, + IntervalFilter, ReorderColumnEvent, Side, SortingOrder, @@ -128,6 +132,10 @@ import * as commonSelectors from '../main_view/common_selectors'; import {ContentCellComponent} from '../../../widgets/data_table/content_cell_component'; import {ContentRowComponent} from '../../../widgets/data_table/content_row_component'; import {HeaderCellComponent} from '../../../widgets/data_table/header_cell_component'; +import {HparamFilter} from '../../../hparams/_redux/types'; +import * as hparamsSelectors from '../../../hparams/_redux/hparams_selectors'; +import * as hparamsActions from '../../../hparams/_redux/hparams_actions'; +import * as runsSelectors from '../../../runs/store/runs_selectors'; @Component({ selector: 'line-chart', @@ -2780,6 +2788,103 @@ describe('scalar card', () => { contentCellTypes.find((type) => type === ColumnHeaderType.SMOOTHED) ).toBeFalsy(); })); + + it('passes columnFilters to table', fakeAsync(() => { + store.overrideSelector( + commonSelectors.getCurrentColumnFilters, + new Map([ + [ + 'discrete hparam', + { + type: DomainType.DISCRETE, + includeUndefined: true, + possibleValues: [2, 4, 6, 8], + filterValues: [2, 4, 6, 8], + }, + ], + [ + 'interval metric', + { + type: DomainType.INTERVAL, + includeUndefined: true, + minValue: 2, + maxValue: 5, + filterLowerValue: 2, + filterUpperValue: 5, + }, + ], + ]) + ); + const fixture = createComponent('card1'); + fixture.detectChanges(); + + const dataTableComponentInstance = fixture.debugElement.query( + By.directive(DataTableComponent) + ).componentInstance; + + expect(dataTableComponentInstance.columnFilters).toEqual( + new Map([ + [ + 'discrete hparam', + { + type: DomainType.DISCRETE, + includeUndefined: true, + possibleValues: [2, 4, 6, 8], + filterValues: [2, 4, 6, 8], + }, + ], + [ + 'interval metric', + { + type: DomainType.INTERVAL, + includeUndefined: true, + minValue: 2, + maxValue: 5, + filterLowerValue: 2, + filterUpperValue: 5, + }, + ], + ]) + ); + })); + + it('passes selectableColumns to table', fakeAsync(() => { + store.overrideSelector(commonSelectors.getSelectableColumns, [ + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + ]); + const fixture = createComponent('card1'); + fixture.detectChanges(); + + const dataTableComponentInstance = fixture.debugElement.query( + By.directive(DataTableComponent) + ).componentInstance; + + expect(dataTableComponentInstance.selectableColumns).toEqual([ + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + ]); + })); }); describe('line chart integration', () => { @@ -3855,20 +3960,54 @@ describe('scalar card', () => { ]) ); + store.overrideSelector(getMetricsLinkedTimeSelection, { + start: {step: 1}, + end: null, + }); + store.overrideSelector( commonSelectors.getFilteredRenderableRunsIds, new Set(['run1']) ); - store.overrideSelector(getMetricsLinkedTimeSelection, { - start: {step: 1}, - end: null, + store.overrideSelector(selectors.getEnableHparamsInTimeSeries, true); + + store.overrideSelector( + hparamsSelectors.getDashboardDisplayedHparamColumns, + [ + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + ] + ); + store.overrideSelector(runsSelectors.getDashboardRunsToHparams, { + run1: { + hparams: [ + {name: 'conv_layers', value: 1}, + {name: 'conv_kernel_size', value: 2}, + ], + metrics: [], + }, + run2: { + hparams: [ + {name: 'conv_layers', value: 3}, + {name: 'conv_kernel_size', value: 4}, + ], + metrics: [], + }, }); }); it('filters runs by hparam when enableHparamsInTimeSeries is true', fakeAsync(() => { - store.overrideSelector(selectors.getEnableHparamsInTimeSeries, true); - const fixture = createComponent('card1'); const scalarCardDataTable = fixture.debugElement.query( By.directive(ScalarCardDataTable) @@ -3876,13 +4015,13 @@ describe('scalar card', () => { const data = scalarCardDataTable.componentInstance.getTimeSelectionTableData(); + expect(data.length).toEqual(1); expect(data[0].run).toEqual('run1'); })); it('does not filter runs by hparam when enableHparamsInTimeSeries is false', fakeAsync(() => { store.overrideSelector(selectors.getEnableHparamsInTimeSeries, false); - const fixture = createComponent('card1'); const scalarCardDataTable = fixture.debugElement.query( By.directive(ScalarCardDataTable) @@ -3890,10 +4029,66 @@ describe('scalar card', () => { const data = scalarCardDataTable.componentInstance.getTimeSelectionTableData(); + expect(data.length).toEqual(2); expect(data[0].run).toEqual('run1'); expect(data[1].run).toEqual('run2'); })); + + it('shows hparam values for selected hparam columns', fakeAsync(() => { + store.overrideSelector( + commonSelectors.getFilteredRenderableRunsIds, + new Set(['run1', 'run2']) + ); + const fixture = createComponent('card1'); + const scalarCardDataTable = fixture.debugElement.query( + By.directive(ScalarCardDataTable) + ); + + const data = + scalarCardDataTable.componentInstance.getTimeSelectionTableData(); + + expect(data).toEqual([ + jasmine.objectContaining({ + id: 'run1', + conv_layers: 1, + conv_kernel_size: 2, + }), + jasmine.objectContaining({ + id: 'run2', + conv_layers: 3, + conv_kernel_size: 4, + }), + ]); + })); + + it('shows hparam values with smoothing enabled', fakeAsync(() => { + store.overrideSelector( + commonSelectors.getFilteredRenderableRunsIds, + new Set(['run1', 'run2']) + ); + store.overrideSelector(selectors.getMetricsScalarSmoothing, 0.3); + const fixture = createComponent('card1'); + const scalarCardDataTable = fixture.debugElement.query( + By.directive(ScalarCardDataTable) + ); + + const data = + scalarCardDataTable.componentInstance.getTimeSelectionTableData(); + + expect(data).toEqual([ + jasmine.objectContaining({ + id: '["smoothed","run1"]', + conv_layers: 1, + conv_kernel_size: 2, + }), + jasmine.objectContaining({ + id: '["smoothed","run2"]', + conv_layers: 3, + conv_kernel_size: 4, + }), + ]); + })); }); }); @@ -4412,7 +4607,7 @@ describe('scalar card', () => { expect(dataTableComponent).toBeFalsy(); })); - it('emits dataTableColumnEdited with DataTableMode.SINGLE when orderColumns is called while in Single Selection', fakeAsync(() => { + it('dispatches dataTableColumnEdited with DataTableMode.SINGLE when orderColumns is called while in Single Selection', fakeAsync(() => { store.overrideSelector(getCardStateMap, { card1: { dataMinMax: { @@ -4459,7 +4654,7 @@ describe('scalar card', () => { ]); })); - it('emits dataTableColumnEdited with DataTableMode.RANGE when orderColumns is called while in Range Selection', fakeAsync(() => { + it('dispatches dataTableColumnEdited with DataTableMode.RANGE when orderColumns is called while in Range Selection', fakeAsync(() => { store.overrideSelector(getCardStateMap, { card1: { dataMinMax: { @@ -4505,6 +4700,218 @@ describe('scalar card', () => { }), ]); })); + + it('dispatches dashboardHparamColumnOrderChanged when reordering hparam columns', fakeAsync(() => { + store.overrideSelector(getCardStateMap, { + card1: { + dataMinMax: { + minStep: 0, + maxStep: 100, + }, + }, + }); + store.overrideSelector(getMetricsCardTimeSelection, { + start: {step: 1}, + end: null, + }); + store.overrideSelector(selectors.getMetricsStepSelectorEnabled, true); + const fixture = createComponent('card1'); + fixture.detectChanges(); + const dataTableComponentInstance = fixture.debugElement.query( + By.directive(DataTableComponent) + ).componentInstance; + const reorderColumnEvent: ReorderColumnEvent = { + source: { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + destination: { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + side: Side.RIGHT, + }; + + dataTableComponentInstance.orderColumns.emit(reorderColumnEvent); + + expect(dispatchedActions).toEqual([ + hparamsActions.dashboardHparamColumnOrderChanged(reorderColumnEvent), + ]); + })); + + it('dispatches dashboardHparamColumnAdded on column add event', fakeAsync(() => { + store.overrideSelector(getCardStateMap, { + card1: { + dataMinMax: { + minStep: 0, + maxStep: 100, + }, + }, + }); + store.overrideSelector(getMetricsCardTimeSelection, { + start: {step: 1}, + end: null, + }); + store.overrideSelector(selectors.getMetricsStepSelectorEnabled, true); + const fixture = createComponent('card1'); + fixture.detectChanges(); + const dataTableComponentInstance = fixture.debugElement.query( + By.directive(DataTableComponent) + ).componentInstance; + const addColumnEvent: AddColumnEvent = { + column: { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + nextTo: { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + side: Side.RIGHT, + }; + + dataTableComponentInstance.addColumn.emit(addColumnEvent); + + expect(dispatchedActions).toEqual([ + hparamsActions.dashboardHparamColumnAdded(addColumnEvent), + ]); + })); + + [ + { + testDesc: 'for single selection', + timeSelectionOverride: { + start: {step: 1}, + end: null, + }, + expectedDataTableMode: DataTableMode.SINGLE, + }, + { + testDesc: 'for range selection', + timeSelectionOverride: { + start: {step: 1}, + end: {step: 20}, + }, + expectedDataTableMode: DataTableMode.RANGE, + }, + ].forEach(({testDesc, timeSelectionOverride, expectedDataTableMode}) => { + it(`dispatches dataTableColumnToggled on column hide event ${testDesc}`, fakeAsync(() => { + store.overrideSelector(getCardStateMap, { + card1: { + dataMinMax: { + minStep: 0, + maxStep: 100, + }, + }, + }); + store.overrideSelector( + getMetricsCardTimeSelection, + timeSelectionOverride + ); + store.overrideSelector(selectors.getMetricsStepSelectorEnabled, true); + const fixture = createComponent('card1'); + fixture.detectChanges(); + const dataTableComponentInstance = fixture.debugElement.query( + By.directive(DataTableComponent) + ).componentInstance; + const columnToHide: ColumnHeader = { + type: ColumnHeaderType.VALUE, + name: 'value', + displayName: 'Value', + enabled: true, + }; + + dataTableComponentInstance.hideColumn.emit(columnToHide); + + expect(dispatchedActions).toEqual([ + dataTableColumnToggled({ + header: columnToHide, + cardId: 'card1', + dataTableMode: expectedDataTableMode, + }), + ]); + })); + }); + + it('dispatches dashboardHparamColumnToggled on column hide event for hparam columns', fakeAsync(() => { + store.overrideSelector(getCardStateMap, { + card1: { + dataMinMax: { + minStep: 0, + maxStep: 100, + }, + }, + }); + store.overrideSelector(getMetricsCardTimeSelection, { + start: {step: 1}, + end: null, + }); + store.overrideSelector(selectors.getMetricsStepSelectorEnabled, true); + const fixture = createComponent('card1'); + fixture.detectChanges(); + const dataTableComponentInstance = fixture.debugElement.query( + By.directive(DataTableComponent) + ).componentInstance; + const columnToHide: ColumnHeader = { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }; + + dataTableComponentInstance.hideColumn.emit(columnToHide); + + expect(dispatchedActions).toEqual([ + hparamsActions.dashboardHparamColumnToggled({column: columnToHide}), + ]); + })); + + it('dispatches dashboardHparamFilterAdded on column filter event', fakeAsync(() => { + store.overrideSelector(getCardStateMap, { + card1: { + dataMinMax: { + minStep: 0, + maxStep: 100, + }, + }, + }); + store.overrideSelector(getMetricsCardTimeSelection, { + start: {step: 1}, + end: null, + }); + store.overrideSelector(selectors.getMetricsStepSelectorEnabled, true); + const fixture = createComponent('card1'); + fixture.detectChanges(); + const dataTableComponentInstance = fixture.debugElement.query( + By.directive(DataTableComponent) + ).componentInstance; + const filterAddedEvent: FilterAddedEvent = { + name: 'conv_kernel_size', + value: { + type: DomainType.DISCRETE, + includeUndefined: true, + filterValues: [5], + possibleValues: [5, 7, 8], + }, + }; + + dataTableComponentInstance.addFilter.emit(filterAddedEvent); + + expect(dispatchedActions).toEqual([ + hparamsActions.dashboardHparamFilterAdded({ + name: filterAddedEvent.name, + filter: filterAddedEvent.value, + }), + ]); + })); }); }); diff --git a/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss b/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss index c12d11cf3b..4923c73223 100644 --- a/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss +++ b/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss @@ -48,7 +48,7 @@ card-view { @include tb-theme-foreground-prop(border, border, 1px solid); border-radius: 4px; box-sizing: border-box; - contain: layout paint; + contain: layout; display: block; // Make sure the cards generally have some guaranteed minimum height to avoid // jank as cards load. diff --git a/tensorboard/webapp/runs/store/runs_selectors.ts b/tensorboard/webapp/runs/store/runs_selectors.ts index 2176b4c6ab..87ed5f73d9 100644 --- a/tensorboard/webapp/runs/store/runs_selectors.ts +++ b/tensorboard/webapp/runs/store/runs_selectors.ts @@ -331,9 +331,8 @@ export const getGroupedRunsTableHeaders = createSelector( getRunsTableHeaders, getDashboardDisplayedHparamColumns, (runsTableHeaders, hparamColumns) => { - let columns = [...runsTableHeaders, ...hparamColumns]; // Override hparam options to match runs table requirements. - columns = columns.map((column) => { + const columns = [...runsTableHeaders, ...hparamColumns].map((column) => { const newColumn = {...column}; if (column.type === 'HPARAM') { newColumn.removable = true; diff --git a/tensorboard/webapp/runs/types.ts b/tensorboard/webapp/runs/types.ts index 8a284d1c39..adfe4421ec 100644 --- a/tensorboard/webapp/runs/types.ts +++ b/tensorboard/webapp/runs/types.ts @@ -12,9 +12,9 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. ==============================================================================*/ -import {Run} from './data_source/runs_data_source_types'; +import {Run, DiscreteHparamValue} from './data_source/runs_data_source_types'; -export {Run} from './data_source/runs_data_source_types'; +export {Run, DiscreteHparamValue} from './data_source/runs_data_source_types'; export type ExperimentIdToRuns = Record< string, @@ -58,3 +58,9 @@ export interface URLDeserializedState { regexFilter: string | null; }; } + +export type RunToHparams = { + [runName: string]: { + [hparamName: string]: DiscreteHparamValue; + }; +}; diff --git a/tensorboard/webapp/widgets/custom_modal/custom_modal_component.ts b/tensorboard/webapp/widgets/custom_modal/custom_modal_component.ts index d082de4911..a2e1a22539 100644 --- a/tensorboard/webapp/widgets/custom_modal/custom_modal_component.ts +++ b/tensorboard/webapp/widgets/custom_modal/custom_modal_component.ts @@ -82,10 +82,11 @@ export class CustomModalComponent implements OnInit { public openAtPosition(position: {x: number; y: number}) { const root = this.viewRef.element.nativeElement; - const top = root.getBoundingClientRect().top; - if (top !== 0) { - root.style.top = top * -1 + root.offsetTop + 'px'; - } + // Set left/top to viewport (0,0) if the element has another "containing block" ancestor. + root.style.top = `${root.offsetTop - root.getBoundingClientRect().top}px`; + root.style.left = `${ + root.offsetLeft - root.getBoundingClientRect().left + }px`; this.content.nativeElement.style.left = position.x + 'px'; this.content.nativeElement.style.top = position.y + 'px'; diff --git a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html index e7e4357aee..25f9ca4374 100644 --- a/tensorboard/webapp/widgets/data_table/data_table_component.ng.html +++ b/tensorboard/webapp/widgets/data_table/data_table_component.ng.html @@ -17,6 +17,14 @@
No Actions Available
+ + +
+
+ +
+
+ + + + + diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.scss b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.scss index 2a3f3c0464..08ba59b01e 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.scss +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.scss @@ -85,3 +85,22 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent); ::ng-deep .mat-mdc-tab-body-wrapper { flex: 1; } + +.hparams { + &-header { + display: flex; + align-items: center; + margin: 10px 10px 0; + } + + &-title { + margin: 0; + font-size: 14px; + font-weight: normal; + } + + &-add-button { + height: 30px; + width: 30px; + } +} diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts index 50aeb4af09..6a7d6227e2 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts @@ -20,9 +20,13 @@ import { Input, OnDestroy, Output, + ViewChild, } from '@angular/core'; import {MatTabChangeEvent} from '@angular/material/tabs'; +import {CustomModalComponent} from '../../../../widgets/custom_modal/custom_modal_component'; +import {ColumnSelectorComponent} from '../../../../widgets/data_table/column_selector_component'; import { + AddColumnEvent, ColumnHeader, DataTableMode, Side, @@ -57,7 +61,10 @@ export class ScalarColumnEditorComponent implements OnDestroy { highlightEdge: Edge = Edge.TOP; @Input() rangeHeaders!: ColumnHeader[]; @Input() singleHeaders!: ColumnHeader[]; + @Input() hparamHeaders!: ColumnHeader[]; + @Input() hparamsEnabled!: boolean; @Input() selectedTab!: DataTableMode; + @Input() selectableColumns!: ColumnHeader[]; @Output() onScalarTableColumnEdit = new EventEmitter(); @Output() onScalarTableColumnToggled = new EventEmitter<{ @@ -66,6 +73,12 @@ export class ScalarColumnEditorComponent implements OnDestroy { }>(); @Output() onScalarTableColumnEditorClosed = new EventEmitter(); @Output() onTabChange = new EventEmitter(); + @Output() onColumnAdded = new EventEmitter(); + + @ViewChild('columnSelectorModal', {static: false}) + private readonly columnSelectorModal!: CustomModalComponent; + @ViewChild(ColumnSelectorComponent, {static: false}) + private readonly columnSelector!: ColumnSelectorComponent; constructor(private readonly hostElement: ElementRef) {} @@ -98,7 +111,12 @@ export class ScalarColumnEditorComponent implements OnDestroy { if (!this.draggingHeader || !this.highlightedHeader) { return; } - const headers = this.getHeadersForMode(dataTableMode); + let headers: ColumnHeader[]; + if (this.draggingHeader.type === 'HPARAM') { + headers = this.hparamHeaders; + } else { + headers = this.getHeadersForMode(dataTableMode); + } const source = this.getHeaderByName(headers, this.draggingHeader.name); const destination = this.getHeaderByName( headers, @@ -126,8 +144,21 @@ export class ScalarColumnEditorComponent implements OnDestroy { return; } + // Prevent hparam columns from interacting with standard columns. + if ( + [this.draggingHeader, header].some((h) => h.type === 'HPARAM') && + this.draggingHeader.type !== header.type + ) { + return; + } + // Highlight the position which the dragging header will go when dropped. - const headers = this.getHeadersForMode(dataTableMode); + let headers: ColumnHeader[]; + if (this.draggingHeader.type === 'HPARAM') { + headers = this.hparamHeaders; + } else { + headers = this.getHeadersForMode(dataTableMode); + } if ( getIndexOfColumn(header, headers) < getIndexOfColumn(this.draggingHeader, headers) @@ -168,4 +199,23 @@ export class ScalarColumnEditorComponent implements OnDestroy { ? this.singleHeaders : this.rangeHeaders; } + + openColumnSelector(event: MouseEvent) { + const rect = ( + (event.target as HTMLElement).closest('button') as HTMLButtonElement + ).getBoundingClientRect(); + this.columnSelectorModal.openAtPosition({ + x: rect.x + rect.width, + y: rect.y, + }); + this.columnSelector.activate(); + } + + focusColumnSelector() { + this.columnSelector.focus(); + } + + onColumnSelected(header: ColumnHeader) { + this.onColumnAdded.emit({column: header}); + } } diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts index 58dcaa9f6f..788843120f 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts @@ -28,10 +28,17 @@ import { } from '../../../store/metrics_selectors'; import {HeaderEditInfo, HeaderToggleInfo} from '../../../types'; import { + AddColumnEvent, ColumnHeader, DataTableMode, } from '../../../../widgets/data_table/types'; import {map} from 'rxjs'; +import {getSelectableColumns} from '../../main_view/common_selectors'; +import { + actions as hparamsActions, + selectors as hparamSelectors, +} from '../../../../hparams'; +import {getEnableHparamsInTimeSeries} from '../../../../feature_flag/store/feature_flag_selectors'; function headersWithoutRuns(headers: ColumnHeader[]) { return headers.filter((header) => header.type !== 'RUN'); @@ -43,11 +50,15 @@ function headersWithoutRuns(headers: ColumnHeader[]) { `, @@ -62,10 +73,21 @@ export class ScalarColumnEditorContainer { readonly rangeHeaders$ = this.store .select(getRangeSelectionHeaders) .pipe(map(headersWithoutRuns)); + readonly hparamHeaders$ = this.store.select( + hparamSelectors.getDashboardDisplayedHparamColumns + ); readonly selectedTab$ = this.store.select(getTableEditorSelectedTab); + readonly selectableColumns$ = this.store.select(getSelectableColumns); + readonly hparamsEnabled$ = this.store.select(getEnableHparamsInTimeSeries); onScalarTableColumnToggled({dataTableMode, header}: HeaderToggleInfo) { - this.store.dispatch(dataTableColumnToggled({dataTableMode, header})); + if (header.type === 'HPARAM') { + this.store.dispatch( + hparamsActions.dashboardHparamColumnToggled({column: header}) + ); + } else { + this.store.dispatch(dataTableColumnToggled({dataTableMode, header})); + } } onScalarTableColumnEdit({ @@ -74,9 +96,19 @@ export class ScalarColumnEditorContainer { side, dataTableMode, }: HeaderEditInfo) { - this.store.dispatch( - dataTableColumnEdited({source, destination, side, dataTableMode}) - ); + if (source.type === 'HPARAM') { + this.store.dispatch( + hparamsActions.dashboardHparamColumnOrderChanged({ + source, + destination, + side, + }) + ); + } else { + this.store.dispatch( + dataTableColumnEdited({source, destination, side, dataTableMode}) + ); + } } onScalarTableColumnEditorClosed() { @@ -86,4 +118,8 @@ export class ScalarColumnEditorContainer { onTabChange(tab: DataTableMode) { this.store.dispatch(tableEditorTabChanged({tab})); } + + onColumnAdded(event: AddColumnEvent) { + this.store.dispatch(hparamsActions.dashboardHparamColumnAdded(event)); + } } diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_module.ts b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_module.ts index 508214d03d..9d6895c4de 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_module.ts +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_module.ts @@ -21,6 +21,8 @@ import {MatButtonModule} from '@angular/material/button'; import {ScalarColumnEditorComponent} from './scalar_column_editor_component'; import {ScalarColumnEditorContainer} from './scalar_column_editor_container'; import {DataTableHeaderModule} from '../../../../widgets/data_table/data_table_header_module'; +import {CustomModalModule} from '../../../../widgets/custom_modal/custom_modal_module'; +import {ColumnSelectorModule} from '../../../../widgets/data_table/column_selector_module'; @NgModule({ declarations: [ScalarColumnEditorComponent, ScalarColumnEditorContainer], @@ -32,6 +34,8 @@ import {DataTableHeaderModule} from '../../../../widgets/data_table/data_table_h MatTabsModule, MatIconModule, MatButtonModule, + CustomModalModule, + ColumnSelectorModule, ], }) export class ScalarColumnEditorModule {} diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_test.ts b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_test.ts index ba5bee7d45..ca89240b6b 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_test.ts +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_test.ts @@ -45,6 +45,14 @@ import {DataTableHeaderModule} from '../../../../widgets/data_table/data_table_h import {ScalarColumnEditorComponent} from './scalar_column_editor_component'; import {ScalarColumnEditorContainer} from './scalar_column_editor_container'; import {MatTabsModule} from '@angular/material/tabs'; +import {getEnableHparamsInTimeSeries} from '../../../../feature_flag/store/feature_flag_selectors'; +import {getDashboardDisplayedHparamColumns} from '../../../../hparams/_redux/hparams_selectors'; +import {provideMockTbStore} from '../../../../testing/utils'; +import {CustomModalComponent} from '../../../../widgets/custom_modal/custom_modal_component'; +import {ColumnSelectorComponent} from '../../../../widgets/data_table/column_selector_component'; +import {CustomModalModule} from '../../../../widgets/custom_modal/custom_modal_module'; +import {ColumnSelectorModule} from '../../../../widgets/data_table/column_selector_module'; +import * as hparamsActions from '../../../../hparams/_redux/hparams_actions'; describe('scalar column editor', () => { let store: MockStore; @@ -79,15 +87,38 @@ describe('scalar column editor', () => { MatTabsModule, NoopAnimationsModule, MatCheckboxModule, + CustomModalModule, + ColumnSelectorModule, ], declarations: [ScalarColumnEditorContainer, ScalarColumnEditorComponent], - providers: [provideMockStore()], + providers: [provideMockTbStore()], schemas: [NO_ERRORS_SCHEMA], }).compileComponents(); store = TestBed.inject>(Store) as MockStore; store.overrideSelector(getRangeSelectionHeaders, []); store.overrideSelector(getSingleSelectionHeaders, []); store.overrideSelector(getTableEditorSelectedTab, DataTableMode.SINGLE); + store.overrideSelector(getDashboardDisplayedHparamColumns, [ + { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + { + type: ColumnHeaderType.HPARAM, + name: 'dense_layers', + displayName: 'Dense Layers', + enabled: true, + }, + ]); + store.overrideSelector(getEnableHparamsInTimeSeries, false); }); afterEach(() => { @@ -573,4 +604,179 @@ describe('scalar column editor', () => { ).toBeTrue(); }); }); + + describe('hparam columns', () => { + beforeEach(() => { + store.overrideSelector(getEnableHparamsInTimeSeries, true); + }); + + it('hides hparam sections when hparams in time series is disabled', () => { + store.overrideSelector(getEnableHparamsInTimeSeries, false); + + const fixture = createComponent(); + + expect(fixture.debugElement.query(By.css('.hparams-header'))).toBeFalsy(); + }); + + it('shows hparam sections when hparams in time series is enabled', () => { + const fixture = createComponent(); + + expect( + fixture.debugElement.query(By.css('.hparams-header')) + ).toBeTruthy(); + }); + + it('opens column selector modal on add button click', async () => { + const focusSpy = spyOn(ColumnSelectorComponent.prototype, 'focus'); + const fixture = createComponent(); + + const addButton = fixture.debugElement.query( + By.css('.hparams-add-button') + ); + addButton.nativeElement.click(); + fixture.detectChanges(); + // Wait for modal init. + await new Promise((resolve) => window.requestAnimationFrame(resolve)); + + expect( + fixture.debugElement.query(By.directive(CustomModalComponent)) + ).toBeTruthy(); + expect( + fixture.debugElement.query(By.directive(ColumnSelectorComponent)) + ).toBeTruthy(); + expect(focusSpy).toHaveBeenCalled(); + }); + + it('dispatches dashboardHparamColumnAdded on column select', async () => { + const dispatchSpy = spyOn(store, 'dispatch'); + const fixture = createComponent(); + + const addButton = fixture.debugElement.query( + By.css('.hparams-add-button') + ); + addButton.nativeElement.click(); + fixture.detectChanges(); + // Wait for modal init. + await new Promise((resolve) => window.requestAnimationFrame(resolve)); + fixture.debugElement + .query(By.directive(ColumnSelectorComponent)) + .componentInstance.columnSelected.emit({ + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }); + + expect(dispatchSpy).toHaveBeenCalledOnceWith( + hparamsActions.dashboardHparamColumnAdded({ + column: { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + }) + ); + }); + + it('lists hparam columns', () => { + const fixture = createComponent(); + + const headerElements = fixture.debugElement.queryAll( + By.css('.hparams-list .header-list-item') + ); + expect(headerElements.length).toEqual(3); + expect(headerElements[0].nativeElement.innerText).toEqual('Conv Layers'); + expect(headerElements[1].nativeElement.innerText).toEqual( + 'Conv Kernel Size' + ); + expect(headerElements[2].nativeElement.innerText).toEqual('Dense Layers'); + }); + + it('dispatches dashboardHparamColumnOrderChanged hparam header is dragged', () => { + const dispatchSpy = spyOn(store, 'dispatch'); + const fixture = createComponent(); + const headerListItems = fixture.debugElement.queryAll( + By.css('.header-list-item') + ); + + headerListItems[0].triggerEventHandler('dragstart'); + headerListItems[1].triggerEventHandler('dragenter'); + headerListItems[0].triggerEventHandler('dragend'); + + expect(dispatchSpy).toHaveBeenCalledOnceWith( + hparamsActions.dashboardHparamColumnOrderChanged({ + source: { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + destination: { + type: ColumnHeaderType.HPARAM, + name: 'conv_kernel_size', + displayName: 'Conv Kernel Size', + enabled: true, + }, + side: Side.RIGHT, + }) + ); + }); + + it('prevents interaction between hparam and standard headers', () => { + const dispatchSpy = spyOn(store, 'dispatch'); + store.overrideSelector(getSingleSelectionHeaders, [ + { + type: ColumnHeaderType.SMOOTHED, + name: 'smoothed', + displayName: 'Smoothed', + enabled: true, + }, + ]); + const fixture = createComponent(); + const standardHeaderDebugEl = fixture.debugElement + .queryAll(By.css('.header-list-item')) + .find((debugEl) => + debugEl.nativeElement.innerHTML.includes('Smoothed') + )!; + const hparamHeaderDebugEl = fixture.debugElement + .queryAll(By.css('.header-list-item')) + .find((debugEl) => + debugEl.nativeElement.innerHTML.includes('Conv Layers') + )!; + + // Moving standard to hparam header + standardHeaderDebugEl.triggerEventHandler('dragstart'); + hparamHeaderDebugEl.triggerEventHandler('dragenter'); + standardHeaderDebugEl.triggerEventHandler('dragend'); + // Moving hparam header to standard header + hparamHeaderDebugEl.triggerEventHandler('dragstart'); + standardHeaderDebugEl.triggerEventHandler('dragenter'); + hparamHeaderDebugEl.triggerEventHandler('dragend'); + + expect(dispatchSpy).not.toHaveBeenCalled(); + }); + + it('dispatches dashboardHparamColumnToggled on hparam header checkbox click', () => { + const dispatchSpy = spyOn(store, 'dispatch'); + const fixture = createComponent(); + const checkbox = fixture.debugElement.query( + By.css('.hparams-list mat-checkbox') + ); + + checkbox.triggerEventHandler('change'); + fixture.detectChanges(); + + expect(dispatchSpy).toHaveBeenCalledOnceWith( + hparamsActions.dashboardHparamColumnToggled({ + column: { + type: ColumnHeaderType.HPARAM, + name: 'conv_layers', + displayName: 'Conv Layers', + enabled: true, + }, + }) + ); + }); + }); }); From d3f2cb9ef3c2a9cf459a496911497453ce2c88ca Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Mon, 29 Jan 2024 17:44:07 +0900 Subject: [PATCH 14/22] Fixes card view stacking context --- .../webapp/metrics/views/main_view/card_grid_component.scss | 1 - 1 file changed, 1 deletion(-) diff --git a/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss b/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss index 4923c73223..48ffef5da6 100644 --- a/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss +++ b/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss @@ -48,7 +48,6 @@ card-view { @include tb-theme-foreground-prop(border, border, 1px solid); border-radius: 4px; box-sizing: border-box; - contain: layout; display: block; // Make sure the cards generally have some guaranteed minimum height to avoid // jank as cards load. From 62f409920dc2ca5045bc607da575a74cdd772365 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Mon, 29 Jan 2024 18:58:24 +0900 Subject: [PATCH 15/22] Applies fixes --- tensorboard/webapp/metrics/actions/index.ts | 4 ++-- .../webapp/metrics/store/metrics_reducers.ts | 2 +- .../metrics/store/metrics_reducers_test.ts | 4 ++-- .../card_renderer/scalar_card_container.ts | 13 +++---------- .../views/card_renderer/scalar_card_test.ts | 10 +++++----- .../scalar_column_editor_component.ts | 16 +++------------- .../scalar_column_editor_container.ts | 17 +++++------------ .../scalar_column_editor_test.ts | 6 +++--- 8 files changed, 24 insertions(+), 48 deletions(-) diff --git a/tensorboard/webapp/metrics/actions/index.ts b/tensorboard/webapp/metrics/actions/index.ts index a7f8f52d10..55cf464875 100644 --- a/tensorboard/webapp/metrics/actions/index.ts +++ b/tensorboard/webapp/metrics/actions/index.ts @@ -234,8 +234,8 @@ export const sortingDataTable = createAction( props() ); -export const dataTableColumnEdited = createAction( - '[Metrics] Data table columns edited in edit menu', +export const dataTableColumnOrderChanged = createAction( + '[Metrics] Data table columns order changed', props() ); diff --git a/tensorboard/webapp/metrics/store/metrics_reducers.ts b/tensorboard/webapp/metrics/store/metrics_reducers.ts index db43c38914..956af53f6b 100644 --- a/tensorboard/webapp/metrics/store/metrics_reducers.ts +++ b/tensorboard/webapp/metrics/store/metrics_reducers.ts @@ -1432,7 +1432,7 @@ const reducer = createReducer( }; }), on( - actions.dataTableColumnEdited, + actions.dataTableColumnOrderChanged, (state, {source, destination, side, dataTableMode}) => { let headers = dataTableMode === DataTableMode.RANGE diff --git a/tensorboard/webapp/metrics/store/metrics_reducers_test.ts b/tensorboard/webapp/metrics/store/metrics_reducers_test.ts index c4538580ca..6631f580af 100644 --- a/tensorboard/webapp/metrics/store/metrics_reducers_test.ts +++ b/tensorboard/webapp/metrics/store/metrics_reducers_test.ts @@ -1601,7 +1601,7 @@ describe('metrics reducers', () => { const nextState = reducers( beforeState, - actions.dataTableColumnEdited({ + actions.dataTableColumnOrderChanged({ source: { type: ColumnHeaderType.END_VALUE, name: 'endValue', @@ -1742,7 +1742,7 @@ describe('metrics reducers', () => { const nextState = reducers( beforeState, - actions.dataTableColumnEdited({ + actions.dataTableColumnOrderChanged({ source: { type: ColumnHeaderType.STEP, name: 'step', diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts index 013ca742c7..df0c33d611 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts @@ -71,13 +71,13 @@ import {Extent} from '../../../widgets/line_chart_v2/lib/public_types'; import {ScaleType} from '../../../widgets/line_chart_v2/types'; import { cardViewBoxChanged, - dataTableColumnEdited, metricsCardFullSizeToggled, metricsCardStateUpdated, sortingDataTable, stepSelectorToggled, timeSelectionChanged, metricsSlideoutMenuOpened, + dataTableColumnOrderChanged, } from '../../actions'; import {PluginType, ScalarStepDatum} from '../../data_source'; import { @@ -674,15 +674,8 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { ); } - editColumnHeaders({ - source, - destination, - side, - dataTableMode, - }: HeaderEditInfo) { - this.store.dispatch( - dataTableColumnEdited({source, destination, side, dataTableMode}) - ); + editColumnHeaders(headerEditInfo: HeaderEditInfo) { + this.store.dispatch(dataTableColumnOrderChanged(headerEditInfo)); } openTableEditMenuToMode(tableMode: DataTableMode) { diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts index d761390179..caf82d14aa 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts @@ -81,7 +81,7 @@ import { stepSelectorToggled, timeSelectionChanged, metricsSlideoutMenuOpened, - dataTableColumnEdited, + dataTableColumnOrderChanged, dataTableColumnToggled, } from '../../actions'; import {PluginType} from '../../data_source'; @@ -4412,7 +4412,7 @@ describe('scalar card', () => { expect(dataTableComponent).toBeFalsy(); })); - it('emits dataTableColumnEdited with DataTableMode.SINGLE when orderColumns is called while in Single Selection', fakeAsync(() => { + it('emits dataTableColumnOrderChanged with DataTableMode.SINGLE when orderColumns is called while in Single Selection', fakeAsync(() => { store.overrideSelector(getCardStateMap, { card1: { dataMinMax: { @@ -4452,14 +4452,14 @@ describe('scalar card', () => { ); expect(dispatchedActions).toEqual([ - dataTableColumnEdited({ + dataTableColumnOrderChanged({ ...reorderColumnEvent, dataTableMode: DataTableMode.SINGLE, }), ]); })); - it('emits dataTableColumnEdited with DataTableMode.RANGE when orderColumns is called while in Range Selection', fakeAsync(() => { + it('emits dataTableColumnOrderChanged with DataTableMode.RANGE when orderColumns is called while in Range Selection', fakeAsync(() => { store.overrideSelector(getCardStateMap, { card1: { dataMinMax: { @@ -4499,7 +4499,7 @@ describe('scalar card', () => { ); expect(dispatchedActions).toEqual([ - dataTableColumnEdited({ + dataTableColumnOrderChanged({ ...reorderColumnEvent, dataTableMode: DataTableMode.RANGE, }), diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts index 50aeb4af09..02ffd555fd 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.ts @@ -76,13 +76,6 @@ export class ScalarColumnEditorComponent implements OnDestroy { ); } - getHeaderByName( - headers: ColumnHeader[], - name: string - ): ColumnHeader | undefined { - return headers.find((header) => header.name === name); - } - tabChange(event: MatTabChangeEvent) { const newMode = event.index === 0 ? DataTableMode.SINGLE : DataTableMode.RANGE; @@ -99,12 +92,9 @@ export class ScalarColumnEditorComponent implements OnDestroy { return; } const headers = this.getHeadersForMode(dataTableMode); - const source = this.getHeaderByName(headers, this.draggingHeader.name); - const destination = this.getHeaderByName( - headers, - this.highlightedHeader.name - ); - if (source && destination && source !== destination) { + const source = {...this.draggingHeader}; + const destination = {...this.highlightedHeader}; + if (source && destination && source.name !== destination.name) { this.onScalarTableColumnEdit.emit({ source, destination, diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts index 58dcaa9f6f..4b97cc9c8e 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts @@ -16,7 +16,7 @@ import {ChangeDetectionStrategy, Component} from '@angular/core'; import {Store} from '@ngrx/store'; import {State} from '../../../../app_state'; import { - dataTableColumnEdited, + dataTableColumnOrderChanged, dataTableColumnToggled, metricsSlideoutMenuClosed, tableEditorTabChanged, @@ -64,19 +64,12 @@ export class ScalarColumnEditorContainer { .pipe(map(headersWithoutRuns)); readonly selectedTab$ = this.store.select(getTableEditorSelectedTab); - onScalarTableColumnToggled({dataTableMode, header}: HeaderToggleInfo) { - this.store.dispatch(dataTableColumnToggled({dataTableMode, header})); + onScalarTableColumnToggled(toggleInfo: HeaderToggleInfo) { + this.store.dispatch(dataTableColumnToggled(toggleInfo)); } - onScalarTableColumnEdit({ - source, - destination, - side, - dataTableMode, - }: HeaderEditInfo) { - this.store.dispatch( - dataTableColumnEdited({source, destination, side, dataTableMode}) - ); + onScalarTableColumnEdit(editInfo: HeaderEditInfo) { + this.store.dispatch(dataTableColumnOrderChanged(editInfo)); } onScalarTableColumnEditorClosed() { diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_test.ts b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_test.ts index ba5bee7d45..b36c9f94fa 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_test.ts +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_test.ts @@ -26,7 +26,7 @@ import {Action, Store} from '@ngrx/store'; import {MockStore, provideMockStore} from '@ngrx/store/testing'; import {State} from '../../../../app_state'; import { - dataTableColumnEdited, + dataTableColumnOrderChanged, dataTableColumnToggled, metricsSlideoutMenuClosed, tableEditorTabChanged, @@ -359,7 +359,7 @@ describe('scalar column editor', () => { headerListItems[0].triggerEventHandler('dragend'); expect(dispatchedActions[0]).toEqual( - dataTableColumnEdited({ + dataTableColumnOrderChanged({ source: { type: ColumnHeaderType.SMOOTHED, name: 'smoothed', @@ -412,7 +412,7 @@ describe('scalar column editor', () => { headerListItems[1].triggerEventHandler('dragend'); expect(dispatchedActions[0]).toEqual( - dataTableColumnEdited({ + dataTableColumnOrderChanged({ source: { type: ColumnHeaderType.MAX_VALUE, name: 'maxValue', From e96b32ad345744c9e2f5ac1d3a9f53d13f1de530 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Mon, 29 Jan 2024 19:03:56 +0900 Subject: [PATCH 16/22] Fixes lint error --- tensorboard/webapp/widgets/data_table/types.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tensorboard/webapp/widgets/data_table/types.ts b/tensorboard/webapp/widgets/data_table/types.ts index 92bc7a5150..83670ccb76 100644 --- a/tensorboard/webapp/widgets/data_table/types.ts +++ b/tensorboard/webapp/widgets/data_table/types.ts @@ -134,5 +134,5 @@ export enum ColumnGroup { RUN = 'RUN', EXPERIMENT_ALIAS = 'EXPERIMENT_ALIAS', HPARAM = 'HPARAM', - OTHER = 'OTHER' -} \ No newline at end of file + OTHER = 'OTHER', +} From 7f39b545fab907b6e51ee813f5816819e5549a15 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Mon, 29 Jan 2024 19:15:33 +0900 Subject: [PATCH 17/22] Fixes lint error --- tensorboard/webapp/widgets/data_table/types.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tensorboard/webapp/widgets/data_table/types.ts b/tensorboard/webapp/widgets/data_table/types.ts index 92bc7a5150..83670ccb76 100644 --- a/tensorboard/webapp/widgets/data_table/types.ts +++ b/tensorboard/webapp/widgets/data_table/types.ts @@ -134,5 +134,5 @@ export enum ColumnGroup { RUN = 'RUN', EXPERIMENT_ALIAS = 'EXPERIMENT_ALIAS', HPARAM = 'HPARAM', - OTHER = 'OTHER' -} \ No newline at end of file + OTHER = 'OTHER', +} From 8b2df63df2f67f195cbce965fc2def94c47793be Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Mon, 29 Jan 2024 22:45:46 +0900 Subject: [PATCH 18/22] Fixes --- .../scalar_column_editor_component.scss | 10 +++++----- .../scalar_column_editor_container.ts | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.scss b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.scss index 08ba59b01e..43133547d6 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.scss +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_component.scss @@ -91,6 +91,11 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent); display: flex; align-items: center; margin: 10px 10px 0; + + &-add-button { + height: 30px; + width: 30px; + } } &-title { @@ -98,9 +103,4 @@ $_accent: map-get(mat.get-color-config($tb-theme), accent); font-size: 14px; font-weight: normal; } - - &-add-button { - height: 30px; - width: 30px; - } } diff --git a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts index e5d4e23cdd..adf9f988d1 100644 --- a/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts +++ b/tensorboard/webapp/metrics/views/right_pane/scalar_column_editor/scalar_column_editor_container.ts @@ -106,7 +106,7 @@ export class ScalarColumnEditorContainer { ); } else { this.store.dispatch( - dataTableColumnEdited({source, destination, side, dataTableMode}) + dataTableColumnOrderChanged({source, destination, side, dataTableMode}) ); } } From dd390f02266d5d932a4d3ca6ac8dc999448e34b4 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Mon, 29 Jan 2024 22:46:50 +0900 Subject: [PATCH 19/22] Revert card view layout paint --- .../webapp/metrics/views/main_view/card_grid_component.scss | 1 + 1 file changed, 1 insertion(+) diff --git a/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss b/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss index 48ffef5da6..224b5ce440 100644 --- a/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss +++ b/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss @@ -49,6 +49,7 @@ card-view { border-radius: 4px; box-sizing: border-box; display: block; + contain: layout paint; // Make sure the cards generally have some guaranteed minimum height to avoid // jank as cards load. min-height: $metrics-min-card-height; From 0e210b25f9136a77801fe58b89de1ff8b78c2f03 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Mon, 29 Jan 2024 22:57:10 +0900 Subject: [PATCH 20/22] More fixes --- .../webapp/metrics/views/main_view/card_grid_component.scss | 1 + 1 file changed, 1 insertion(+) diff --git a/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss b/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss index 48ffef5da6..224b5ce440 100644 --- a/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss +++ b/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss @@ -49,6 +49,7 @@ card-view { border-radius: 4px; box-sizing: border-box; display: block; + contain: layout paint; // Make sure the cards generally have some guaranteed minimum height to avoid // jank as cards load. min-height: $metrics-min-card-height; From 8f95404aa8cbb452c72b2b2389f22955408342cd Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Mon, 29 Jan 2024 23:10:10 +0900 Subject: [PATCH 21/22] Move contain layout paint to original position --- .../webapp/metrics/views/main_view/card_grid_component.scss | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss b/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss index 224b5ce440..c12d11cf3b 100644 --- a/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss +++ b/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss @@ -48,8 +48,8 @@ card-view { @include tb-theme-foreground-prop(border, border, 1px solid); border-radius: 4px; box-sizing: border-box; - display: block; contain: layout paint; + display: block; // Make sure the cards generally have some guaranteed minimum height to avoid // jank as cards load. min-height: $metrics-min-card-height; From 5fe35515a72b5392603938e501ab96f44a963852 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Mon, 29 Jan 2024 23:12:21 +0900 Subject: [PATCH 22/22] Fix strange merge --- .../webapp/metrics/views/main_view/card_grid_component.scss | 1 - 1 file changed, 1 deletion(-) diff --git a/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss b/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss index 38316a183b..c12d11cf3b 100644 --- a/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss +++ b/tensorboard/webapp/metrics/views/main_view/card_grid_component.scss @@ -50,7 +50,6 @@ card-view { box-sizing: border-box; contain: layout paint; display: block; - contain: layout paint; // Make sure the cards generally have some guaranteed minimum height to avoid // jank as cards load. min-height: $metrics-min-card-height;