Default single step selection to max step; Refactor TimeSelection type so end is always set. #6364

bmd3k · 2023-05-03T13:25:53Z

Motivation for features / changes

We are changing single step selection to default to the maximum step. We think this is more useful than making the default the minimum step.

Technical description of changes

Change TimeSelection type so that start is nullable while end is not and adjust the code to fit.
Where default value is set to "min" step, instead set it to the "max" step.

One tricky aspect: In some places we were checking that end was falsy or truthy to determine if it was set. There was a latent bug that if end was set to 0 then it would be considered unset in those cases. This was not a big problem because end was rarely set to 0. But start is often set to 0 so those cases needed to be carefully tracked down and fixed.

Another tricky aspect: The TimeSelection refactor does not cleanly import into the internal repo. I have three CLs that accompagny this change.

cl/528745120 adjusts or comments out code to allow the import to succeed. It will be submitted prior to merging this PR.
cl/528745440 is a set of harmless screenshot-updates. It will be patched into the import CL and submitted at the same time as the import.
cl/528745657 is a set of changes to uncomment out the code from the first CL and make remaining adjustments. It will be submitted after the import.

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

I tested it manually quite a bit.
I imported it into the internal repo and ensured that end-to-end tests pass (after adjusting for the change in behavior).

rileyajones · 2023-05-03T17:55:28Z

tensorboard/webapp/widgets/card_fob/card_fob_types.ts

 export interface TimeSelection {
-  start: {step: number};
-  end: {step: number} | null;
+  start: {step: number} | null;


The decision to have end be end: {step: number} | null rather than end?: {step: number} has lead to a lot of clunky code (especially in tests). I would love to see this changed and I think this may be a good time for this update. However, I recognize that this is not a small change so I'll leave it to your discretion.

rileyajones

I will get back to reviewing this tomorrow, I only made it ~50% of the way through today but I wanted to leave some initial comments for now.

This has clearly taken a lot of work and I do think it is an improvement so I'm not going to block the effort, however, given the number of PR/CLs that are going to be required to get his checked in please think carefully about the cost benefit of making such a large change.

Given the existing plans to remove the global values for step and range selection along with the check boxes I'm not sure how this will actually be different from the existing experience. This is definitely an improvement while the check boxes and default values stay around though.
The same result could likely have been achieved with only updates to the fob controller, the rangeSelectionEnabled reducer, the stepSelectionEnabled reducer, and the getCardTimeSelection selector
At the redux level there is always both a start and end value. This value is then clipped at the container level. You could have just removed that, removed the ability for end to be null, and updated the data table and card fob controller to read the value of getCardRangeSelectionEnabled (like they should).

rileyajones · 2023-05-03T22:28:13Z

tensorboard/webapp/metrics/store/metrics_reducers.ts

        };
      }
