Skip to content

Commit

Permalink
HParams: Add effect to remove hparam filters when the related column …
Browse files Browse the repository at this point in the history 
…is removed (#6572)

## Motivation for features / changes
We found it confusing that a user could remove an hparam column but
still have related filters in place. This also lead to awkward UX where
they would need to re-add the column in order to remove the filter.

## Technical description of changes
Since the action is a runs action I added an effect to runs effects
which dispatches a newly created action to clear out related hparam
filters.
  • Loading branch information
@rileyajones
rileyajones authored Sep 20, 2023
1 parent cc6e294 commit 718aad2
Show file tree
Hide file tree
Showing 7 changed files with 229 additions and 0 deletions.
10 changes: 10 additions & 0 deletions tensorboard/webapp/hparams/_redux/hparams_actions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,13 @@ export const dashboardMetricFilterAdded = createAction(
'[Hparams] Dashboard Metric Filter Added',
props<{name: string; filter: MetricFilter}>()
);

export const dashboardHparamFilterRemoved = createAction(
'[Hparams] Dashboard Hparam Filter Removed',
props<{name: string}>()
);

export const dashboardMetricFilterRemoved = createAction(
'[Hparams] Dashboard Metric Filter Removed',
props<{name: string}>()
);
24 changes: 24 additions & 0 deletions tensorboard/webapp/hparams/_redux/hparams_reducers.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,30 @@ const reducer: ActionReducer<HparamsState, Action> = createReducer(
const nextMetricFilters = new Map(state.dashboardFilters.metrics);
nextMetricFilters.set(action.name, action.filter);

return {
...state,
dashboardFilters: {
...state.dashboardFilters,
metrics: nextMetricFilters,
},
};
}),
on(actions.dashboardHparamFilterRemoved, (state, action) => {
const nextHparamFilters = new Map(state.dashboardFilters.hparams);
nextHparamFilters.delete(action.name);

return {
...state,
dashboardFilters: {
...state.dashboardFilters,
hparams: nextHparamFilters,
},
};
}),
on(actions.dashboardMetricFilterRemoved, (state, action) => {
const nextMetricFilters = new Map(state.dashboardFilters.metrics);
nextMetricFilters.delete(action.name);

return {
...state,
dashboardFilters: {
Expand Down
110 changes: 110 additions & 0 deletions tensorboard/webapp/hparams/_redux/hparams_reducers_test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -752,4 +752,114 @@ describe('hparams/_redux/hparams_reducers_test', () => {
});
});
});

describe('dashboardHparamFilterRemoved', () => {
it('removes entry from dashboardFilters', () => {
const state = buildHparamsState({
dashboardFilters: {
hparams: new Map([
[
'hparam1',
{
type: DomainType.INTERVAL,
includeUndefined: true,
minValue: 5,
maxValue: 10,
filterLowerBound: 5,
filterUpperBound: 10,
},
],
[
'hparam2',
{
type: DomainType.INTERVAL,
includeUndefined: true,
minValue: 2,
maxValue: 20,
filterLowerBound: 2,
filterUpperBound: 20,
},
],
]),
},
});
const state2 = reducers(
state,
actions.dashboardHparamFilterRemoved({
name: 'hparam1',
})
);
expect(state2.dashboardFilters).toEqual({
hparams: new Map([
[
'hparam2',
{
type: DomainType.INTERVAL,
includeUndefined: true,
minValue: 2,
maxValue: 20,
filterLowerBound: 2,
filterUpperBound: 20,
},
],
]),
metrics: new Map(),
});
});
});

