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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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 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 08/11] 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 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 09/11] 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 10/11] 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 8f4a38bf385c9ef3390034021e7986ae1a9ca934 Mon Sep 17 00:00:00 2001 From: Hoonji <736199+hoonji@users.noreply.github.com> Date: Wed, 31 Jan 2024 15:48:52 +0900 Subject: [PATCH 11/11] Rename dataTableColumnEdited to dataTableColumnOrderChanged in tests --- tensorboard/webapp/metrics/store/metrics_reducers_test.ts | 2 +- .../scalar_column_editor/scalar_column_editor_test.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tensorboard/webapp/metrics/store/metrics_reducers_test.ts b/tensorboard/webapp/metrics/store/metrics_reducers_test.ts index 6631f580af..56db73a6d7 100644 --- a/tensorboard/webapp/metrics/store/metrics_reducers_test.ts +++ b/tensorboard/webapp/metrics/store/metrics_reducers_test.ts @@ -1536,7 +1536,7 @@ describe('metrics reducers', () => { expect(nextState.timeSeriesData).toEqual(createTimeSeriesData()); }); - describe('dataTableColumnEdited', () => { + describe('dataTableColumnOrderChanged', () => { it('edits range selection when dataTableMode is range', () => { const beforeState = buildMetricsState({ rangeSelectionHeaders: [ 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 b36c9f94fa..d8c781b78d 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 @@ -325,7 +325,7 @@ describe('scalar column editor', () => { }); }); - it('dispatches dataTableColumnEdited action with single selection when header is dragged', fakeAsync(() => { + it('dispatches dataTableColumnOrderChanged action with single selection when header is dragged', fakeAsync(() => { store.overrideSelector(getSingleSelectionHeaders, [ { type: ColumnHeaderType.SMOOTHED, @@ -378,7 +378,7 @@ describe('scalar column editor', () => { ); })); - it('dispatches dataTableColumnEdited action with range selection when header is dragged', fakeAsync(() => { + it('dispatches dataTableColumnOrderChanged action with range selection when header is dragged', fakeAsync(() => { store.overrideSelector(getRangeSelectionHeaders, [ { type: ColumnHeaderType.SMOOTHED,