Skip to content

Commit

Permalink
Changes reorder column events to use source and destination (#6733)
Browse files Browse the repository at this point in the history 
## Motivation for features / changes
To prepare for showing shared hparam columns across runs and scalar
tables, we need to unify the column re-ordering logic to use the same
source/destination parameters (introduced in #6727).

## Technical description of changes
- Changes the DataTable component (shared by runs and scalar tables) to
use the new ReorderColumnEvent type when column order is changed. The
changes propagate to scalar card scalar column editor containers. Most
of the changes are getting the tests to pass with the new signature.
- Changes the metrics reducers to correctly handle these new events.
Logic is mostly unchanged, except that sort order will now be preserved
even after column toggle (previously, toggling would group all disabled
columns together, which destroys relative column order

misc:
- creates moveColumn utility to abstract shared logic between the
hparams and metrics reducers. Removes redundant tests in hparams reducer

## Detailed steps to verify changes work correctly (as executed by you)
- Unit tests pass
- Manually tested sorting behavior
  • Loading branch information
@hoonji
hoonji authored Jan 31, 2024
1 parent 304046d commit 0d76a12
Show file tree
Hide file tree
Showing 27 changed files with 528 additions and 794 deletions.
2 changes: 2 additions & 0 deletions tensorboard/webapp/hparams/_redux/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
],
)
Expand Down Expand Up @@ -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",
Expand Down
29 changes: 7 additions & 22 deletions tensorboard/webapp/hparams/_redux/hparams_reducers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ 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';

Expand Down Expand Up @@ -141,28 +142,12 @@ const reducer: ActionReducer<HparamsState, Action> = 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,
Expand Down
107 changes: 13 additions & 94 deletions tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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({
Expand All @@ -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],
Expand Down
4 changes: 2 additions & 2 deletions tensorboard/webapp/metrics/actions/index.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -234,8 +234,8 @@ export const sortingDataTable = createAction(
props<SortingInfo>()
);

export const dataTableColumnEdited = createAction(
'[Metrics] Data table columns edited in edit menu',
export const dataTableColumnOrderChanged = createAction(
'[Metrics] Data table columns order changed',
props<HeaderEditInfo>()
);

Expand Down
7 changes: 3 additions & 4 deletions tensorboard/webapp/metrics/internal_types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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;
Expand Down
102 changes: 26 additions & 76 deletions tensorboard/webapp/metrics/store/metrics_reducers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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[] = [];
Expand Down Expand Up @@ -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.dataTableColumnOrderChanged,
(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(
Expand All @@ -1471,40 +1464,24 @@ 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 {
...state,
rangeSelectionHeaders: newHeaders,
};
}

return {
...state,
singleSelectionHeaders: newHeaders,
Expand Down Expand Up @@ -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;
}
Loading

0 comments on commit 0d76a12

Please sign in to comment.