Adds redux code for displaying shared hparam columns in the Time Series page #6727
Conversation
hoonji
changed the title
| if (destinationIndex === -1) { | ||
| destinationIndex = side === Side.LEFT ? 0 : columns.length - 1; | ||
| } | ||
|
|
||
| const newColumns = [...columns]; | ||
| newColumns.splice(sourceIndex, 1); | ||
| newColumns.splice(destinationIndex, 0, source); |
There was a problem hiding this comment.
If the destination IS found, the side is not taken into account.
Heads up, if side IS accounted for you will likely run into an issue with way you derive the indexes, then mutate the array.
There was a problem hiding this comment.
If the destination IS found, the side is not taken into account.
Yeah this is WAI: in typical cases, we just swap source with dest - no Side required. Side is essentially used as a backup for when dest is not found. This allows us to move columns to the end of a valid "column group" as in this gif: 
I'll add additional conditional logic to make sure this only triggers when dest is not found AND side exists, and a comment to clarify this behavior.
PLMK if this seems unnecessarily convoluted - I'm also open to simply removing the side param and simply doing nothing if dest isn't found. (IMO reordering elements to the end of the column group should be rather intuitive though)
## Motivation for features / changes Scalar card table columns are [currently persisted to localstorage](https://github.com/tensorflow/tensorboard/blob/28be621b99e1bb2595b39d915055c32ca986a805/tensorboard/webapp/metrics/metrics_module.ts#L174-L179); it should be reasonable to save selected hparam columns to localstorage as well. ## Technical description of changes Adds a new `dashboardDisplayedHparamColumns` PersistableSetting to save selected hparam columns (similar to the existing `singleSelectionHeaders` and `rangeSelectionHeaders` settings) ## Detailed steps to verify changes work correctly (as executed by you) Unit tests pass
## 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
Motivation for features / changes
Required redux edits for displaying hparams in time series.
Technical description of changes
Adds a new property to HparamsState,
dashboardDisplayedHparamColumnsto represent the columns shown across the time series page (similar in concept to the existingdashboardFilters)Adds four actions for hparam column manipulation:
and reducer logic to handle these events.
Note that the add actions cannot simply pass the header index in this case as the table components will not have knowledge of the relative index of hparam columns. The actions instead specify the column to insert next to and the side, and the indices are computed from within the reducer (this can be seen as simply moving the index computation from the component to the reducer, which better conforms to https://redux.js.org/style-guide/#put-as-much-logic-as-possible-in-reducers )
Also exports required types from widgets/data_table - these will be also used by the runs and metrics tables the following PRs
Detailed steps to verify changes work correctly (as executed by you)
Alternate designs / implementations considered (or N/A)
An alternative is to duplicate the hparams state across the runs and scalar card tables as RunsDataState.runsTableHeaders and MetricsState.singleSelectionHeaders/MetricsState.rangeSelectionHeaders, but this would either incur massive complications in syncing the various column operations (add/remove/hide/sort) or alternatively, would force the user to duplicate operations multiple times to get the hparams columns in sync. The chosen design also aligns with keeping state minimal.