Skip to content

Commit

Permalink
Step Selector: Add Range enabled boolean to state and update based ch…
Browse files Browse the repository at this point in the history
…eckbox (tensorflow#5927)

Motivation for features / changes
This is part of the effort to enable range selection in an upcoming launch.

Technical description of changes
This simply follows the same pattern used for other settings such as the step selector.
  • Loading branch information
JamesHollyer authored and yatbear committed Mar 27, 2023
1 parent 8f0348e commit 697d7fe
Show file tree
Hide file tree
Showing 10 changed files with 130 additions and 2 deletions.
9 changes: 8 additions & 1 deletion tensorboard/webapp/metrics/actions/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,13 @@ export const stepSelectorToggled = createAction(
affordance?: TimeSelectionToggleAffordance;
}>()
);

export const stepSelectorRangeToggled = createAction(
'[Metrics] Time Selector Range Toggle',
props<{
// Affordance for internal analytics purpose. When no affordance is specified or is
// undefined we do not want to log an analytics event.
affordance?: TimeSelectionToggleAffordance;
}>()
);
// TODO(jieweiwu): Delete after internal code is updated.
export const stepSelectorTimeSelectionChanged = timeSelectionChanged;
15 changes: 15 additions & 0 deletions tensorboard/webapp/metrics/store/metrics_reducers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,7 @@ const {initialState, reducers: namespaceContextedReducer} =
linkedTimeSelection: null,
linkedTimeEnabled: false,
stepSelectorEnabled: false,
stepSelectorRangeEnabled: false,
linkedTimeRangeEnabled: false,
filteredPluginTypes: new Set(),
stepMinMax: {
Expand Down Expand Up @@ -978,6 +979,20 @@ const reducer = createReducer(
stepSelectorEnabled: nextStepSelectorEnabled,
};
}),
on(actions.stepSelectorRangeToggled, (state) => {
const nextStepSelectorRangeEnabled = !state.stepSelectorRangeEnabled;
let nextStepSelectorEnabled = state.stepSelectorEnabled;

if (nextStepSelectorRangeEnabled) {
nextStepSelectorEnabled = nextStepSelectorRangeEnabled;
}

return {
...state,
stepSelectorEnabled: nextStepSelectorEnabled,
stepSelectorRangeEnabled: nextStepSelectorRangeEnabled,
};
}),
on(actions.timeSelectionChanged, (state, change) => {
const {timeSelection} = change;
const nextStartStep = timeSelection.start.step;
Expand Down
36 changes: 36 additions & 0 deletions tensorboard/webapp/metrics/store/metrics_reducers_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2724,6 +2724,42 @@ describe('metrics reducers', () => {
});
});

describe('#stepSelectorRangeToggled', () => {
it('toggles whether stepSelectorRange is enabled or not', () => {
const state1 = buildMetricsState({
stepSelectorRangeEnabled: false,
});

const state2 = reducers(state1, actions.stepSelectorRangeToggled({}));
expect(state2.stepSelectorRangeEnabled).toBe(true);

const state3 = reducers(state2, actions.stepSelectorRangeToggled({}));
expect(state3.stepSelectorRangeEnabled).toBe(false);
});

it('enables stepSelector if disabled when enabling stepSelectorRange', () => {
const state1 = buildMetricsState({
stepSelectorEnabled: false,
stepSelectorRangeEnabled: false,
});

const state2 = reducers(state1, actions.stepSelectorRangeToggled({}));
expect(state2.stepSelectorEnabled).toBe(true);
expect(state2.stepSelectorRangeEnabled).toBe(true);
});

it('keeps stepSelector enabled when disabling stepSelectorRange', () => {
const state1 = buildMetricsState({
stepSelectorEnabled: true,
stepSelectorRangeEnabled: true,
});

const state2 = reducers(state1, actions.stepSelectorRangeToggled({}));
expect(state2.stepSelectorEnabled).toBe(true);
expect(state2.stepSelectorRangeEnabled).toBe(false);
});
});

