Skip to content

Commit e49a86f

Browse files
rileyajonesrileyajones
authored
Bug Fix: Prevent toggling range from resetting time selection (#5969)
* Motivation for features / changes When toggling range selection the remaining fob was being set to the min value in the range. * Screenshots of UI changes Range Enabled: ![image](https://user-images.githubusercontent.com/78179109/195214454-bf2c3461-ea33-45b0-9210-8eb2171b6c8b.png) Range Disabled: ![image](https://user-images.githubusercontent.com/78179109/195214485-64236a20-5967-4ddd-992a-7822e3506059.png) See it in action ![69353c11-7537-4b81-9bf7-441c6b34fee1](https://user-images.githubusercontent.com/78179109/195214562-e1cf16f7-6960-4fa1-8314-c463c92cc525.gif) Zooming Still Works: ![2ff41be9-598c-4bfb-8959-c4d7073b3870](https://user-images.githubusercontent.com/78179109/195214607-b057147c-bb20-411f-b9e6-89c211e61758.gif) * Detailed steps to verify changes work correctly (as executed by you) 1) Start tensorboard 2) Go to localhost:6006?enableRangeSelection 3) Enable range selection in the settings panel 4) Move both fobs on the scalar card 5) Disable range selection either via the checkbox or by removing a fob 6) Assert the remaining fob is not effected. 8) Renable range selection 7) Zoom in on the chart and assert both fobs are visible.
1 parent bc2a761 commit e49a86f

File tree

4 files changed

+117
-4
lines changed

4 files changed

+117
-4
lines changed

‎tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,7 @@ import {
100100
SortingInfo,
101101
} from './scalar_card_types';
102102
import {
103+
clipStepWithinMinMax,
103104
maybeClipLinkedTimeSelection,
104105
partitionSeries,
105106
TimeSelectionView,
@@ -604,12 +605,19 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy {
604605
this.store.select(getMetricsStepSelectorEnabled),
605606
this.store.select(getMetricsRangeSelectionEnabled),
606607
]).subscribe(
607-
([{minStep, maxStep}, enableStepSelector, stepSelectorRangeEnabled]) => {
608+
([{minStep, maxStep}, enableStepSelector, rangeSelectionEnabled]) => {
608609
this.stepSelectorTimeSelection$.next(
609610
enableStepSelector
610611
? {
611-
start: {step: minStep},
612-
end: stepSelectorRangeEnabled ? {step: maxStep} : null,
612+
start: {
613+
step: clipStepWithinMinMax(
614+
this.stepSelectorTimeSelection$.getValue()?.start.step ??
615+
minStep,
616+
minStep,
617+
maxStep
618+
),
619+
},
620+
end: rangeSelectionEnabled ? {step: maxStep} : null,
613621
}
614622
: null
615623
);

‎tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts

Lines changed: 85 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -80,6 +80,7 @@ import {
8080
getMetricsLinkedTimeSelection,
8181
getMetricsRangeSelectionEnabled,
8282
getMetricsScalarSmoothing,
83+
getMetricsStepSelectorEnabled,
8384
} from '../../store';
8485
import {
8586
appStateFromMetricsState,
@@ -2154,6 +2155,90 @@ describe('scalar card', () => {
21542155
fobs[0].query(By.css('span')).nativeElement.textContent.trim()
21552156
).toEqual('30');
21562157
}));
2158+
2159+
describe('stepSelectorTimeSelection', () => {
2160+
beforeEach(() => {
2161+
provideMockCardRunToSeriesData(
2162+
selectSpy,
2163+
PluginType.SCALARS,
2164+
'card1',
2165+
null /* metadataOverride */,
2166+
{}
2167+
);
2168+
});
2169+
2170+
it('defaults to min/max', fakeAsync(() => {
2171+
store.overrideSelector(getMetricsStepSelectorEnabled, true);
2172+
store.overrideSelector(getMetricsRangeSelectionEnabled, true);
2173+
const fixture = createComponent('card1');
2174+
fixture.componentInstance.minMaxSteps$.next({
2175+
minStep: 0,
2176+
maxStep: 50,
2177+
});
2178+
expect(
2179+
fixture.componentInstance.stepSelectorTimeSelection$.getValue()
2180+
).toEqual({
2181+
start: {step: 0},
2182+
end: {step: 50},
2183+
});
2184+
}));
2185+
2186+
it('sets end to null when range is disabled', fakeAsync(() => {
2187+
store.overrideSelector(getMetricsStepSelectorEnabled, true);
2188+
store.overrideSelector(getMetricsRangeSelectionEnabled, false);
2189+
const fixture = createComponent('card1');
2190+
fixture.componentInstance.minMaxSteps$.next({
2191+
minStep: 0,
2192+
maxStep: 50,
2193+
});
2194+
expect(
2195+
fixture.componentInstance.stepSelectorTimeSelection$.getValue()
2196+
).toEqual({
2197+
start: {step: 0},
2198+
end: null,
2199+
});
2200+
}));
2201+
2202+
it('uses existing start step when defined', fakeAsync(() => {
2203+
store.overrideSelector(getMetricsStepSelectorEnabled, true);
2204+
store.overrideSelector(getMetricsRangeSelectionEnabled, true);
2205+
const fixture = createComponent('card1');
2206+
fixture.componentInstance.stepSelectorTimeSelection$.next({
2207+
start: {step: 10},
2208+
end: {step: 50},
2209+
});
2210+
fixture.componentInstance.minMaxSteps$.next({
2211+
minStep: 0,
2212+
maxStep: 50,
2213+
});
2214+
expect(
2215+
fixture.componentInstance.stepSelectorTimeSelection$.getValue()
2216+
).toEqual({
2217+
start: {step: 10},
2218+
end: {step: 50},
2219+
});
2220+
}));
2221+
2222+
it('cannot generate steps outside min/max', fakeAsync(() => {
2223+
store.overrideSelector(getMetricsStepSelectorEnabled, true);
2224+
store.overrideSelector(getMetricsRangeSelectionEnabled, true);
2225+
const fixture = createComponent('card1');
2226+
fixture.componentInstance.stepSelectorTimeSelection$.next({
2227+
start: {step: 10},
2228+
end: {step: 50},
2229+
});
2230+
fixture.componentInstance.minMaxSteps$.next({
2231+
minStep: 20,
2232+
maxStep: 30,
2233+
});
2234+
expect(
2235+
fixture.componentInstance.stepSelectorTimeSelection$.getValue()
2236+
).toEqual({
2237+
start: {step: 20},
2238+
end: {step: 30},
2239+
});
2240+
}));
2241+
});
21572242
});
21582243

21592244
describe('fob controls', () => {

‎tensorboard/webapp/metrics/views/card_renderer/utils.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@ export interface TimeSelectionView {
9191
clipped: boolean;
9292
}
9393

94-
function clipStepWithinMinMax(value: number, min: number, max: number) {
94+
export function clipStepWithinMinMax(value: number, min: number, max: number) {
9595
if (value < min) {
9696
return min;
9797
}

‎tensorboard/webapp/metrics/views/card_renderer/utils_test.ts

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ limitations under the License.
1515
import {buildRun} from '../../../runs/store/testing';
1616
import {PartialSeries} from './scalar_card_types';
1717
import {
18+
clipStepWithinMinMax,
1819
getClosestStep,
1920
getDisplayNameForRun,
2021
maybeClipLinkedTimeSelection,
@@ -312,6 +313,25 @@ describe('metrics card_renderer utils test', () => {
312313
});
313314
});
314315

316+
describe('#clipStepWithinMinMax', () => {
317+
it('returns step if greater than min', () => {
318+
expect(clipStepWithinMinMax(1, 0, 5)).toBe(1);
319+
});
320+
321+
it('returns step if less than max', () => {
322+
expect(clipStepWithinMinMax(1, 0, 5)).toBe(1);
323+
});
324+
325+
it('returns min if greater than step', () => {
326+
expect(clipStepWithinMinMax(1, 3, 5)).toBe(3);
327+
expect(clipStepWithinMinMax(1, 5, 3)).toBe(5);
328+
});
329+
330+
it('returns max if less than step', () => {
331+
expect(clipStepWithinMinMax(6, 0, 5)).toBe(5);
332+
});
333+
});
334+
315335
describe('#maybeClipLinkedTimeSelection', () => {
316336
it('clips to the minStep when time selection start step is smaller than the view extend', () => {
317337
expect(

0 commit comments

Comments
 (0)