Skip to content

Commit 1040678

Browse files
rileyajonesrileyajones
committed
respond to code review comments
1 parent 435d8d4 commit 1040678

File tree

7 files changed

+65
-41
lines changed

7 files changed

+65
-41
lines changed

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

+5-1
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ import {
4242
CardUniqueInfo,
4343
SCALARS_SMOOTHING_MAX,
4444
SCALARS_SMOOTHING_MIN,
45+
TooltipSort,
4546
URLDeserializedState,
4647
} from '../internal_types';
4748
import {groupCardIdWithMetdata} from '../utils';
@@ -425,7 +426,10 @@ const reducer = createReducer(
425426
}),
426427
on(globalSettingsLoaded, (state, {partialSettings}) => {
427428
const metricsSettings: Partial<MetricsSettings> = {};
428-
if (partialSettings.tooltipSortString) {
429+
if (
430+
partialSettings.tooltipSortString &&
431+
Object.values(TooltipSort).includes(partialSettings.tooltipSortString)
432+
) {
429433
metricsSettings.tooltipSort = partialSettings.tooltipSortString;
430434
}
431435
if (typeof partialSettings.timeSeriesCardMinWidth === 'number') {

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

+2-2
Original file line numberDiff line numberDiff line change
@@ -2359,7 +2359,7 @@ describe('metrics reducers', () => {
23592359
globalSettingsLoaded({
23602360
partialSettings: {
23612361
ignoreOutliers: true,
2362-
tooltipSortString: 'descending',
2362+
tooltipSortString: 'descending' as TooltipSort,
23632363
},
23642364
})
23652365
);
@@ -2387,7 +2387,7 @@ describe('metrics reducers', () => {
23872387
beforeState,
23882388
globalSettingsLoaded({
23892389
partialSettings: {
2390-
tooltipSortString: 'yo',
2390+
tooltipSortString: 'yo' as TooltipSort,
23912391
},
23922392
})
23932393
);

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

+12-24
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ import {TimeSelectionView} from './utils';
5858

5959
type ScalarTooltipDatum = TooltipDatum<
6060
ScalarCardSeriesMetadata & {
61-
distSqToCursor: number;
61+
distToCursor: number;
6262
closest: boolean;
6363
}
6464
>;
@@ -172,31 +172,25 @@ export class ScalarCardComponent<Downloader> {
172172
metadata: {
173173
...datum.metadata,
174174
closest: false,
175-
distSqToCursor: Math.hypot(
175+
distToCursor: Math.hypot(
176176
datum.point.x - cursorLocationInDataCoord.x,
177177
datum.point.y - cursorLocationInDataCoord.y
178178
),
179-
distSqToCursorPixels: Math.hypot(
180-
datum.point.x - cursorLocation.x,
181-
datum.point.y - cursorLocation.y
182-
),
183-
distSqToCursorX: Math.hypot(
184-
datum.point.x - cursorLocationInDataCoord.x,
185-
0
186-
),
187-
distSqToCursorY: Math.hypot(
188-
0,
189-
datum.point.y - cursorLocationInDataCoord.y
179+
distToCursorPixels: Math.hypot(
180+
datum.pixelLocation.x - cursorLocation.x,
181+
datum.pixelLocation.y - cursorLocation.y
190182
),
183+
distToCursorX: datum.point.x - cursorLocationInDataCoord.x,
184+
distToCursorY: datum.point.y - cursorLocationInDataCoord.y,
191185
},
192186
};
193187
});
194188

195189
let minDist = Infinity;
196190
let minIndex = 0;
197191
for (let index = 0; index < scalarTooltipData.length; index++) {
198-
if (minDist > scalarTooltipData[index].metadata.distSqToCursor) {
199-
minDist = scalarTooltipData[index].metadata.distSqToCursor;
192+
if (minDist > scalarTooltipData[index].metadata.distToCursor) {
193+
minDist = scalarTooltipData[index].metadata.distToCursor;
200194
minIndex = index;
201195
}
202196
}
@@ -212,21 +206,15 @@ export class ScalarCardComponent<Downloader> {
212206
return scalarTooltipData.sort((a, b) => b.point.y - a.point.y);
213207
case TooltipSort.NEAREST:
214208
return scalarTooltipData.sort((a, b) => {
215-
return a.metadata.distSqToCursor - b.metadata.distSqToCursor;
216-
});
217-
case TooltipSort.NEAREST_PIXEL:
218-
return scalarTooltipData.sort((a, b) => {
219-
return (
220-
a.metadata.distSqToCursorPixels - b.metadata.distSqToCursorPixels
221-
);
209+
return a.metadata.distToCursorPixels - b.metadata.distToCursorPixels;
222210
});
223211
case TooltipSort.NEAREST_X:
224212
return scalarTooltipData.sort((a, b) => {
225-
return a.metadata.distSqToCursorX - b.metadata.distSqToCursorX;
213+
return a.metadata.distToCursorX - b.metadata.distToCursorX;
226214
});
227215
case TooltipSort.NEAREST_Y:
228216
return scalarTooltipData.sort((a, b) => {
229-
return a.metadata.distSqToCursorY - b.metadata.distSqToCursorY;
217+
return a.metadata.distToCursorY - b.metadata.distToCursorY;
230218
});
231219
case TooltipSort.DEFAULT:
232220
case TooltipSort.ALPHABETICAL:

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

+38-10
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,7 @@ import {
6767
import {
6868
DataSeries,
6969
DataSeriesMetadataMap,
70+
Point,
7071
RendererType,
7172
ScaleType,
7273
TooltipDatum,
@@ -115,7 +116,8 @@ import {VisLinkedTimeSelectionWarningModule} from './vis_linked_time_selection_w
115116
[ngTemplateOutlet]="tooltipTemplate"
116117
[ngTemplateOutletContext]="{
117118
data: tooltipDataForTesting,
118-
cursorLocationInDataCoord: cursorLocForTesting
119+
cursorLocationInDataCoord: cursorLocationInDataCoordForTesting,
120+
cursorLocation: cursorLocationForTesting
119121
}"
120122
></ng-container>
121123
<ng-container
@@ -165,10 +167,14 @@ class TestableLineChart {
165167
@Output()
166168
onViewBoxOverridden = new EventEmitter<boolean>();
167169

168-
// This input does not exist on real line-chart and is devised to make tooltipTemplate
170+
// These inputs do not exist on the real line-chart and is devised to make tooltipTemplate
169171
// testable without using the real implementation.
170172
@Input() tooltipDataForTesting: TooltipDatum[] = [];
171-
@Input() cursorLocForTesting: {x: number; y: number} = {x: 0, y: 0};
173+
@Input() cursorLocationInDataCoordForTesting: {x: number; y: number} = {
174+
x: 0,
175+
y: 0,
176+
};
177+
@Input() cursorLocationForTesting: {x: number; y: number} = {x: 0, y: 0};
172178

173179
private isViewBoxOverridden = new ReplaySubject<boolean>(1);
174180

@@ -1142,7 +1148,8 @@ describe('scalar card', () => {
11421148
describe('tooltip', () => {
11431149
function buildTooltipDatum(
11441150
metadata?: ScalarCardSeriesMetadata,
1145-
point: Partial<ScalarCardPoint> = {}
1151+
point: Partial<ScalarCardPoint> = {},
1152+
pixelLocation: Point = {x: 0, y: 0}
11461153
): TooltipDatum<ScalarCardSeriesMetadata, ScalarCardPoint> {
11471154
return {
11481155
id: metadata?.id ?? 'a',
@@ -1165,6 +1172,7 @@ describe('scalar card', () => {
11651172
relativeTimeInMs: 0,
11661173
...point,
11671174
},
1175+
pixelLocation,
11681176
};
11691177
}
11701178

@@ -1180,11 +1188,19 @@ describe('scalar card', () => {
11801188

11811189
function setCursorLocation(
11821190
fixture: ComponentFixture<ScalarCardContainer>,
1183-
cursorLocInDataCoord?: {x: number; y: number}
1191+
cursorLocInDataCoord?: {x: number; y: number},
1192+
cursorLocationInPixels?: Point
11841193
) {
11851194
const lineChart = fixture.debugElement.query(Selector.LINE_CHART);
11861195

1187-
lineChart.componentInstance.cursorLocForTesting = cursorLocInDataCoord;
1196+
if (cursorLocInDataCoord) {
1197+
lineChart.componentInstance.cursorLocationInDataCoordForTesting =
1198+
cursorLocInDataCoord;
1199+
}
1200+
if (cursorLocationInPixels) {
1201+
lineChart.componentInstance.cursorLocationForTesting =
1202+
cursorLocationInPixels;
1203+
}
11881204
lineChart.componentInstance.changeDetectorRef.markForCheck();
11891205
}
11901206

@@ -1616,6 +1632,10 @@ describe('scalar card', () => {
16161632
y: 1000,
16171633
value: 1000,
16181634
wallTime: new Date('2020-01-01').getTime(),
1635+
},
1636+
{
1637+
x: 0,
1638+
y: 100,
16191639
}
16201640
),
16211641
buildTooltipDatum(
@@ -1634,6 +1654,10 @@ describe('scalar card', () => {
16341654
y: -500,
16351655
value: -500,
16361656
wallTime: new Date('2020-12-31').getTime(),
1657+
},
1658+
{
1659+
x: 50,
1660+
y: 0,
16371661
}
16381662
),
16391663
buildTooltipDatum(
@@ -1652,26 +1676,30 @@ describe('scalar card', () => {
16521676
y: 3,
16531677
value: 3,
16541678
wallTime: new Date('2021-01-01').getTime(),
1679+
},
1680+
{
1681+
x: 1000,
1682+
y: 30,
16551683
}
16561684
),
16571685
]);
1658-
setCursorLocation(fixture, {x: 500, y: -100});
1686+
setCursorLocation(fixture, {x: 500, y: -100}, {x: 50, y: 0});
16591687
fixture.detectChanges();
16601688
assertTooltipRows(fixture, [
16611689
['', 'Row 2', '-500', '1,000', anyString, anyString],
16621690
['', 'Row 1', '1000', '0', anyString, anyString],
16631691
['', 'Row 3', '3', '10,000', anyString, anyString],
16641692
]);
16651693

1666-
setCursorLocation(fixture, {x: 500, y: 600});
1694+
setCursorLocation(fixture, {x: 500, y: 600}, {x: 50, y: 80});
16671695
fixture.detectChanges();
16681696
assertTooltipRows(fixture, [
16691697
['', 'Row 1', '1000', '0', anyString, anyString],
16701698
['', 'Row 2', '-500', '1,000', anyString, anyString],
16711699
['', 'Row 3', '3', '10,000', anyString, anyString],
16721700
]);
16731701

1674-
setCursorLocation(fixture, {x: 10000, y: -100});
1702+
setCursorLocation(fixture, {x: 10000, y: -100}, {x: 1000, y: 20});
16751703
fixture.detectChanges();
16761704
assertTooltipRows(fixture, [
16771705
['', 'Row 3', '3', '10,000', anyString, anyString],
@@ -1680,7 +1708,7 @@ describe('scalar card', () => {
16801708
]);
16811709

16821710
// Right between row 1 and row 2. When tied, original order is used.
1683-
setCursorLocation(fixture, {x: 500, y: 250});
1711+
setCursorLocation(fixture, {x: 500, y: 250}, {x: 25, y: 50});
16841712
fixture.detectChanges();
16851713
assertTooltipRows(fixture, [
16861714
['', 'Row 1', '1000', '0', anyString, anyString],

‎tensorboard/webapp/metrics/views/right_pane/settings_view_component.ts

+1-2
Original file line numberDiff line numberDiff line change
@@ -86,8 +86,7 @@ export class SettingsViewComponent {
8686
{value: TooltipSort.ALPHABETICAL, displayText: 'Alphabetical'},
8787
{value: TooltipSort.ASCENDING, displayText: 'Ascending'},
8888
{value: TooltipSort.DESCENDING, displayText: 'Descending'},
89-
{value: TooltipSort.NEAREST, displayText: 'Nearest'},
90-
{value: TooltipSort.NEAREST_PIXEL, displayText: 'Nearest Pixel'},
89+
{value: TooltipSort.NEAREST, displayText: 'Nearest Pixel'},
9190
{value: TooltipSort.NEAREST_X, displayText: 'Nearest X'},
9291
{value: TooltipSort.NEAREST_Y, displayText: 'Nearest Y'},
9392
];

‎tensorboard/webapp/persistent_settings/_data_source/types.ts

-1
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ export enum TooltipSort {
2929
ASCENDING = 'ascending',
3030
DESCENDING = 'descending',
3131
NEAREST = 'nearest',
32-
NEAREST_PIXEL = 'nearest_pixel',
3332
NEAREST_X = 'nearest_x',
3433
NEAREST_Y = 'nearest_Y',
3534
}

‎tensorboard/webapp/widgets/line_chart_v2/sub_view/line_chart_interactive_view.ts

+7-1
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,7 @@ export interface TooltipDatum<
6868
metadata: Metadata;
6969
closestPointIndex: number;
7070
point: PointDatum;
71+
pixelLocation: Point;
7172
}
7273

7374
const SCROLL_ZOOM_SPEED_FACTOR = 0.01;
@@ -555,10 +556,15 @@ export class LineChartInteractiveViewComponent
555556
})
556557
.map(({seriesDatum, metadata}) => {
557558
const index = findClosestIndex(seriesDatum.points, cursorLoc.x);
559+
const point = seriesDatum.points[index];
558560
return {
559561
id: seriesDatum.id,
560562
closestPointIndex: index,
561-
point: seriesDatum.points[index],
563+
point,
564+
pixelLocation: {
565+
x: this.getDomX(point.x),
566+
y: this.getDomY(point.y),
567+
},
562568
metadata,
563569
};
564570
})

0 commit comments

Comments
 (0)