describe('#linkedTimeEnabled', () => {
const imageCardId = 'test image card id "plugin":"images"';
const cardMetadataMap = {
Expand Down
7 changes: 7 additions & 0 deletions tensorboard/webapp/metrics/store/metrics_selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,13 @@ export const getMetricsStepSelectorEnabled = createSelector(
}
);

export const getMetricsStepSelectorRangeEnabled = createSelector(
selectMetricsState,
(state: MetricsState): boolean => {
return state.stepSelectorRangeEnabled;
}
);

export const getMetricsLinkedTimeRangeEnabled = createSelector(
selectMetricsState,
(state: MetricsState): boolean => {
Expand Down
1 change: 1 addition & 0 deletions tensorboard/webapp/metrics/store/metrics_types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ export interface MetricsNamespacedState {
linkedTimeSelection: TimeSelection | null;
linkedTimeEnabled: boolean;
stepSelectorEnabled: boolean;
stepSelectorRangeEnabled: boolean;
linkedTimeRangeEnabled: boolean;
// Empty Set would signify "show all". `filteredPluginTypes` will never have
// all pluginTypes in the Set.
Expand Down
1 change: 1 addition & 0 deletions tensorboard/webapp/metrics/testing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ function buildBlankState(): MetricsState {
linkedTimeSelection: null,
linkedTimeEnabled: false,
stepSelectorEnabled: false,
stepSelectorRangeEnabled: false,
linkedTimeRangeEnabled: false,
filteredPluginTypes: new Set(),
stepMinMax: {min: Infinity, max: -Infinity},
Expand Down
41 changes: 41 additions & 0 deletions tensorboard/webapp/metrics/views/right_pane/right_pane_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,10 @@ describe('metrics right_pane', () => {
store.overrideSelector(selectors.getMetricsCardMinWidth, null);
store.overrideSelector(selectors.getMetricsLinkedTimeEnabled, false);
store.overrideSelector(selectors.getMetricsStepSelectorEnabled, false);
store.overrideSelector(
selectors.getMetricsStepSelectorRangeEnabled,
false
);
store.overrideSelector(selectors.getAllowRangeSelection, false);
store.overrideSelector(selectors.getMetricsLinkedTimeSelectionSetting, {
start: {step: 0},
Expand Down Expand Up @@ -665,6 +669,43 @@ describe('metrics right_pane', () => {
fixture.debugElement.query(By.css('.range-selection mat-checkbox'))
).toBeTruthy();
});

it('only enables checkbox when X Axis Type is Step', () => {
store.overrideSelector(selectors.getMetricsXAxisType, XAxisType.STEP);
const fixture = TestBed.createComponent(SettingsViewContainer);
fixture.detectChanges();
console.log(
'properties',
fixture.debugElement.query(By.css('.range-selection mat-checkbox'))
.properties
);
const checkbox = fixture.debugElement.query(
By.css('.range-selection mat-checkbox input')
);
expect(checkbox.properties['disabled']).toBe(false);

store.overrideSelector(
selectors.getMetricsXAxisType,
XAxisType.WALL_TIME
);
store.refreshState();
fixture.detectChanges();

expect(checkbox.properties['disabled']).toBe(true);
});

it('dispatches stepSelectorRangeEnableToggled on toggle', () => {
const fixture = TestBed.createComponent(SettingsViewContainer);
fixture.detectChanges();

select(fixture, '.range-selection input').nativeElement.click();

expect(dispatchSpy).toHaveBeenCalledWith(
actions.stepSelectorRangeToggled({
affordance: TimeSelectionToggleAffordance.CHECK_BOX,
})
);
});
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,10 @@ <h3 class="section-title">General</h3>
</mat-checkbox>
<span class="indent">(Scalars only)</span>
<div class="indent range-selection" *ngIf="isRangeSelectionAllowed">
<mat-checkbox [disabled]="!isAxisTypeStep()"
<mat-checkbox
[checked]="isScalarStepSelectorRangeEnabled"
(change)="stepSelectorRangeToggled.emit()"
[disabled]="!isAxisTypeStep()"
>Enable Range Selection
</mat-checkbox>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ export class SettingsViewComponent {
@Input() isLinkedTimeEnabled!: boolean;
@Input() isScalarStepSelectorFeatureEnabled!: boolean;
@Input() isScalarStepSelectorEnabled!: boolean;
@Input() isScalarStepSelectorRangeEnabled!: boolean;
@Input() linkedTimeSelection!: TimeSelection | null;
@Input() stepMinMax!: {min: number; max: number};

Expand All @@ -78,6 +79,7 @@ export class SettingsViewComponent {
linkedTimeSelectionChanged = new EventEmitter<LinkedTimeSelectionChanged>();

@Output() stepSelectorToggled = new EventEmitter<void>();
@Output() stepSelectorRangeToggled = new EventEmitter<void>();

@Input() isImageSupportEnabled!: boolean;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import {
metricsScalarPartitionNonMonotonicXToggled,
metricsToggleIgnoreOutliers,
metricsToggleImageShowActualSize,
stepSelectorRangeToggled,
stepSelectorToggled,
timeSelectionChanged,
} from '../../actions';
Expand Down Expand Up @@ -87,12 +88,16 @@ const RANGE_INPUT_SOURCE_TO_AFFORDANCE: Record<
isScalarStepSelectorFeatureEnabled$ | async
"
[isScalarStepSelectorEnabled]="isScalarStepSelectorEnabled$ | async"
[isScalarStepSelectorRangeEnabled]="
isScalarStepSelectorRangeEnabled$ | async
"
[isLinkedTimeEnabled]="isLinkedTimeEnabled$ | async"
[linkedTimeSelection]="linkedTimeSelection$ | async"
[stepMinMax]="stepMinMax$ | async"
(linkedTimeToggled)="onLinkedTimeToggled()"
(linkedTimeSelectionChanged)="onLinkedTimeSelectionChanged($event)"
(stepSelectorToggled)="onStepSelectorToggled()"
(stepSelectorRangeToggled)="onStepSelectorRangeToggled()"
>
</metrics-dashboard-settings-component>
`,
Expand All @@ -111,6 +116,8 @@ export class SettingsViewContainer {
this.store.select(selectors.getIsDataTableEnabled);
readonly isScalarStepSelectorEnabled$: Observable<boolean> =
this.store.select(selectors.getMetricsStepSelectorEnabled);
readonly isScalarStepSelectorRangeEnabled$: Observable<boolean> =
this.store.select(selectors.getMetricsStepSelectorRangeEnabled);
readonly isLinkedTimeEnabled$: Observable<boolean> = this.store.select(
selectors.getMetricsLinkedTimeEnabled
);
Expand Down Expand Up @@ -221,6 +228,14 @@ export class SettingsViewContainer {
);
}

onStepSelectorRangeToggled() {
this.store.dispatch(
stepSelectorRangeToggled({
affordance: TimeSelectionToggleAffordance.CHECK_BOX,
})
);
}

onLinkedTimeSelectionChanged({
timeSelection,
source,
Expand Down

0 comments on commit 697d7fe

Please sign in to comment.