Skip to content

Commit 73f1609

Browse files
authored
Linked Time: Update histogram card to show linked time end step based on global range selection (#6263)
## Motivation for features / changes Now that the step selection is being read from redux I found a bug where the histogram card will show a linked time end step even if range selection is disabled. Note this could only happen if an end step was set a card. ## Technical description of changes I added another parameter to the observable that gets the value of the linked time selection in the histogram container card then conditionally removed the end value based on status of the global range selection setting. ## Screenshots of UI changes Before ![image](https://user-images.githubusercontent.com/78179109/227029314-9198f501-308e-47e6-8e6e-fafbfad692f8.png) After ![image](https://user-images.githubusercontent.com/78179109/227028830-2191560e-b0ff-4ef0-824f-6699e57d2efe.png) ## Detailed steps to verify changes work correctly (as executed by you) 1) Start tensorboard 2) Navigate to localhost:6006 3) Add a start step to two different scalar cards 4) Add an end step to one of the scalar cards 5) Enable link by step 6) Ensure range selection is not enabled and that end steps are not shown on either card 7) Ensure only the start step is shown on histogram cards 8) Enable range selection 9) Ensure an end step is shown on all cards
1 parent dca1819 commit 73f1609

File tree

4 files changed

+74
-5
lines changed

4 files changed

+74
-5
lines changed

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -42,17 +42,19 @@ import {
4242
getCardLoadState,
4343
getCardMetadata,
4444
getCardPinnedState,
45+
getCardStateMap,
4546
getCardTimeSeries,
4647
getMetricsHistogramMode,
4748
getMetricsLinkedTimeSelection,
49+
getMetricsRangeSelectionEnabled,
4850
getMetricsXAxisType,
4951
} from '../../store';
50-
import {getCardStateMap} from '../../../selectors';
5152
import {CardId, CardMetadata} from '../../types';
5253
import {CardRenderer} from '../metrics_view_types';
5354
import {getTagDisplayName} from '../utils';
5455
import {
5556
maybeClipTimeSelectionView,
57+
maybeOmitTimeSelectionEnd,
5658
maybeSetClosestStartStep,
5759
TimeSelectionView,
5860
} from './utils';
@@ -173,8 +175,9 @@ export class HistogramCardContainer implements CardRenderer, OnInit {
173175
this.linkedTimeSelection$ = combineLatest([
174176
this.store.select(getMetricsLinkedTimeSelection),
175177
this.steps$,
178+
this.store.select(getMetricsRangeSelectionEnabled),
176179
]).pipe(
177-
map(([linkedTimeSelection, steps]) => {
180+
map(([linkedTimeSelection, steps, rangeSelectionEnabled]) => {
178181
if (!linkedTimeSelection) return null;
179182

180183
let minStep = Infinity;
@@ -183,8 +186,12 @@ export class HistogramCardContainer implements CardRenderer, OnInit {
183186
minStep = Math.min(step, minStep);
184187
maxStep = Math.max(step, maxStep);
185188
}
186-
const linkedTimeSelectionView = maybeClipTimeSelectionView(
189+
const formattedTimeSelection = maybeOmitTimeSelectionEnd(
187190
linkedTimeSelection,
191+
rangeSelectionEnabled
192+
);
193+
const linkedTimeSelectionView = maybeClipTimeSelectionView(
194+
formattedTimeSelection,
188195
minStep,
189196
maxStep
190197
);

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

Lines changed: 30 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -39,9 +39,9 @@ import {
3939
import {buildNormalizedHistograms} from '../../../widgets/histogram/histogram_util';
4040
import {TruncatedPathModule} from '../../../widgets/text/truncated_path_module';
4141
import {
42+
metricsCardFullSizeToggled,
4243
stepSelectorToggled,
4344
timeSelectionChanged,
44-
metricsCardFullSizeToggled,
4545
} from '../../actions';
4646
import {PluginType} from '../../data_source';
4747
import * as selectors from '../../store/metrics_selectors';
@@ -287,6 +287,10 @@ describe('histogram card', () => {
287287
});
288288

289289
describe('linked time', () => {
290+
beforeEach(() => {
291+
store.overrideSelector(selectors.getMetricsRangeSelectionEnabled, true);
292+
});
293+
290294
it('dispatches timeSelectionChanged when HistogramComponent emits onLinkedTimeSelectionChanged event', () => {
291295
provideMockCardSeriesData(selectSpy, PluginType.HISTOGRAMS, 'card1');
292296
store.overrideSelector(selectors.getMetricsLinkedTimeSelection, {
@@ -368,6 +372,31 @@ describe('histogram card', () => {
368372
});
369373
});
370374

375+
it('removes end step when range selection is disabled', () => {
376+
provideMockCardSeriesData(
377+
selectSpy,
378+
PluginType.HISTOGRAMS,
379+
'card1',
380+
undefined,
381+
[buildHistogramStepData({step: 5}), buildHistogramStepData({step: 15})]
382+
);
383+
store.overrideSelector(selectors.getMetricsLinkedTimeSelection, {
384+
start: {step: 5},
385+
end: {step: 10},
386+
});
387+
store.overrideSelector(selectors.getMetricsRangeSelectionEnabled, false);
388+
const fixture = createHistogramCardContainer();
389+
fixture.detectChanges();
390+
391+
const viz = fixture.debugElement.query(
392+
By.directive(TestableHistogramWidget)
393+
);
394+
expect(viz.componentInstance.timeSelection).toEqual({
395+
start: {step: 5},
396+
end: null,
397+
});
398+
});
399+
371400
describe('time selection beyond range of data', () => {
372401
it('clips the time selection to max step', () => {
373402
provideMockCardSeriesData(

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -221,7 +221,7 @@ export function isDatumVisible(
221221
* @param timeSelection
222222
* @param rangeSelectionEnabled
223223
*/
224-
function maybeOmitTimeSelectionEnd(
224+
export function maybeOmitTimeSelectionEnd(
225225
timeSelection: TimeSelection,
226226
rangeSelectionEnabled: boolean
227227
): TimeSelection {

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import {
2020
getClosestStep,
2121
getDisplayNameForRun,
2222
maybeClipTimeSelectionView,
23+
maybeOmitTimeSelectionEnd,
2324
maybeSetClosestStartStep,
2425
partitionSeries,
2526
} from './utils';
@@ -469,6 +470,38 @@ describe('metrics card_renderer utils test', () => {
469470
});
470471
});
471472

473+
describe('#maybeOmitTimeSelectionEnd', () => {
474+
it('does nothing when range selection is enabled', () => {
475+
expect(
476+
maybeOmitTimeSelectionEnd(
477+
{
478+
start: {step: 5},
479+
end: {step: 10},
480+
},
481+
true
482+
)
483+
).toEqual({
484+
start: {step: 5},
485+
end: {step: 10},
486+
});
487+
});
488+
489+
it('sets end step to null when range selection is disabled', () => {
490+
expect(
491+
maybeOmitTimeSelectionEnd(
492+
{
493+
start: {step: 5},
494+
end: {step: 10},
495+
},
496+
false
497+
)
498+
).toEqual({
499+
start: {step: 5},
500+
end: null,
501+
});
502+
});
503+
});
504+
472505
describe('#formatTimeSelection', () => {
473506
it('returns [maxStep, maxStep] when above minMax', () => {
474507
expect(

0 commit comments

Comments
 (0)