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

rileyajones · 2022-12-27T20:19:52Z

~~Tests coming soon~~

Motivation for features / changes

#5854
The scalars dashboard handled the "nearest" value displayed in the tooltip differently than the time series dashboard and some users prefer that.

Technical description of changes

Add three new types to the TooltipSort enum
Because tooltip sorting is a persisted setting I have moved the type declaration to the persistent settings types. This prevents a dependency cycle and allows direct typing of the persisted settings object thus removing the need to manually deserialize it.
Pipe the cursor position relative to the chart through from the line_chart_interactive_view to the scalar_card_component

Screenshots of UI changes

New tooltip options in settings menu

All of these screenshots have my cursor at the top right of the chart.

Nearest (Before):

Nearest Pixel (After):

Nearest Y:

Detailed steps to verify changes work correctly (as executed by you)

I could only test this using internal tensorboard. For those looking to follow along start tensorboard with this PR merged in (e.g. cl/498013225) and check this experiment 3630455328016159781

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

tensorboard/webapp/persistent_settings/_data_source/types.ts

bmd3k · 2023-01-04T15:27:09Z

tensorboard/webapp/metrics/store/metrics_reducers_test.ts

@@ -2359,7 +2359,7 @@ describe('metrics reducers', () => {
        globalSettingsLoaded({
          partialSettings: {
            ignoreOutliers: true,
-            tooltipSortString: 'descending',
+            tooltipSortString: 'descending' as TooltipSort,


Can you just use TooltipSort.DESCENDING?

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

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.

bmd3k · 2023-01-04T15:32:43Z

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

@@ -68,6 +68,7 @@ export interface TooltipDatum<
  metadata: Metadata;
  closestPointIndex: number;
  point: PointDatum;


Can we get some consistency in naming to make the code clearer?

point and cursorLocationInDataCoord live in the same space but that's not clear.
pixelLocation and cursorLocation live in the same space but that's not clear.

Maybe instead:
pointInDataCoord and cursorInDataCoord and
pointInPixelCoord and cusorInPixelCoord?

Or maybe:
dataPoint and dataCursorLocation and
pixelPoint and pixelCursorLocation.

Another point of reference for naming is the quartet of functions getDomX, getDomY, getDataX, getDataY.

So that suggests:
dataPoint and dataCursorLocation and
domPoint and domCursorLocation.

bmd3k · 2023-01-04T15:36:16Z

tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source_test.ts

@@ -311,7 +316,7 @@ describe('persistent_settings data_source test', () => {

          expect(actual).toEqual({
            scalarSmoothing: 0.5,
-            tooltipSortString: 'default',
+            tooltipSortString: 'default' as TooltipSort,


Use TooltipSort.DEFAULT?

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

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.

tensorboard/webapp/metrics/store/metrics_reducers.ts

bmd3k · 2023-01-04T15:42:08Z

tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html

@@ -119,7 +119,8 @@
  <ng-template
    #tooltip
    let-tooltipData="data"
-    let-cursorLoc="cursorLocationInDataCoord"
+    let-cursorLocationInDataCoord="cursorLocationInDataCoord"


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

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).

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.

bmd3k · 2023-01-04T15:42:26Z

tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html

@@ -136,7 +137,7 @@
      <tbody>
        <ng-container
          *ngFor="
-            let datum of getCursorAwareTooltipData(tooltipData, cursorLoc);
+            let datum of getCursorAwareTooltipData(tooltipData, cursorLocationInDataCoord, cursorLocation);


Nit: Can this be line wrapped?

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

bmd3k · 2023-01-09T18:06:50Z

tensorboard/webapp/metrics/store/metrics_reducers_test.ts

@@ -2359,7 +2359,7 @@ describe('metrics reducers', () => {
        globalSettingsLoaded({
          partialSettings: {
            ignoreOutliers: true,
-            tooltipSortString: 'descending',
+            tooltipSortString: 'descending' as TooltipSort,


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.

bmd3k · 2023-01-09T18:10:26Z

tensorboard/webapp/metrics/views/card_renderer/scalar_card_component.ng.html

@@ -119,7 +119,8 @@
  <ng-template
    #tooltip
    let-tooltipData="data"
-    let-cursorLoc="cursorLocationInDataCoord"
+    let-cursorLocationInDataCoord="cursorLocationInDataCoord"


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.

bmd3k · 2023-01-09T18:14:40Z

tensorboard/webapp/persistent_settings/_data_source/persistent_settings_data_source_test.ts

@@ -311,7 +316,7 @@ describe('persistent_settings data_source test', () => {

          expect(actual).toEqual({
            scalarSmoothing: 0.5,
-            tooltipSortString: 'default',
+            tooltipSortString: 'default' as TooltipSort,


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.

WORKSPACE

bmd3k · 2023-01-09T18:31:11Z

tensorboard/webapp/metrics/types.ts

@@ -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


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.

+1 that totally makes sense as a followup

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

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

bmd3k · 2023-01-09T18:47:36Z

Also, please update the PR description. You have tests now and you aren't supporting 'Nearest X' for example.

tensorflow#6116) ~Tests coming soon~ ## Motivation for features / changes tensorflow#5854 The scalars dashboard handled the "nearest" value displayed in the tooltip differently than the time series dashboard and some users prefer that. ## Technical description of changes * Add three new types to the `TooltipSort` enum * Because tooltip sorting is a persisted setting I have moved the type declaration to the persistent settings types. This prevents a dependency cycle and allows direct typing of the persisted settings object thus removing the need to manually deserialize it. * Pipe the cursor position relative to the chart through from the `line_chart_interactive_view` to the `scalar_card_component` ## Screenshots of UI changes New tooltip options in settings menu ![image](https://user-images.githubusercontent.com/78179109/210456117-a5932e11-3796-4298-ad9f-79c582286fa8.png) All of these screenshots have my cursor at the top right of the chart. Nearest (Before): ![image](https://user-images.githubusercontent.com/78179109/209720008-6a16d19b-64d6-40c3-8ce5-b94cc3ef455e.png) Nearest Pixel (After): ![image](https://user-images.githubusercontent.com/78179109/210456192-fb9a54e5-3239-492d-8987-862a4a142659.png) Nearest Y: ![image](https://user-images.githubusercontent.com/78179109/209719876-29779bde-3dc5-474f-a5d3-831fc3b068d5.png) ## Detailed steps to verify changes work correctly (as executed by you) I could only test this using internal tensorboard. For those looking to follow along start tensorboard with this PR merged in (e.g. cl/498013225) and check this experiment `3630455328016159781`

add three new tooltip sort orders

435d8d4

rileyajones marked this pull request as ready for review December 27, 2022 20:41

rileyajones requested review from bmd3k and removed request for bmd3k December 27, 2022 22:34

bmd3k reviewed Dec 28, 2022

View reviewed changes

respond to code review comments

eb5123d

rileyajones force-pushed the nearest-pixel branch from 1040678 to eb5123d Compare January 3, 2023 21:27

rileyajones requested a review from bmd3k January 3, 2023 23:03

bmd3k reviewed Jan 4, 2023

View reviewed changes

rileyajones added 2 commits January 4, 2023 18:05

remove nearest_x, rename tooltipSortSting

edf2125

update closest logic

7140374

rileyajones force-pushed the nearest-pixel branch 2 times, most recently from e3866e2 to b58dc0e Compare January 6, 2023 20:06

resolve circular dependency issue

e3ce6e7

rileyajones force-pushed the nearest-pixel branch from b58dc0e to e3ce6e7 Compare January 6, 2023 22:24

rileyajones requested a review from bmd3k January 9, 2023 18:00

bmd3k approved these changes Jan 9, 2023

View reviewed changes

rename variables, remove workspace file

6d27028

rileyajones force-pushed the nearest-pixel branch from edf5e19 to 6d27028 Compare January 9, 2023 22:19

rileyajones merged commit 183ff2b into tensorflow:master Jan 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rileyajones commented Dec 27, 2022 •

edited

Loading

bmd3k Jan 4, 2023

rileyajones Jan 4, 2023

bmd3k Jan 9, 2023

bmd3k Jan 4, 2023

bmd3k Jan 4, 2023

rileyajones Jan 4, 2023

bmd3k Jan 9, 2023

bmd3k Jan 4, 2023

rileyajones Jan 4, 2023

bmd3k Jan 9, 2023

bmd3k Jan 4, 2023

bmd3k Jan 9, 2023

bmd3k Jan 9, 2023

bmd3k Jan 9, 2023

bmd3k Jan 9, 2023

rileyajones Jan 9, 2023

bmd3k commented Jan 9, 2023

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

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

Conversation

rileyajones commented Dec 27, 2022 • edited Loading

Motivation for features / changes

Technical description of changes

Screenshots of UI changes

Detailed steps to verify changes work correctly (as executed by you)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmd3k commented Jan 9, 2023

rileyajones commented Dec 27, 2022 •

edited

Loading