-      if (!linkedTimeSelection.end) {
+      if (linkedTimeSelection.start === null) {


nit: While your solution is technically more correct I'm not sure the distinction between null and falsey is important here.

Suggested change

if (linkedTimeSelection.start === null) {

if (!linkedTimeSelection.start) {

rileyajones · 2023-05-03T22:30:24Z

tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts


    let nextStepIndexMetaData: CardStepIndexMetaData | null = null;
-    if (timeSelection.end === null) {
+    if (timeSelection.start === null) {


nit

Suggested change

if (timeSelection.start === null) {

if (!timeSelection.start) {

rileyajones · 2023-05-03T22:30:36Z

tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts

-    if (steps.indexOf(timeSelection.start.step) !== -1)
-      return [timeSelection.start.step];
+  // Single selection: returns end step if matching any step in the list, otherwise returns nothing.
+  if (timeSelection.start === null) {


nit

Suggested change

if (timeSelection.start === null) {

if (!timeSelection.start) {

rileyajones · 2023-05-03T22:34:20Z

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

  @Input() timeSelection!: {
-    start: {step: number};
-    end: {step: number} | null;
+    start: {step: number} | null;
+    end: {step: number};
  } | null;


I'm aware that you are just updating the existing code but it would be nice to actually use the type here

Suggested change

@Input() timeSelection!: {

start: {step: number};

end: {step: number} | null;

start: {step: number} | null;

end: {step: number};

} | null;

@Input() timeSelection!: TimeSelection | null`

rileyajones · 2023-05-03T22:39:24Z

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

          (step) =>
-            step >= linkedTimeSelection.startStep &&
-            step <= linkedTimeSelection.endStep!
+            step >= linkedTimeSelection.startStep! &&


This is a rare situation where the difference between null and undefined actually matters. If you do decide to take my advice to change to using undefined rather than null you will need to this.
For reference numbers are never greater than nor less than undefined but null is treated as 0.

rileyajones · 2023-05-03T22:40:33Z

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

    ]).pipe(
      map(([timeSelection, singleSelectionHeaders, rangeSelectionHeaders]) => {
-        if (!timeSelection || timeSelection.end === null) {
+        if (!timeSelection || timeSelection.start === null) {


nit

Suggested change

if (!timeSelection || timeSelection.start === null) {

if (!timeSelection || !timeSelection.start) {

rileyajones · 2023-05-03T22:42:27Z

tensorboard/webapp/metrics/store/metrics_store_internal_utils.ts

-      return [timeSelection.start.step];
+  // Single selection: returns end step if matching any step in the list, otherwise returns nothing.
+  if (timeSelection.start === null) {
+    if (steps.indexOf(timeSelection.end.step) !== -1)


Because we are not actually using the index this seems like an excellent place to use the includes function

Suggested change

if (steps.indexOf(timeSelection.end.step) !== -1)

if (steps.includes(timeSelection.end.step))

bmd3k · 2023-05-08T16:39:48Z

This has clearly taken a lot of work and I do think it is an improvement so I'm not going to block the effort, however, given the number of PR/CLs that are going to be required to get his checked in please think carefully about the cost benefit of making such a large change.

Good advice. I will close this and have a simpler change in #6372.

Set end step for single-step selection. Default to max.

cca2a42

bmd3k requested a review from rileyajones May 3, 2023 14:42

rileyajones reviewed May 3, 2023

View reviewed changes

bmd3k closed this May 8, 2023

rileyajones mentioned this pull request May 8, 2023

Use maximum step as default for single step selection. #6372

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Default single step selection to max step; Refactor TimeSelection type so end is always set. #6364

Default single step selection to max step; Refactor TimeSelection type so end is always set. #6364

Uh oh!

bmd3k commented May 3, 2023

Uh oh!

rileyajones May 3, 2023 •

edited

Loading

Uh oh!

rileyajones left a comment

Uh oh!

rileyajones May 3, 2023

Uh oh!

rileyajones May 3, 2023

Uh oh!

rileyajones May 3, 2023

Uh oh!

rileyajones May 3, 2023

Uh oh!

rileyajones May 3, 2023

Uh oh!

rileyajones May 3, 2023

Uh oh!

rileyajones May 3, 2023

Uh oh!

bmd3k commented May 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	if (linkedTimeSelection.start === null) {
	if (!linkedTimeSelection.start) {

	if (timeSelection.start === null) {
	if (!timeSelection.start) {

	if (!timeSelection \|\| timeSelection.start === null) {
	if (!timeSelection \|\| !timeSelection.start) {

	if (steps.indexOf(timeSelection.end.step) !== -1)
	if (steps.includes(timeSelection.end.step))

Default single step selection to max step; Refactor TimeSelection type so end is always set. #6364

Default single step selection to max step; Refactor TimeSelection type so end is always set. #6364

Uh oh!

Conversation

bmd3k commented May 3, 2023

Motivation for features / changes

Technical description of changes

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

Uh oh!

rileyajones May 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rileyajones left a comment

Choose a reason for hiding this comment

Uh oh!

rileyajones May 3, 2023

Choose a reason for hiding this comment

Uh oh!

rileyajones May 3, 2023

Choose a reason for hiding this comment

Uh oh!

rileyajones May 3, 2023

Choose a reason for hiding this comment

Uh oh!

rileyajones May 3, 2023

Choose a reason for hiding this comment

Uh oh!

rileyajones May 3, 2023

Choose a reason for hiding this comment

Uh oh!

rileyajones May 3, 2023

Choose a reason for hiding this comment

Uh oh!

rileyajones May 3, 2023

Choose a reason for hiding this comment

Uh oh!

bmd3k commented May 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rileyajones May 3, 2023 •

edited

Loading