Skip to content

Adds redux code for displaying shared hparam columns in the Time Series page #6725

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from

Conversation

hoonji
Copy link
Member

@hoonji hoonji commented Jan 22, 2024

Motivation for features / changes

Required redux edits for displaying hparams in time series.

Technical description of changes

Adds a new property to HparamsState, dashboardDisplayedHparamColumns to represent the columns shown across the time series page (similar in concept to the existing dashboardFilters)

Adds four actions for hparam column manipulation:

  • dashboardHparamColumnAdded
  • dashboardHparamColumnRemoved
  • dashboardHparamColumnToggled
  • dashboardHparamColumnOrderChanged

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)

  • Unit tests pass

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.

},
];

it('appends an hparam column to the end', () => {
Copy link
Member Author

@hoonji hoonji Jan 22, 2024

Choose a reason for hiding this comment

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

QQ: is there a recommended way to do parameterized testing in 3p code? Would something like the example in https://stackoverflow.com/questions/38659880/jasmine-parameterized-unit-test be acceptable? I haven't found any such examples in the codebase so far

Namely:

it('action(value) should reset the forms pool only if value is true', () => {
  [
    { value: true, calledTimes: 1 },
    { value: false, calledTimes: 0 },
  ].forEach(({ value, calledTimes }) => {
    spyResetFormsPool.calls.reset();

    component.action(value);

    expect(spyResetFormsPool).toHaveBeenCalledTimes(calledTimes);
  });
});

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant