Skip to content

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

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 1 commit into from
May 9, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 9 additions & 5 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 @@ -1141,8 +1141,8 @@ const reducer = createReducer(

// Updates cardStepIndex only when toggle to enable linked time.
if (nextLinkedTimeEnabled) {
const {min} = state.stepMinMax;
const startStep = min === Infinity ? 0 : min;
const {max} = state.stepMinMax;
const startStep = max === -Infinity ? 0 : max;
nextLinkedTimeSelection = state.linkedTimeSelection ?? {
start: {step: startStep},
end: null,
Expand Down Expand Up @@ -1202,15 +1202,19 @@ const reducer = createReducer(
};
}
if (!linkedTimeSelection.end) {
// Enabling range selection from single selection selects the first
// step as the start of the range. The previous start step from single
// selection is now the end step.
linkedTimeSelection = {
...linkedTimeSelection,
end: {step: state.stepMinMax.max},
start: {step: state.stepMinMax.min},
end: linkedTimeSelection.start,
};
}
} else {
if (linkedTimeSelection) {
// Disabling range selection keeps the largest step from the range.
linkedTimeSelection = {
...linkedTimeSelection,
start: linkedTimeSelection.end ?? linkedTimeSelection.start,
end: null,
};
}
Expand Down
10 changes: 5 additions & 5 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 @@ -4057,8 +4057,8 @@ describe('metrics reducers', () => {

const state2 = reducers(state1, actions.rangeSelectionToggled({}));
expect(state2.linkedTimeSelection).toEqual({
start: {step: 100},
end: {step: -Infinity},
start: {step: Infinity},
end: {step: 100},
});
});

Expand All @@ -4073,7 +4073,7 @@ describe('metrics reducers', () => {

const state2 = reducers(state1, actions.rangeSelectionToggled({}));
expect(state2.linkedTimeSelection).toEqual({
start: {step: 100},
start: {step: 1000},
end: null,
});
});
Expand Down Expand Up @@ -4358,15 +4358,15 @@ describe('metrics reducers', () => {
});
});

it('sets linkedTimeSelection to min step when linkedTimeSelection is null before toggling', () => {
it('sets linkedTimeSelection to max step when linkedTimeSelection is null before toggling', () => {
const state1 = buildMetricsState({
stepMinMax: {min: 10, max: 100},
});

const state2 = reducers(state1, actions.linkedTimeToggled({}));

expect(state2.linkedTimeSelection).toEqual({
start: {step: 10},
start: {step: 100},
end: null,
});
});
Expand Down
34 changes: 26 additions & 8 deletions tensorboard/webapp/metrics/store/metrics_selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,7 @@ export const getMetricsLinkedTimeSelectionSetting = createSelector(
if (!state.linkedTimeSelection) {
return {
start: {
step: stepMinMax.min,
step: stepMinMax.max,
},
end: null,
};
Expand Down Expand Up @@ -588,15 +588,33 @@ export const getMetricsCardTimeSelection = createSelector(
globalRangeSelectionEnabled
);

const startStep = cardState.timeSelection?.start.step ?? minMaxStep.minStep;
const endStep = cardState.timeSelection?.end?.step ?? minMaxStep.maxStep;
let timeSelection = cardState.timeSelection;
if (!timeSelection) {
timeSelection = {
start: {step: minMaxStep.minStep},
end: {step: minMaxStep.maxStep},
};
}
if (rangeSelectionEnabled) {
if (!timeSelection.end) {
// Enabling range selection from single selection selects the first
// step as the start of the range. The previous start step from single
// selection is now the end step.
timeSelection = {
start: {step: minMaxStep.minStep},
end: timeSelection.start,
};
}
} else {
// Disabling range selection keeps the largest step from the range.
timeSelection = {
start: timeSelection.end ?? timeSelection.start,
end: null,
};
Comment on lines +610 to +613
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.

}

// The default time selection
return formatTimeSelection(
{
start: {step: startStep},
end: {step: endStep},
},
timeSelection,
minMaxStep,
rangeSelectionEnabled
);
Expand Down
12 changes: 6 additions & 6 deletions 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 @@ -549,7 +549,7 @@ describe('metrics selectors', () => {
).toBeUndefined();
});

it('uses max step as end value if none exists', () => {
it('uses min step as start value if end value does not exists', () => {
const state = appStateFromMetricsState(
buildMetricsState({
...partialState,
Expand All @@ -562,7 +562,7 @@ describe('metrics selectors', () => {
maxStep: 10,
},
timeSelection: {
start: {step: 0},
start: {step: 5},
end: null,
},
},
Expand All @@ -572,7 +572,7 @@ describe('metrics selectors', () => {

expect(selectors.getMetricsCardTimeSelection(state, 'card1')).toEqual({
start: {step: 0},
end: {step: 10},
end: {step: 5},
});
});

Expand Down Expand Up @@ -619,7 +619,7 @@ describe('metrics selectors', () => {
);

expect(selectors.getMetricsCardTimeSelection(state, 'card1')).toEqual({
start: {step: 0},
start: {step: 5},
end: null,
});
});
Expand All @@ -642,7 +642,7 @@ describe('metrics selectors', () => {
);

expect(selectors.getMetricsCardTimeSelection(state, 'card1')).toEqual({
start: {step: 0},
start: {step: 5},
end: null,
});
});
Expand Down Expand Up @@ -1392,7 +1392,7 @@ describe('metrics selectors', () => {
})
);
expect(selectors.getMetricsLinkedTimeSelectionSetting(state)).toEqual({
start: {step: 0},
start: {step: 1000},
end: null,
});
});
Expand Down
2 changes: 1 addition & 1 deletion 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 @@ -3963,7 +3963,7 @@ describe('scalar card', () => {
);
const fakeEvent = new MouseEvent('mousemove', {
clientX: 25 + controllerStartPosition,
movementX: 1,
movementX: -1,
});
testController.mouseMove(fakeEvent);

Expand Down