Skip to content

Commit e659943

Browse files
rileyajonesrileyajones
authored
Linked Time: Move time selection formatting to the selector layer (#6265)
## Motivation for features / changes This is simply a change to our code organization. In #6229 we decided to handle formatting the time selection in container layer, but that has become a bit messy so I'm moving it back to the selector layer ## Technical description of changes Rather than getting the cards time selection in the selector layer, then formatting it in the container layer, I'm opting to handle all the logic in the container layer. ## Screenshots of UI changes None ## Detailed steps to verify changes work correctly (as executed by you) I added lots of tests, all the tests should pass For Googlers [internal tests passed](https://fusion2.corp.google.com/presubmit/tap/518714309/OCL:518714309:BASE:518717432:1679531577959:4d567b9a/targets) ## Alternate designs / implementations considered
1 parent 8685981 commit e659943

File tree

5 files changed

+249
-60
lines changed

5 files changed

+249
-60
lines changed

‎tensorboard/webapp/metrics/store/BUILD‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ tf_ts_library(
2323
"//tensorboard/webapp/metrics/actions",
2424
"//tensorboard/webapp/metrics/data_source",
2525
"//tensorboard/webapp/metrics/views/card_renderer:scalar_card_types",
26+
"//tensorboard/webapp/metrics/views/card_renderer:utils",
2627
"//tensorboard/webapp/persistent_settings",
2728
"//tensorboard/webapp/types",
2829
"//tensorboard/webapp/util:dom",

‎tensorboard/webapp/metrics/store/metrics_selectors.ts‎

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@ import {
3333
ColumnHeader,
3434
MinMaxStep,
3535
} from '../views/card_renderer/scalar_card_types';
36+
import {formatTimeSelection} from '../views/card_renderer/utils';
3637
import * as storeUtils from './metrics_store_internal_utils';
3738
import {
3839
getCardSelectionStateToBoolean,
@@ -536,20 +537,17 @@ export const getMetricsCardDataMinMax = createSelector(
536537
export const getMetricsCardTimeSelection = createSelector(
537538
getCardStateMap,
538539
getMetricsStepSelectorEnabled,
540+
getMetricsRangeSelectionEnabled,
539541
getMetricsLinkedTimeEnabled,
540542
getMetricsLinkedTimeSelection,
541543
(
542544
cardStateMap: CardStateMap,
543545
globalStepSelectionEnabled: boolean,
546+
globalRangeSelectionEnabled: boolean,
544547
linkedTimeEnabled: boolean,
545548
linkedTimeSelection: TimeSelection | null,
546549
cardId: CardId
547550
): TimeSelection | undefined => {
548-
// Handling Linked Time
549-
if (linkedTimeEnabled && linkedTimeSelection) {
550-
return linkedTimeSelection;
551-
}
552-
553551
const cardState = cardStateMap[cardId];
554552
if (!cardState) {
555553
return;
@@ -559,6 +557,16 @@ export const getMetricsCardTimeSelection = createSelector(
559557
return;
560558
}
561559

560+
// Handling Linked Time
561+
if (linkedTimeEnabled && linkedTimeSelection) {
562+
return formatTimeSelection(
563+
linkedTimeSelection,
564+
minMaxStep,
565+
// Note that globalRangeSelection should always be used with linked time.
566+
globalRangeSelectionEnabled
567+
);
568+
}
569+
562570
// If the user has disabled step selection, nothing should be returned.
563571
if (
564572
!getCardSelectionStateToBoolean(
@@ -569,13 +577,22 @@ export const getMetricsCardTimeSelection = createSelector(
569577
return;
570578
}
571579

580+
const rangeSelectionEnabled = getCardSelectionStateToBoolean(
581+
cardState.rangeSelectionOverride,
582+
globalRangeSelectionEnabled
583+
);
584+
572585
const startStep = cardState.timeSelection?.start.step ?? minMaxStep.minStep;
573586
const endStep = cardState.timeSelection?.end?.step ?? minMaxStep.maxStep;
574587

575588
// The default time selection
576-
return {
577-
start: {step: startStep},
578-
end: {step: endStep},
579-
};
589+
return formatTimeSelection(
590+
{
591+
start: {step: startStep},
592+
end: {step: endStep},
593+
},
594+
minMaxStep,
595+
rangeSelectionEnabled
596+
);
580597
}
581598
);

‎tensorboard/webapp/metrics/store/metrics_selectors_test.ts‎

Lines changed: 195 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -438,6 +438,7 @@ describe('metrics selectors', () => {
438438
const state = appStateFromMetricsState(
439439
buildMetricsState({
440440
...partialState,
441+
rangeSelectionEnabled: true,
441442
cardStateMap: {
442443
card1: {
443444
stepSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
@@ -551,6 +552,7 @@ describe('metrics selectors', () => {
551552
const state = appStateFromMetricsState(
552553
buildMetricsState({
553554
...partialState,
555+
rangeSelectionEnabled: true,
554556
cardStateMap: {
555557
card1: {
556558
stepSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
@@ -577,6 +579,7 @@ describe('metrics selectors', () => {
577579
const state = appStateFromMetricsState(
578580
buildMetricsState({
579581
...partialState,
582+
rangeSelectionEnabled: true,
580583
cardStateMap: {
581584
card1: {
582585
stepSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
@@ -594,22 +597,206 @@ describe('metrics selectors', () => {
594597
end: {step: 5},
595598
});
596599
});
600+
601+
it('removes end value if range selection is overridden as disabled', () => {
602+
const state = appStateFromMetricsState(
603+
buildMetricsState({
604+
...partialState,
605+
rangeSelectionEnabled: true,
606+
cardStateMap: {
607+
card1: {
608+
stepSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
609+
rangeSelectionOverride:
610+
CardFeatureOverride.OVERRIDE_AS_DISABLED,
611+
dataMinMax: {
612+
minStep: 0,
613+
maxStep: 5,
614+
},
615+
},
616+
},
617+
})
618+
);
619+
620+
expect(selectors.getMetricsCardTimeSelection(state, 'card1')).toEqual({
621+
start: {step: 0},
622+
end: null,
623+
});
624+
});
625+
626+
it('removes end value if range selection is globally disabled', () => {
627+
const state = appStateFromMetricsState(
628+
buildMetricsState({
629+
...partialState,
630+
rangeSelectionEnabled: false,
631+
cardStateMap: {
632+
card1: {
633+
stepSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
634+
dataMinMax: {
635+
minStep: 0,
636+
maxStep: 5,
637+
},
638+
},
639+
},
640+
})
641+
);
642+
643+
expect(selectors.getMetricsCardTimeSelection(state, 'card1')).toEqual({
644+
start: {step: 0},
645+
end: null,
646+
});
647+
});
648+
649+
it('does not remove end value if range selection is overridden as enabled', () => {
650+
const state = appStateFromMetricsState(
651+
buildMetricsState({
652+
...partialState,
653+
rangeSelectionEnabled: false,
654+
cardStateMap: {
655+
card1: {
656+
stepSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
657+
rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
658+
dataMinMax: {
659+
minStep: 0,
660+
maxStep: 5,
661+
},
662+
},
663+
},
664+
})
665+
);
666+
667+
expect(selectors.getMetricsCardTimeSelection(state, 'card1')).toEqual({
668+
start: {step: 0},
669+
end: {step: 5},
670+
});
671+
});
672+
673+
it('clips time selection if it exceeds the cards minMax', () => {
674+
const state = appStateFromMetricsState(
675+
buildMetricsState({
676+
...partialState,
677+
rangeSelectionEnabled: true,
678+
cardStateMap: {
679+
card1: {
680+
stepSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
681+
dataMinMax: {
682+
minStep: 5,
683+
maxStep: 10,
684+
},
685+
timeSelection: {
686+
start: {step: 0},
687+
end: {step: 15},
688+
},
689+
},
690+
},
691+
})
692+
);
693+
694+
expect(selectors.getMetricsCardTimeSelection(state, 'card1')).toEqual({
695+
start: {step: 5},
696+
end: {step: 10},
697+
});
698+
});
597699
});
598700

599-
it('returns linkedTimeSelection if linkedTime is enabled', () => {
600-
const state = appStateFromMetricsState(
601-
buildMetricsState({
701+
describe('with linkedTime enabled', () => {
702+
let partialState: Partial<MetricsState>;
703+
beforeEach(() => {
704+
partialState = {
602705
linkedTimeEnabled: true,
603706
linkedTimeSelection: {
604707
start: {step: 0},
605708
end: {step: 5},
606709
},
607-
})
608-
);
710+
};
711+
});
609712

610-
expect(selectors.getMetricsCardTimeSelection(state, 'card1')).toEqual({
611-
start: {step: 0},
612-
end: {step: 5},
713+
it('returns linkedTimeSelection if linkedTime is enabled', () => {
714+
const state = appStateFromMetricsState(
715+
buildMetricsState({
716+
...partialState,
717+
rangeSelectionEnabled: true,
718+
cardStateMap: {
719+
card1: {
720+
dataMinMax: {
721+
minStep: 0,
722+
maxStep: 5,
723+
},
724+
},
725+
},
726+
})
727+
);
728+
729+
expect(selectors.getMetricsCardTimeSelection(state, 'card1')).toEqual({
730+
start: {step: 0},
731+
end: {step: 5},
732+
});
733+
});
734+
735+
it('removes end value if global range selection is disabled', () => {
736+
const state = appStateFromMetricsState(
737+
buildMetricsState({
738+
...partialState,
739+
rangeSelectionEnabled: false,
740+
cardStateMap: {
741+
card1: {
742+
dataMinMax: {
743+
minStep: 0,
744+
maxStep: 5,
745+
},
746+
},
747+
},
748+
})
749+
);
750+
751+
expect(selectors.getMetricsCardTimeSelection(state, 'card1')).toEqual({
752+
start: {step: 0},
753+
end: null,
754+
});
755+
});
756+
757+
it('maintains end value if local range selection is overridden as disabled', () => {
758+
const state = appStateFromMetricsState(
759+
buildMetricsState({
760+
...partialState,
761+
rangeSelectionEnabled: true,
762+
cardStateMap: {
763+
card1: {
764+
rangeSelectionOverride: CardFeatureOverride.OVERRIDE_AS_ENABLED,
765+
dataMinMax: {
766+
minStep: 0,
767+
maxStep: 5,
768+
},
769+
},
770+
},
771+
})
772+
);
773+
774+
expect(selectors.getMetricsCardTimeSelection(state, 'card1')).toEqual({
775+
start: {step: 0},
776+
end: {step: 5},
777+
});
778+
});
779+
780+
it('clips time selection based on card minMax', () => {
781+
const state = appStateFromMetricsState(
782+
buildMetricsState({
783+
...partialState,
784+
rangeSelectionEnabled: true,
785+
cardStateMap: {
786+
card1: {
787+
dataMinMax: {
788+
minStep: 1,
789+
maxStep: 4,
790+
},
791+
},
792+
},
793+
})
794+
);
795+
796+
expect(selectors.getMetricsCardTimeSelection(state, 'card1')).toEqual({
797+
start: {step: 1},
798+
end: {step: 4},
799+
});
613800
});
614801
});
615802
});

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

Lines changed: 5 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -69,8 +69,8 @@ import {ScaleType} from '../../../widgets/line_chart_v2/types';
6969
import {
7070
cardMinMaxChanged,
7171
dataTableColumnDrag,
72-
metricsCardStateUpdated,
7372
metricsCardFullSizeToggled,
73+
metricsCardStateUpdated,
7474
sortingDataTable,
7575
stepSelectorToggled,
7676
timeSelectionChanged,
@@ -82,7 +82,6 @@ import {
8282
getCardMetadata,
8383
getCardTimeSeries,
8484
getMetricsCardMinMax,
85-
getMetricsCardRangeSelectionEnabled,
8685
getMetricsIgnoreOutliers,
8786
getMetricsScalarPartitionNonMonotonicX,
8887
getMetricsScalarSmoothing,
@@ -108,7 +107,6 @@ import {
108107
SortingInfo,
109108
} from './scalar_card_types';
110109
import {
111-
formatTimeSelection,
112110
maybeClipTimeSelectionView,
113111
partitionSeries,
114112
TimeSelectionView,
@@ -450,22 +448,10 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy {
450448
})
451449
);
452450

453-
this.stepOrLinkedTimeSelection$ = this.store
454-
.select(getMetricsCardTimeSelection, this.cardId)
455-
.pipe(
456-
combineLatestWith(
457-
this.minMaxSteps$,
458-
this.store.select(getMetricsCardRangeSelectionEnabled, this.cardId)
459-
),
460-
map(([timeSelection, minMaxSteps, rangeSelection]) => {
461-
if (!timeSelection || !minMaxSteps) return;
462-
return formatTimeSelection(
463-
timeSelection,
464-
minMaxSteps,
465-
rangeSelection
466-
);
467-
})
468-
);
451+
this.stepOrLinkedTimeSelection$ = this.store.select(
452+
getMetricsCardTimeSelection,
453+
this.cardId
454+
);
469455

470456
this.columnHeaders$ = combineLatest([
471457
this.stepOrLinkedTimeSelection$,

0 commit comments

Comments
 (0)