Skip to content

Feature: Allow tooltip sorting by X, Y, and Pixel distance from cursor #6116

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Jan 10, 2023
Merged

Feature: Allow tooltip sorting by X, Y, and Pixel distance from cursor #6116

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
435d8d4
add three new tooltip sort orders
rileyajones Dec 27, 2022
eb5123d
respond to code review comments
rileyajones Jan 3, 2023
edf2125
remove nearest_x, rename tooltipSortSting
rileyajones Jan 4, 2023
7140374
update closest logic
rileyajones Jan 4, 2023
e3ce6e7
resolve circular dependency issue
rileyajones Jan 6, 2023
6d27028
rename variables, remove workspace file
rileyajones Jan 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tensorboard/webapp/metrics/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ tf_ts_library(
],
visibility = ["//tensorboard/webapp/metrics:__subpackages__"],
deps = [
"//tensorboard/webapp/persistent_settings/_data_source:types",
"//tensorboard/webapp/widgets/card_fob:types",
"//tensorboard/webapp/widgets/histogram:types",
],
Expand Down
13 changes: 1 addition & 12 deletions tensorboard/webapp/metrics/internal_types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ limitations under the License.
import {TimeSelection} from '../widgets/card_fob/card_fob_types';
import {HistogramMode} from '../widgets/histogram/histogram_types';

export {TooltipSort} from '../persistent_settings/_data_source/types';
export {HistogramMode, TimeSelection};

