Skip to content
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.

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

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

Merged
merged 1 commit into from
May 9, 2023

Conversation

bmd3k
Copy link
Contributor

@bmd3k bmd3k commented May 8, 2023

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.

This also means that when step selection changes from single selection to range selection, the minimum value of the chart is added as the start of the range and the previous single step now becomes the end of the range. Similarly, when step selection changes from range selection to single selection, the new single selection is the previous maximum value of the range.

Technical description of changes

Where default value is set to "min" step, instead set it to the "max" step. Where range selection is enabled, ensure the new start of the range is the "min" step.

I have three internal 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).

@bmd3k bmd3k force-pushed the step-selection-end-as-default-small branch from 35e107e to ce935b9 Compare May 8, 2023 16:32
@bmd3k bmd3k changed the title Max is default for single step selection. Use maximum step as default for single step selection. May 8, 2023
@bmd3k bmd3k requested a review from rileyajones May 8, 2023 16:40
Copy link
Contributor

@rileyajones rileyajones left a comment

Choose a reason for hiding this comment

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

This is a bit confusing at first. But with the context I have around the problem, this makes sense as a solution.

Comment on lines +610 to +613
timeSelection = {
start: timeSelection.end ?? timeSelection.start,
end: null,
};
Copy link
Contributor

Choose a reason for hiding this comment

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

As discussed in #6364 I would love to remove the ability for end to be null. If range selection is disabled then the nd value shouldn't be used so I don't see a reason to remove it here.

Suggested change
timeSelection = {
start: timeSelection.end ?? timeSelection.start,
end: null,
};
timeSelection.start = timeSelection.end ?? timeSelection.start;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't intend to change the data model as part of this PR. Besides, I don't really agree with the sentiment that motivates that particular change.

I set end to null here to be clear that we are returning to single step selection. The end gets set to null, anyway, as a side-effect of calling formatTimeSelection but I find it unlikely that somebody will read this code and understand that.

@bmd3k bmd3k merged commit f52eb8c into tensorflow:master May 9, 2023
14 checks passed
@bmd3k bmd3k mentioned this pull request Aug 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants