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 5 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
4 changes: 2 additions & 2 deletions WORKSPACE
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,11 +24,11 @@ http_archive(
load("@bazel_skylib//lib:versions.bzl", "versions")

versions.check(
# TODO(https://github.com/tensorflow/tensorboard/issues/6115): Support building TensorBoard with Bazel version >= 6.0.0
maximum_bazel_version = "5.4.0",
# Keep this version in sync with:
# * The BAZEL environment variable defined in .github/workflows/ci.yml, which is used for CI and nightly builds.
minimum_bazel_version = "4.2.2",
# TODO(https://github.com/tensorflow/tensorboard/issues/6115): Support building TensorBoard with Bazel version >= 6.0.0
maximum_bazel_version = "5.4.0",
)

http_archive(
Expand Down
1 change: 0 additions & 1 deletion tensorboard/webapp/metrics/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ tf_ts_library(
],
deps = [
":internal_types",
"//tensorboard/webapp/metrics/store:types",
],
)

Expand Down
2 changes: 1 addition & 1 deletion tensorboard/webapp/metrics/actions/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ tf_ts_library(
"index.ts",
],
deps = [
"//tensorboard/webapp/metrics:internal_types",
"//tensorboard/webapp/metrics:types",
"//tensorboard/webapp/metrics/data_source",
"//tensorboard/webapp/metrics/views/card_renderer:scalar_card_types",
"//tensorboard/webapp/util:dom",
Expand Down
2 changes: 1 addition & 1 deletion tensorboard/webapp/metrics/actions/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
PluginType,
TooltipSort,
XAxisType,
} from '../internal_types';
} from '../types';
import {
ColumnHeader,
SortingInfo,
Expand Down
12 changes: 0 additions & 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 @@ -23,18 +23,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
2 changes: 1 addition & 1 deletion tensorboard/webapp/metrics/metrics_module.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ export function getMetricsIgnoreOutliersSettingFactory() {

export function getMetricsTooltipSortSettingFactory() {
return createSelector(getMetricsTooltipSort, (tooltipSort) => {
return {tooltipSortString: String(tooltipSort)};
return {tooltipSort: String(tooltipSort)};
});
}

Expand Down
6 changes: 3 additions & 3 deletions tensorboard/webapp/metrics/store/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ tf_ts_library(
"//tensorboard/webapp/app_routing:types",
"//tensorboard/webapp/app_routing/actions",
"//tensorboard/webapp/core/actions",
"//tensorboard/webapp/metrics:internal_types",
"//tensorboard/webapp/metrics:types",
"//tensorboard/webapp/metrics:utils",
"//tensorboard/webapp/metrics/actions",
"//tensorboard/webapp/metrics/data_source",
Expand Down Expand Up @@ -68,7 +68,7 @@ tf_ts_library(
],
deps = [
"//tensorboard/webapp/app_routing:namespaced_state_reducer_helper",
"//tensorboard/webapp/metrics:internal_types",
"//tensorboard/webapp/metrics:types",
"//tensorboard/webapp/metrics/data_source",
"//tensorboard/webapp/metrics/views/card_renderer:scalar_card_types",
"//tensorboard/webapp/types",
Expand All @@ -92,8 +92,8 @@ tf_ts_library(
"//tensorboard/webapp/app_routing:types",
"//tensorboard/webapp/app_routing/actions",
"//tensorboard/webapp/core/actions",
"//tensorboard/webapp/metrics:internal_types",
"//tensorboard/webapp/metrics:test_lib",
"//tensorboard/webapp/metrics:types",
"//tensorboard/webapp/metrics/actions",
"//tensorboard/webapp/metrics/data_source",
"//tensorboard/webapp/persistent_settings",
Expand Down
24 changes: 6 additions & 18 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 @@ -44,7 +44,7 @@ import {
SCALARS_SMOOTHING_MIN,
TooltipSort,
URLDeserializedState,
} from '../internal_types';
} from '../types';
import {groupCardIdWithMetdata} from '../utils';
import {ColumnHeaderType} from '../views/card_renderer/scalar_card_types';
import {
Expand Down 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.tooltipSort &&
Object.values(TooltipSort).includes(partialSettings.tooltipSort)
) {
metricsSettings.tooltipSort = partialSettings.tooltipSort;
}
if (typeof partialSettings.timeSeriesCardMinWidth === 'number') {
metricsSettings.cardMinWidth = partialSettings.timeSeriesCardMinWidth;
Expand Down
18 changes: 9 additions & 9 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 @@ -26,13 +26,6 @@ import {
ScalarStepDatum,
TagMetadata as DataSourceTagMetadata,
} from '../data_source';
import {
CardId,
CardMetadata,
HistogramMode,
TooltipSort,
XAxisType,
} from '../internal_types';
import {
buildDataSourceTagMetadata,
buildMetricsSettingsState,
Expand All @@ -46,6 +39,13 @@ import {
createScalarStepData,
createTimeSeriesData,
} from '../testing';
import {
CardId,
CardMetadata,
HistogramMode,
TooltipSort,
XAxisType,
} from '../types';
import {reducers} from './metrics_reducers';
import {getCardId, getPinnedCardId} from './metrics_store_internal_utils';
import {
Expand Down Expand Up @@ -2359,7 +2359,7 @@ describe('metrics reducers', () => {
globalSettingsLoaded({
partialSettings: {
ignoreOutliers: true,
tooltipSortString: 'descending',
tooltipSort: 'descending' as TooltipSort,
},
})
);
Expand Down Expand Up @@ -2387,7 +2387,7 @@ describe('metrics reducers', () => {
beforeState,
globalSettingsLoaded({
partialSettings: {
tooltipSortString: 'yo',
tooltipSort: 'yo' as TooltipSort,
},
})
);
Expand Down
2 changes: 1 addition & 1 deletion tensorboard/webapp/metrics/store/metrics_selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ import {
TimeSelection,
TooltipSort,
XAxisType,
} from '../internal_types';
} from '../types';
import {ColumnHeader} from '../views/card_renderer/scalar_card_types';
import * as storeUtils from './metrics_store_internal_utils';
import {
Expand Down
2 changes: 1 addition & 1 deletion tensorboard/webapp/metrics/store/metrics_selectors_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ limitations under the License.
import {DataLoadState} from '../../types/data';
import {nextElementId} from '../../util/dom';
import {PluginType} from '../data_source';
import {HistogramMode, TooltipSort, XAxisType} from '../internal_types';
import {
appStateFromMetricsState,
buildMetricsSettingsState,
Expand All @@ -26,6 +25,7 @@ import {
createScalarStepData,
createTimeSeriesData,
} from '../testing';
import {HistogramMode, TooltipSort, XAxisType} from '../types';
import * as selectors from './metrics_selectors';

describe('metrics selectors', () => {
Expand Down
2 changes: 1 addition & 1 deletion tensorboard/webapp/metrics/store/metrics_types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import {
TimeSelection,
TooltipSort,
XAxisType,
} from '../internal_types';
} from '../types';
import {ColumnHeader} from '../views/card_renderer/scalar_card_types';

export const METRICS_FEATURE_KEY = 'metrics';
Expand Down
13 changes: 13 additions & 0 deletions tensorboard/webapp/metrics/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,3 +13,16 @@ See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
export * from './internal_types';

// When adding a new value to the enum, please implement the deserializer on
Copy link
Contributor

Choose a reason for hiding this comment

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

To be honest, I didn't realize that types is just reexporting internal_types. That's odd.

Maybe (as a followup PR, please not in this one) we should just get rid of internal_types and move everything into this file. The indirection is unnecessary and we shouldn't fool ourselves into thinking anything in internal_types is actually internal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

+1 that totally makes sense as a followup

// 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_Y = 'nearest_Y',
}
6 changes: 4 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,8 @@
<tbody>
<ng-container
*ngFor="
let datum of getCursorAwareTooltipData(tooltipData, cursorLoc);
let datum of getCursorAwareTooltipData(tooltipData,
cursorLocationInDataCoord, cursorLocation);
trackBy: trackByTooltipDatum
"
>
Expand Down
22 changes: 14 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,6 @@ import {TimeSelectionView} from './utils';

type ScalarTooltipDatum = TooltipDatum<
ScalarCardSeriesMetadata & {
distSqToCursor: number;
closest: boolean;
}
>;
Expand Down Expand Up @@ -163,27 +162,30 @@ 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
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.distToCursorPixels) {
minDist = scalarTooltipData[index].metadata.distToCursorPixels;
minIndex = index;
}
}
Expand All @@ -199,7 +201,11 @@ 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_Y:
return scalarTooltipData.sort((a, b) => {
return a.metadata.distToCursorY - b.metadata.distToCursorY;
});
case TooltipSort.DEFAULT:
case TooltipSort.ALPHABETICAL:
Expand Down
Loading