describe('dashboardMetricFilterRemoved', () => {
it('removes entry from dashboardFilters', () => {
const state = buildHparamsState({
dashboardFilters: {
metrics: new Map([
[
'metric 1',
{
type: DomainType.INTERVAL,
includeUndefined: true,
minValue: 5,
maxValue: 10,
filterLowerBound: 5,
filterUpperBound: 10,
},
],
[
'metric 2',
{
type: DomainType.INTERVAL,
includeUndefined: true,
minValue: 2,
maxValue: 20,
filterLowerBound: 2,
filterUpperBound: 20,
},
],
]),
},
});
const state2 = reducers(
state,
actions.dashboardMetricFilterRemoved({
name: 'metric 1',
})
);
expect(state2.dashboardFilters).toEqual({
hparams: new Map(),
metrics: new Map([
[
'metric 2',
{
type: DomainType.INTERVAL,
includeUndefined: true,
minValue: 2,
maxValue: 20,
filterLowerBound: 2,
filterUpperBound: 20,
},
],
]),
});
});
});
});
4 changes: 4 additions & 0 deletions tensorboard/webapp/runs/effects/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,12 @@ tf_ng_module(
"//tensorboard/webapp/app_routing:types",
"//tensorboard/webapp/app_routing/actions",
"//tensorboard/webapp/core/actions",
"//tensorboard/webapp/hparams/_redux:hparams_actions",
"//tensorboard/webapp/runs:types",
"//tensorboard/webapp/runs/actions",
"//tensorboard/webapp/runs/data_source",
"//tensorboard/webapp/types",
"//tensorboard/webapp/widgets/data_table:types",
"@npm//@angular/core",
"@npm//@ngrx/effects",
"@npm//@ngrx/store",
Expand All @@ -43,11 +45,13 @@ tf_ts_library(
"//tensorboard/webapp/app_routing:testing",
"//tensorboard/webapp/app_routing:types",
"//tensorboard/webapp/core/actions",
"//tensorboard/webapp/hparams/_redux:hparams_actions",
"//tensorboard/webapp/runs/actions",
"//tensorboard/webapp/runs/data_source",
"//tensorboard/webapp/runs/data_source:testing",
"//tensorboard/webapp/testing:utils",
"//tensorboard/webapp/types",
"//tensorboard/webapp/widgets/data_table:types",
"@npm//@ngrx/effects",
"@npm//@ngrx/store",
"@npm//@types/jasmine",
Expand Down
32 changes: 32 additions & 0 deletions tensorboard/webapp/runs/effects/runs_effects.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,12 +39,14 @@ import {
} from '../../selectors';
import {DataLoadState, LoadState} from '../../types/data';
import * as actions from '../actions';
import * as hparamsActions from '../../hparams/_redux/hparams_actions';
import {
HparamsAndMetadata,
Run,
RunsDataSource,
} from '../data_source/runs_data_source_types';
import {ExperimentIdToRunsAndMetadata} from '../types';
import {ColumnHeaderType} from '../../widgets/data_table/types';

/**
* Runs effect for fetching data from the backend.
Expand Down Expand Up @@ -172,6 +174,36 @@ export class RunsEffects {
{dispatch: false}
);

/**
* Removes hparam filter when column is removed.
*
* @export
*/
removeHparamFilterWhenColumnIsRemoved$ = createEffect(
() =>
this.actions$.pipe(
ofType(actions.runsTableHeaderRemoved),
tap(({header}) => {
if (header.type === ColumnHeaderType.HPARAM) {
this.store.dispatch(
hparamsActions.dashboardHparamFilterRemoved({
name: header.name,
})
);
return;
}
if (header.type === ColumnHeaderType.METRIC) {
this.store.dispatch(
hparamsActions.dashboardMetricFilterRemoved({
name: header.name,
})
);
}
})
),
{dispatch: false}
);

/**
* IMPORTANT: actions are dispatched even when there are no experiments to
* fetch.
Expand Down
48 changes: 48 additions & 0 deletions tensorboard/webapp/runs/effects/runs_effects_test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
import {RouteKind} from '../../app_routing/types';
import {State} from '../../app_state';
import * as coreActions from '../../core/actions';
import * as hparamsActions from '../../hparams/_redux/hparams_actions';
import {
getActiveRoute,
getExperimentIdsFromRoute,
Expand All @@ -42,6 +43,7 @@ import {
TestingRunsDataSource,
} from '../data_source/testing';
import {RunsEffects} from './index';
import {ColumnHeaderType} from '../../widgets/data_table/types';

function createRun(override: Partial<Run> = {}) {
return {
Expand Down Expand Up @@ -947,4 +949,50 @@ describe('runs_effects', () => {
});
});
});

describe('removeHparamFilterWhenColumnIsRemoved$', () => {
beforeEach(() => {
effects.removeHparamFilterWhenColumnIsRemoved$.subscribe(() => {});
});

it('dispatches dashboardHparamFilterRemoved when column type is hparam', () => {
action.next(
actions.runsTableHeaderRemoved({
header: {
type: ColumnHeaderType.HPARAM,
name: 'some_hparam',
enabled: true,
displayName: 'Some Hparam',
},
})
);
store.refreshState();

expect(actualActions).toEqual([
hparamsActions.dashboardHparamFilterRemoved({
name: 'some_hparam',
}),
]);
});

it('dispatches dashboardMetricFilterRemoved when column type is metric', () => {
action.next(
actions.runsTableHeaderRemoved({
header: {
type: ColumnHeaderType.METRIC,
name: 'some_metric',
enabled: true,
displayName: 'Some Metric',
},
})
);
store.refreshState();

expect(actualActions).toEqual([
hparamsActions.dashboardMetricFilterRemoved({
name: 'some_metric',
}),
]);
});
});
});
1 change: 1 addition & 0 deletions tensorboard/webapp/widgets/data_table/types.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ export enum ColumnHeaderType {
MEAN = 'MEAN',
RAW_CHANGE = 'RAW_CHANGE',
HPARAM = 'HPARAM',
METRIC = 'METRIC',
CUSTOM = 'CUSTOM',
}

Expand Down

0 comments on commit 718aad2

Please sign in to comment.