export enum PluginType {
Expand All @@ -23,18 +24,6 @@ export enum PluginType {
IMAGES = 'images',
}

// When adding a new value to the enum, please implement the deserializer on
// data_source/metrics_data_source.ts.
// When editing a value of the enum, please write a backward compatible
// deserializer in data_source/metrics_data_source.ts.
export enum TooltipSort {
DEFAULT = 'default',
ALPHABETICAL = 'alphabetical',
ASCENDING = 'ascending',
DESCENDING = 'descending',
NEAREST = 'nearest',
}

export enum XAxisType {
STEP,
RELATIVE,
Expand Down
22 changes: 5 additions & 17 deletions tensorboard/webapp/metrics/store/metrics_reducers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,23 +426,11 @@ const reducer = createReducer(
}),
on(globalSettingsLoaded, (state, {partialSettings}) => {
const metricsSettings: Partial<MetricsSettings> = {};
if (partialSettings.tooltipSortString) {
switch (partialSettings.tooltipSortString) {
case TooltipSort.DEFAULT:
case TooltipSort.ALPHABETICAL:
metricsSettings.tooltipSort = TooltipSort.ALPHABETICAL;
break;
case TooltipSort.ASCENDING:
metricsSettings.tooltipSort = TooltipSort.ASCENDING;
break;
case TooltipSort.DESCENDING:
metricsSettings.tooltipSort = TooltipSort.DESCENDING;
break;
case TooltipSort.NEAREST:
metricsSettings.tooltipSort = TooltipSort.NEAREST;
break;
default:
}
if (
partialSettings.tooltipSortString &&
Object.values(TooltipSort).includes(partialSettings.tooltipSortString)
) {
metricsSettings.tooltipSort = partialSettings.tooltipSortString;
}
if (typeof partialSettings.timeSeriesCardMinWidth === 'number') {
metricsSettings.cardMinWidth = partialSettings.timeSeriesCardMinWidth;
Expand Down
4 changes: 2 additions & 2 deletions tensorboard/webapp/metrics/store/metrics_reducers_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2359,7 +2359,7 @@ describe('metrics reducers', () => {
globalSettingsLoaded({
partialSettings: {
ignoreOutliers: true,
tooltipSortString: 'descending',
tooltipSortString: 'descending' as TooltipSort,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just use TooltipSort.DESCENDING?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can but, because the test is testing deserialization from browser storage, I prefer testing the string literal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reasoning doesn't seem to apply here.

At this point in the code you are several layers removed from the serialized form. The only serialized form of the string exists in LocalStorage. The type globalSettingsLoaded says it will include a TooltipSort object, so that is the ideal type to use for the test.

},
})
);
Expand Down Expand Up @@ -2387,7 +2387,7 @@ describe('metrics reducers', () => {
beforeState,
globalSettingsLoaded({
partialSettings: {
tooltipSortString: 'yo',
tooltipSortString: 'yo' as TooltipSort,
},
})
);
Expand Down
5 changes: 3 additions & 2 deletions tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,8 @@
<ng-template
#tooltip
let-tooltipData="data"
let-cursorLoc="cursorLocationInDataCoord"
let-cursorLocationInDataCoord="cursorLocationInDataCoord"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whatever names you choose line_chart_interactive_view.ts, please mirror here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These names all look the same to me? Am I missing something? (of note, they were not the same before and I renamed them to be the same).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was another comment I left about naming inconsistency, which does not seem to have been addressed. Do you mind taking a look at that one and then revisiting this one? I assume this comment doesn't make sense to you because you missed the other comment.

let-cursorLocation="cursorLocation"
>
<table class="tooltip">
<thead>
Expand All @@ -136,7 +137,7 @@
<tbody>
<ng-container
*ngFor="
let datum of getCursorAwareTooltipData(tooltipData, cursorLoc);
let datum of getCursorAwareTooltipData(tooltipData, cursorLocationInDataCoord, cursorLocation);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Can this be line wrapped?

trackBy: trackByTooltipDatum
"
>
Expand Down
31 changes: 23 additions & 8 deletions tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ import {TimeSelectionView} from './utils';

type ScalarTooltipDatum = TooltipDatum<
ScalarCardSeriesMetadata & {
distSqToCursor: number;
distToCursor: number;
closest: boolean;
}
>;
Expand Down Expand Up @@ -163,27 +163,34 @@ export class ScalarCardComponent<Downloader> {

getCursorAwareTooltipData(
tooltipData: TooltipDatum<ScalarCardSeriesMetadata>[],
cursorLoc: {x: number; y: number}
cursorLocationInDataCoord: {x: number; y: number},
cursorLocation: {x: number; y: number}
): ScalarTooltipDatum[] {
const scalarTooltipData = tooltipData.map((datum) => {
return {
...datum,
metadata: {
...datum.metadata,
closest: false,
distSqToCursor: Math.hypot(
datum.point.x - cursorLoc.x,
datum.point.y - cursorLoc.y
distToCursor: Math.hypot(
datum.point.x - cursorLocationInDataCoord.x,
datum.point.y - cursorLocationInDataCoord.y
),
distToCursorPixels: Math.hypot(
datum.pixelLocation.x - cursorLocation.x,
datum.pixelLocation.y - cursorLocation.y
),
distToCursorX: datum.point.x - cursorLocationInDataCoord.x,
distToCursorY: datum.point.y - cursorLocationInDataCoord.y,
},
};
});

let minDist = Infinity;
let minIndex = 0;
for (let index = 0; index < scalarTooltipData.length; index++) {
if (minDist > scalarTooltipData[index].metadata.distSqToCursor) {
minDist = scalarTooltipData[index].metadata.distSqToCursor;
if (minDist > scalarTooltipData[index].metadata.distToCursor) {
minDist = scalarTooltipData[index].metadata.distToCursor;
minIndex = index;
}
}
Expand All @@ -199,7 +206,15 @@ export class ScalarCardComponent<Downloader> {
return scalarTooltipData.sort((a, b) => b.point.y - a.point.y);
case TooltipSort.NEAREST:
return scalarTooltipData.sort((a, b) => {
return a.metadata.distSqToCursor - b.metadata.distSqToCursor;
return a.metadata.distToCursorPixels - b.metadata.distToCursorPixels;
});
case TooltipSort.NEAREST_X:
return scalarTooltipData.sort((a, b) => {
return a.metadata.distToCursorX - b.metadata.distToCursorX;
});
case TooltipSort.NEAREST_Y:
return scalarTooltipData.sort((a, b) => {
return a.metadata.distToCursorY - b.metadata.distToCursorY;
});
case TooltipSort.DEFAULT:
case TooltipSort.ALPHABETICAL:
Expand Down
48 changes: 38 additions & 10 deletions tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ import {
import {
DataSeries,
DataSeriesMetadataMap,
Point,
RendererType,
ScaleType,
TooltipDatum,
Expand Down Expand Up @@ -115,7 +116,8 @@ import {VisLinkedTimeSelectionWarningModule} from './vis_linked_time_selection_w
[ngTemplateOutlet]="tooltipTemplate"
[ngTemplateOutletContext]="{
data: tooltipDataForTesting,
cursorLocationInDataCoord: cursorLocForTesting
cursorLocationInDataCoord: cursorLocationInDataCoordForTesting,
cursorLocation: cursorLocationForTesting
}"
></ng-container>
<ng-container
Expand Down Expand Up @@ -165,10 +167,14 @@ class TestableLineChart {
@Output()
onViewBoxOverridden = new EventEmitter<boolean>();

// This input does not exist on real line-chart and is devised to make tooltipTemplate
// These inputs do not exist on the real line-chart and is devised to make tooltipTemplate
// testable without using the real implementation.
@Input() tooltipDataForTesting: TooltipDatum[] = [];
@Input() cursorLocForTesting: {x: number; y: number} = {x: 0, y: 0};
@Input() cursorLocationInDataCoordForTesting: {x: number; y: number} = {
x: 0,
y: 0,
};
@Input() cursorLocationForTesting: {x: number; y: number} = {x: 0, y: 0};

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

Expand Down Expand Up @@ -1142,7 +1148,8 @@ describe('scalar card', () => {
describe('tooltip', () => {
function buildTooltipDatum(
metadata?: ScalarCardSeriesMetadata,
point: Partial<ScalarCardPoint> = {}
point: Partial<ScalarCardPoint> = {},
pixelLocation: Point = {x: 0, y: 0}
): TooltipDatum<ScalarCardSeriesMetadata, ScalarCardPoint> {
return {
id: metadata?.id ?? 'a',
Expand All @@ -1165,6 +1172,7 @@ describe('scalar card', () => {
relativeTimeInMs: 0,
...point,
},
pixelLocation,
};
}

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

function setCursorLocation(
fixture: ComponentFixture<ScalarCardContainer>,
cursorLocInDataCoord?: {x: number; y: number}
cursorLocInDataCoord?: {x: number; y: number},
cursorLocationInPixels?: Point
) {
const lineChart = fixture.debugElement.query(Selector.LINE_CHART);

lineChart.componentInstance.cursorLocForTesting = cursorLocInDataCoord;
if (cursorLocInDataCoord) {
lineChart.componentInstance.cursorLocationInDataCoordForTesting =
cursorLocInDataCoord;
}
if (cursorLocationInPixels) {
lineChart.componentInstance.cursorLocationForTesting =
cursorLocationInPixels;
}
lineChart.componentInstance.changeDetectorRef.markForCheck();
}

Expand Down Expand Up @@ -1616,6 +1632,10 @@ describe('scalar card', () => {
y: 1000,
value: 1000,
wallTime: new Date('2020-01-01').getTime(),
},
{
x: 0,
y: 100,
}
),
buildTooltipDatum(
Expand All @@ -1634,6 +1654,10 @@ describe('scalar card', () => {
y: -500,
value: -500,
wallTime: new Date('2020-12-31').getTime(),
},
{
x: 50,
y: 0,
}
),
buildTooltipDatum(
Expand All @@ -1652,26 +1676,30 @@ describe('scalar card', () => {
y: 3,
value: 3,
wallTime: new Date('2021-01-01').getTime(),
},
{
x: 1000,
y: 30,
}
),
]);
setCursorLocation(fixture, {x: 500, y: -100});
setCursorLocation(fixture, {x: 500, y: -100}, {x: 50, y: 0});
fixture.detectChanges();
assertTooltipRows(fixture, [
['', 'Row 2', '-500', '1,000', anyString, anyString],
['', 'Row 1', '1000', '0', anyString, anyString],
['', 'Row 3', '3', '10,000', anyString, anyString],
]);

setCursorLocation(fixture, {x: 500, y: 600});
setCursorLocation(fixture, {x: 500, y: 600}, {x: 50, y: 80});
fixture.detectChanges();
assertTooltipRows(fixture, [
['', 'Row 1', '1000', '0', anyString, anyString],
['', 'Row 2', '-500', '1,000', anyString, anyString],
['', 'Row 3', '3', '10,000', anyString, anyString],
]);

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

// Right between row 1 and row 2. When tied, original order is used.
setCursorLocation(fixture, {x: 500, y: 250});
setCursorLocation(fixture, {x: 500, y: 250}, {x: 25, y: 50});
fixture.detectChanges();
assertTooltipRows(fixture, [
['', 'Row 1', '1000', '0', anyString, anyString],
Expand Down
4 changes: 3 additions & 1 deletion tensorboard/webapp/metrics/views/right_pane/settings_view_component.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,9 @@ export class SettingsViewComponent {
{value: TooltipSort.ALPHABETICAL, displayText: 'Alphabetical'},
{value: TooltipSort.ASCENDING, displayText: 'Ascending'},
{value: TooltipSort.DESCENDING, displayText: 'Descending'},
{value: TooltipSort.NEAREST, displayText: 'Nearest'},
{value: TooltipSort.NEAREST, displayText: 'Nearest Pixel'},
{value: TooltipSort.NEAREST_X, displayText: 'Nearest X'},
{value: TooltipSort.NEAREST_Y, displayText: 'Nearest Y'},
];
@Input() tooltipSort!: TooltipSort;
@Output() tooltipSortChanged = new EventEmitter<TooltipSort>();
Expand Down
11 changes: 8 additions & 3 deletions tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,12 @@ import {
SettingsConverter,
TEST_ONLY,
} from './persistent_settings_data_source';
import {BackendSettings, PersistableSettings, ThemeValue} from './types';
import {
BackendSettings,
PersistableSettings,
ThemeValue,
TooltipSort,
} from './types';

describe('persistent_settings data_source test', () => {
let getItemSpy: jasmine.Spy;
Expand Down Expand Up @@ -289,7 +294,7 @@ describe('persistent_settings data_source test', () => {

expect(actual).toEqual({
scalarSmoothing: 0.3,
tooltipSortString: 'ascending',
tooltipSortString: 'ascending' as TooltipSort,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use TooltipSort.ASCENDING?

notificationLastReadTimeInMs: 3,
});
});
Expand All @@ -311,7 +316,7 @@ describe('persistent_settings data_source test', () => {

expect(actual).toEqual({
scalarSmoothing: 0.5,
tooltipSortString: 'default',
tooltipSortString: 'default' as TooltipSort,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use TooltipSort.DEFAULT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the test is testing deserialization from browser storage, I prefer testing the string literal.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very similar to my other response: The only serialized form of the string exists in LocalStorage. The contract of dataSource.getSettings() says it will return a TooltipSort object, so that seems to be the type you should use for the test.

I think the reasoning you are using would apply to setSettings() tests but it doesn't really apply to getSettings() tests.

notificationLastReadTimeInMs: 100,
});
});
Expand Down
18 changes: 16 additions & 2 deletions tensorboard/webapp/persistent_settings/_data_source/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,20 @@ export enum ThemeValue {
DARK = 'dark',
}

// When adding a new value to the enum, please implement the deserializer on
// data_source/metrics_data_source.ts.
// When editing a value of the enum, please write a backward compatible
// deserializer in tensorboard/webapp/metrics/store/metrics_reducers.ts
export enum TooltipSort {
DEFAULT = 'default',
ALPHABETICAL = 'alphabetical',
ASCENDING = 'ascending',
DESCENDING = 'descending',
NEAREST = 'nearest',
NEAREST_X = 'nearest_x',
NEAREST_Y = 'nearest_Y',
}

/**
* Global settings that the backend remembers. `declare`d so properties do not
* get mangled or mangled differently when a version compiler changes.
Expand All @@ -28,7 +42,7 @@ export enum ThemeValue {
*/
export declare interface BackendSettings {
scalarSmoothing?: number;
tooltipSort?: string;
tooltipSort?: TooltipSort;
ignoreOutliers?: boolean;
autoReload?: boolean;
autoReloadPeriodInMs?: number;
Expand All @@ -50,7 +64,7 @@ export declare interface BackendSettings {
*/
export interface PersistableSettings {
scalarSmoothing?: number;
tooltipSortString?: string;
tooltipSortString?: TooltipSort;
ignoreOutliers?: boolean;
autoReload?: boolean;
autoReloadPeriodInMs?: number;
Expand Down
3 changes: 2 additions & 1 deletion tensorboard/webapp/widgets/line_chart_v2/sub_view/line_chart_interactive_view.ng.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,8 @@
[ngTemplateOutlet]="tooltipTemplate ? tooltipTemplate : defaultTooltip"
[ngTemplateOutletContext]="{
data: cursoredData,
cursorLocationInDataCoord: cursorLocationInDataCoord
cursorLocationInDataCoord: cursorLocationInDataCoord,
cursorLocation: cursorLocation
}"
></ng-container>
</div>
Expand Down
Loading