Linked Time: Fix bug with zooming always maximizing end step (tensorf…

…low#6058) ## Motivation for features / changes When changing the zoom on the scalar card with an the end fob the end fob was always placed at the maximum possible step. For Googlers b/259562961 ## Technical description of changes The `StepSelectorTimeSelection` derived by a subscription to a bunch of other Observables. The main issue was that it simply never checked if an existing end step existed. The code was also kinda hard to read(:raised_hand_with_fingers_splayed: my fault) so I went ahead with a bit of a cleanup there. ## Screenshots of UI changes Before: Zooming In: ![129f960a-2ee0-4762-98eb-875594b31b6f](https://user-images.githubusercontent.com/78179109/202584325-cdd164e7-2b04-4930-8de7-4c551ce85bcd.gif) Resetting Zoom Level: ![00194071-2280-4d9b-953c-18740f7cad77](https://user-images.githubusercontent.com/78179109/202584465-4ddb3235-93b7-431b-92e1-16a7961d79e5.gif) After: ![cf40658d-c57c-4d76-a267-9feb8b35f7fe](https://user-images.githubusercontent.com/78179109/202583150-6e7c662c-8230-4060-a289-7880dafda39a.gif) ## Detailed steps to verify changes work correctly (as executed by you) 1) Start tensorboard 2) Navigate to http://localhost:6006?enableRangeSelection&enableDataTable 3) Place two fobs on the scalar card 4) Zoom in on a segment of the chart which contains both fobs (much easier now that tensorflow#5932 is closed) 5) Assert neither fob has moved 6) Reset the chart zoom level 7) Assert that neither fob has moved.
bmd3k · Nov 18, 2022 · 888dcdc · 888dcdc
1 parent 0246da2
commit 888dcdc
Show file tree

Hide file tree

Showing 6 changed files with 67 additions and 32 deletions.
diff --git a/tensorboard/webapp/metrics/views/card_renderer/histogram_card_container.ts b/tensorboard/webapp/metrics/views/card_renderer/histogram_card_container.ts
@@ -48,7 +48,7 @@ import {CardId, CardMetadata} from '../../types';
 import {CardRenderer} from '../metrics_view_types';
 import {getTagDisplayName} from '../utils';
 import {
-  maybeClipLinkedTimeSelection,
+  maybeClipTimeSelectionView,
   maybeSetClosestStartStep,
   TimeSelectionView,
 } from './utils';
@@ -182,7 +182,7 @@ export class HistogramCardContainer implements CardRenderer, OnInit {
           minStep = Math.min(step, minStep);
           maxStep = Math.max(step, maxStep);
         }
-        const linkedTimeSelectionView = maybeClipLinkedTimeSelection(
+        const linkedTimeSelectionView = maybeClipTimeSelectionView(
           linkedTimeSelection,
           minStep,
           maxStep

diff --git a/tensorboard/webapp/metrics/views/card_renderer/image_card_container.ts b/tensorboard/webapp/metrics/views/card_renderer/image_card_container.ts
@@ -56,7 +56,7 @@ import {
 import {CardId, CardMetadata} from '../../types';
 import {CardRenderer} from '../metrics_view_types';
 import {getTagDisplayName} from '../utils';
-import {maybeClipLinkedTimeSelection, TimeSelectionView} from './utils';
+import {maybeClipTimeSelectionView, TimeSelectionView} from './utils';
 
 const DISTANCE_RATIO = 0.1;
 
@@ -263,7 +263,7 @@ export class ImageCardContainer implements CardRenderer, OnInit, OnDestroy {
 
           const minStep = Math.min(...steps);
           const maxStep = Math.max(...steps);
-          return maybeClipLinkedTimeSelection(
+          return maybeClipTimeSelectionView(
             linkedTimeSelection,
             minStep,
             maxStep

diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts
@@ -101,8 +101,8 @@ import {
   SortingInfo,
 } from './scalar_card_types';
 import {
-  clipStepWithinMinMax,
-  maybeClipLinkedTimeSelection,
+  maybeClipTimeSelection,
+  maybeClipTimeSelectionView,
   partitionSeries,
   TimeSelectionView,
 } from './utils';
@@ -445,7 +445,7 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy {
             forkedTimeSelection.end = {step: maxStep};
           }
 
-          return maybeClipLinkedTimeSelection(
+          return maybeClipTimeSelectionView(
             forkedTimeSelection,
             minStep,
             maxStep
@@ -615,21 +615,28 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy {
       this.store.select(getMetricsRangeSelectionEnabled),
     ]).subscribe(
       ([{minStep, maxStep}, enableStepSelector, rangeSelectionEnabled]) => {
-        this.stepSelectorTimeSelection$.next(
-          enableStepSelector
-            ? {
-                start: {
-                  step: clipStepWithinMinMax(
-                    this.stepSelectorTimeSelection$.getValue()?.start.step ??
-                      minStep,
-                    minStep,
-                    maxStep
-                  ),
-                },
-                end: rangeSelectionEnabled ? {step: maxStep} : null,
-              }
-            : null
+        if (!enableStepSelector) {
+          this.stepSelectorTimeSelection$.next(null);
+          return;
+        }
+        const currentStartStep =
+          this.stepSelectorTimeSelection$.getValue()?.start.step;
+        const currentEndStep =
+          this.stepSelectorTimeSelection$.getValue()?.end?.step;
+
+        const potentiallyClippedTimeSelection = maybeClipTimeSelection(
+          {
+            start: {
+              step: currentStartStep ?? minStep,
+            },
+            end: rangeSelectionEnabled
+              ? {step: currentEndStep ?? maxStep}
+              : null,
+          },
+          minStep,
+          maxStep
         );
+        this.stepSelectorTimeSelection$.next(potentiallyClippedTimeSelection);
       }
     );
   }
@@ -689,7 +696,7 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy {
     newTimeSelectionWithAffordance: TimeSelectionWithAffordance
   ) {
     const {minStep, maxStep} = this.minMaxSteps$.getValue();
-    const {startStep, endStep} = maybeClipLinkedTimeSelection(
+    const {startStep, endStep} = maybeClipTimeSelectionView(
       newTimeSelectionWithAffordance.timeSelection,
       minStep,
       maxStep

diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts
@@ -2509,6 +2509,15 @@ describe('scalar card', () => {
           minStep: 10,
           maxStep: 30,
         });
+
+        fixture.componentInstance.onLineChartZoom({
+          x: [8, 31],
+          y: [0, 100],
+        });
+        expect(newSteps!).toEqual({
+          minStep: 10,
+          maxStep: 30,
+        });
       }));
     });
   });

diff --git a/tensorboard/webapp/metrics/views/card_renderer/utils.ts b/tensorboard/webapp/metrics/views/card_renderer/utils.ts
@@ -101,7 +101,26 @@ export function clipStepWithinMinMax(value: number, min: number, max: number) {
   return value;
 }
 
-export function maybeClipLinkedTimeSelection(
+export function maybeClipTimeSelection(
+  timeSelection: TimeSelection,
+  minStep: number,
+  maxStep: number
+): TimeSelection {
+  const timeSelectionView = maybeClipTimeSelectionView(
+    timeSelection,
+    minStep,
+    maxStep
+  );
+  return {
+    start: {step: timeSelectionView.startStep},
+    end:
+      timeSelectionView.endStep === null
+        ? null
+        : {step: timeSelectionView.endStep},
+  };
+}
+
+export function maybeClipTimeSelectionView(
   timeSelection: TimeSelection,
   minStep: number,
   maxStep: number

diff --git a/tensorboard/webapp/metrics/views/card_renderer/utils_test.ts b/tensorboard/webapp/metrics/views/card_renderer/utils_test.ts
@@ -18,7 +18,7 @@ import {
   clipStepWithinMinMax,
   getClosestStep,
   getDisplayNameForRun,
-  maybeClipLinkedTimeSelection,
+  maybeClipTimeSelectionView,
   maybeSetClosestStartStep,
   partitionSeries,
 } from './utils';
@@ -335,7 +335,7 @@ describe('metrics card_renderer utils test', () => {
   describe('#maybeClipLinkedTimeSelection', () => {
     it('clips to the minStep when time selection start step is smaller than the view extend', () => {
       expect(
-        maybeClipLinkedTimeSelection(
+        maybeClipTimeSelectionView(
           {
             start: {step: 0},
             end: null,
@@ -352,7 +352,7 @@ describe('metrics card_renderer utils test', () => {
 
     it('clips to maxStep when time selection end step is greater than view extend', () => {
       expect(
-        maybeClipLinkedTimeSelection(
+        maybeClipTimeSelectionView(
           {
             start: {step: 0},
             end: {step: 4},
@@ -369,7 +369,7 @@ describe('metrics card_renderer utils test', () => {
 
     it('does not clip when time selection falls into the view extend', () => {
       expect(
-        maybeClipLinkedTimeSelection(
+        maybeClipTimeSelectionView(
           {
             start: {step: 10},
             end: null,
@@ -386,7 +386,7 @@ describe('metrics card_renderer utils test', () => {
 
     it('returns minStep and maxStep when the timeselection is a superset of the min/maxstep', () => {
       expect(
-        maybeClipLinkedTimeSelection(
+        maybeClipTimeSelectionView(
           {
             start: {step: 0},
             end: {step: 100},
@@ -403,7 +403,7 @@ describe('metrics card_renderer utils test', () => {
 
     it('clips both fobs to maxStep when timeSelection is greater than maxStep', () => {
       expect(
-        maybeClipLinkedTimeSelection(
+        maybeClipTimeSelectionView(
           {
             start: {step: 50},
             end: {step: 100},
@@ -420,7 +420,7 @@ describe('metrics card_renderer utils test', () => {
 
     it('returns startStep === endStep === minStep when timeSelection is below minStep', () => {
       expect(
-        maybeClipLinkedTimeSelection(
+        maybeClipTimeSelectionView(
           {
             start: {step: 0},
             end: {step: 10},
@@ -437,7 +437,7 @@ describe('metrics card_renderer utils test', () => {
 
     it('does not clip when time selection falls within the view extent', () => {
       expect(
-        maybeClipLinkedTimeSelection(
+        maybeClipTimeSelectionView(
           {
             start: {step: 0},
             end: {step: 4},
@@ -452,7 +452,7 @@ describe('metrics card_renderer utils test', () => {
       });
 
       expect(
-        maybeClipLinkedTimeSelection(
+        maybeClipTimeSelectionView(
           {
             start: {step: 1},
             end: {step: 3},