From f6f6c82f49419c674159ff89d8f7c68907e557ee Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Fri, 19 May 2023 20:40:58 +0000 Subject: [PATCH 1/2] only filter scalar card runs by hparams when enableHparamsInTimeSeries is true --- .../card_renderer/scalar_card_container.ts | 5 +- .../views/card_renderer/scalar_card_test.ts | 97 +++++++++++++++++++ 2 files changed, 101 insertions(+), 1 deletion(-) diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts index 5cbf455e94..5fd6a57969 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts @@ -38,6 +38,7 @@ import { import {State} from '../../../app_state'; import {ExperimentAlias} from '../../../experiments/types'; import { + getEnableHparamsInTimeSeries, getForceSvgFeatureFlag, getIsScalarColumnCustomizationEnabled, } from '../../../feature_flag/store/feature_flag_selectors'; @@ -498,6 +499,7 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { }), combineLatestWith( this.store.select(getCurrentRouteRunSelection), + this.store.select(getEnableHparamsInTimeSeries), this.store.select(getFilteredRenderableRunsIdsFromRoute), this.store.select(getRunColorMap), this.store.select(getMetricsScalarSmoothing) @@ -511,6 +513,7 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { ([ namedPartitionedSeries, runSelectionMap, + hparamsInTimeSeriesEnabled, renderableRuns, colorMap, smoothing, @@ -539,7 +542,7 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { visible: Boolean( runSelectionMap && runSelectionMap.get(runId) && - renderableRuns.has(runId) + (renderableRuns.has(runId) || !hparamsInTimeSeriesEnabled) ), color: colorMap[runId] ?? '#fff', aux: false, diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts index dd5b3689e3..24f351d5f4 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts @@ -3631,6 +3631,103 @@ describe('scalar card', () => { expect(data[5].run).toEqual('3 c/run6'); expect(data[6].run).toEqual('1 a/run7'); })); + + it('filters runs by hparam when enableHparamsInTimeSeries is true', fakeAsync(() => { + const runToSeries = { + run1: [{wallTime: 1, value: 1, step: 1}], + run2: [{wallTime: 1, value: 2, step: 1}], + run3: [{wallTime: 1, value: 3, step: 1}], + run4: [{wallTime: 1, value: NaN, step: 1}], + run5: [{wallTime: 1, value: 'NaN', step: 1}], + run6: [{wallTime: 1, value: null, step: 1}], + run7: [{wallTime: 1, value: undefined, step: 1}], + }; + provideMockCardRunToSeriesData( + selectSpy, + PluginType.SCALARS, + 'card1', + null /* metadataOverride */, + runToSeries as any + ); + store.overrideSelector( + selectors.getCurrentRouteRunSelection, + new Map([ + ['run1', true], + ['run2', true], + ]) + ); + + store.overrideSelector( + commonSelectors.getFilteredRenderableRunsIdsFromRoute, + new Set(['run1']) + ); + + store.overrideSelector(getMetricsLinkedTimeSelection, { + start: {step: 1}, + end: null, + }); + + store.overrideSelector(selectors.getEnableHparamsInTimeSeries, true); + + const fixture = createComponent('card1'); + const scalarCardDataTable = fixture.debugElement.query( + By.directive(ScalarCardDataTable) + ); + + const data = + scalarCardDataTable.componentInstance.getTimeSelectionTableData(); + expect(data.length).toEqual(1); + expect(data[0].run).toEqual('run1'); + })); + + it('does not filter runs by hparam when enableHparamsInTimeSeries is false', fakeAsync(() => { + const runToSeries = { + run1: [{wallTime: 1, value: 1, step: 1}], + run2: [{wallTime: 1, value: 2, step: 1}], + run3: [{wallTime: 1, value: 3, step: 1}], + run4: [{wallTime: 1, value: NaN, step: 1}], + run5: [{wallTime: 1, value: 'NaN', step: 1}], + run6: [{wallTime: 1, value: null, step: 1}], + run7: [{wallTime: 1, value: undefined, step: 1}], + }; + provideMockCardRunToSeriesData( + selectSpy, + PluginType.SCALARS, + 'card1', + null /* metadataOverride */, + runToSeries as any + ); + store.overrideSelector( + selectors.getCurrentRouteRunSelection, + new Map([ + ['run1', true], + ['run2', true], + ]) + ); + + store.overrideSelector( + commonSelectors.getFilteredRenderableRunsIdsFromRoute, + new Set(['run1']) + ); + + store.overrideSelector(getMetricsLinkedTimeSelection, { + start: {step: 1}, + end: null, + }); + + store.overrideSelector(selectors.getEnableHparamsInTimeSeries, false); + + const fixture = createComponent('card1'); + const scalarCardDataTable = fixture.debugElement.query( + By.directive(ScalarCardDataTable) + ); + + const data = + scalarCardDataTable.componentInstance.getTimeSelectionTableData(); + expect(data.length).toEqual(2); + expect(data[0].run).toEqual('run1'); + expect(data[1].run).toEqual('run2'); + })); }); describe('toggleTableExpanded', () => { From 12e07051bf398bb1bdf583783daca5d2e954d040 Mon Sep 17 00:00:00 2001 From: Riley Jones Date: Mon, 22 May 2023 15:46:17 +0000 Subject: [PATCH 2/2] refactor to address pr comments --- .../card_renderer/scalar_card_container.ts | 2 +- .../views/card_renderer/scalar_card_test.ts | 144 +++++++----------- 2 files changed, 58 insertions(+), 88 deletions(-) diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts index 5fd6a57969..77a33002bf 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts @@ -542,7 +542,7 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy { visible: Boolean( runSelectionMap && runSelectionMap.get(runId) && - (renderableRuns.has(runId) || !hparamsInTimeSeriesEnabled) + (!hparamsInTimeSeriesEnabled || renderableRuns.has(runId)) ), color: colorMap[runId] ?? '#fff', aux: false, diff --git a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts index 24f351d5f4..e6c264f21d 100644 --- a/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts +++ b/tensorboard/webapp/metrics/views/card_renderer/scalar_card_test.ts @@ -3632,102 +3632,72 @@ describe('scalar card', () => { expect(data[6].run).toEqual('1 a/run7'); })); - it('filters runs by hparam when enableHparamsInTimeSeries is true', fakeAsync(() => { - const runToSeries = { - run1: [{wallTime: 1, value: 1, step: 1}], - run2: [{wallTime: 1, value: 2, step: 1}], - run3: [{wallTime: 1, value: 3, step: 1}], - run4: [{wallTime: 1, value: NaN, step: 1}], - run5: [{wallTime: 1, value: 'NaN', step: 1}], - run6: [{wallTime: 1, value: null, step: 1}], - run7: [{wallTime: 1, value: undefined, step: 1}], - }; - provideMockCardRunToSeriesData( - selectSpy, - PluginType.SCALARS, - 'card1', - null /* metadataOverride */, - runToSeries as any - ); - store.overrideSelector( - selectors.getCurrentRouteRunSelection, - new Map([ - ['run1', true], - ['run2', true], - ]) - ); + describe('while integrated with hparams in timeseries', () => { + beforeEach(() => { + const runToSeries = { + run1: [{wallTime: 1, value: 1, step: 1}], + run2: [{wallTime: 1, value: 2, step: 1}], + run3: [{wallTime: 1, value: 3, step: 1}], + run4: [{wallTime: 1, value: NaN, step: 1}], + run5: [{wallTime: 1, value: 'NaN', step: 1}], + run6: [{wallTime: 1, value: null, step: 1}], + run7: [{wallTime: 1, value: undefined, step: 1}], + }; + provideMockCardRunToSeriesData( + selectSpy, + PluginType.SCALARS, + 'card1', + null /* metadataOverride */, + runToSeries as any + ); + store.overrideSelector( + selectors.getCurrentRouteRunSelection, + new Map([ + ['run1', true], + ['run2', true], + ]) + ); - store.overrideSelector( - commonSelectors.getFilteredRenderableRunsIdsFromRoute, - new Set(['run1']) - ); + store.overrideSelector( + commonSelectors.getFilteredRenderableRunsIdsFromRoute, + new Set(['run1']) + ); - store.overrideSelector(getMetricsLinkedTimeSelection, { - start: {step: 1}, - end: null, + store.overrideSelector(getMetricsLinkedTimeSelection, { + start: {step: 1}, + end: null, + }); }); - store.overrideSelector(selectors.getEnableHparamsInTimeSeries, true); - - const fixture = createComponent('card1'); - const scalarCardDataTable = fixture.debugElement.query( - By.directive(ScalarCardDataTable) - ); - - const data = - scalarCardDataTable.componentInstance.getTimeSelectionTableData(); - expect(data.length).toEqual(1); - expect(data[0].run).toEqual('run1'); - })); - - it('does not filter runs by hparam when enableHparamsInTimeSeries is false', fakeAsync(() => { - const runToSeries = { - run1: [{wallTime: 1, value: 1, step: 1}], - run2: [{wallTime: 1, value: 2, step: 1}], - run3: [{wallTime: 1, value: 3, step: 1}], - run4: [{wallTime: 1, value: NaN, step: 1}], - run5: [{wallTime: 1, value: 'NaN', step: 1}], - run6: [{wallTime: 1, value: null, step: 1}], - run7: [{wallTime: 1, value: undefined, step: 1}], - }; - provideMockCardRunToSeriesData( - selectSpy, - PluginType.SCALARS, - 'card1', - null /* metadataOverride */, - runToSeries as any - ); - store.overrideSelector( - selectors.getCurrentRouteRunSelection, - new Map([ - ['run1', true], - ['run2', true], - ]) - ); + it('filters runs by hparam when enableHparamsInTimeSeries is true', fakeAsync(() => { + store.overrideSelector(selectors.getEnableHparamsInTimeSeries, true); - store.overrideSelector( - commonSelectors.getFilteredRenderableRunsIdsFromRoute, - new Set(['run1']) - ); + const fixture = createComponent('card1'); + const scalarCardDataTable = fixture.debugElement.query( + By.directive(ScalarCardDataTable) + ); - store.overrideSelector(getMetricsLinkedTimeSelection, { - start: {step: 1}, - end: null, - }); + const data = + scalarCardDataTable.componentInstance.getTimeSelectionTableData(); + expect(data.length).toEqual(1); + expect(data[0].run).toEqual('run1'); + })); - store.overrideSelector(selectors.getEnableHparamsInTimeSeries, false); + it('does not filter runs by hparam when enableHparamsInTimeSeries is false', fakeAsync(() => { + store.overrideSelector(selectors.getEnableHparamsInTimeSeries, false); - const fixture = createComponent('card1'); - const scalarCardDataTable = fixture.debugElement.query( - By.directive(ScalarCardDataTable) - ); + const fixture = createComponent('card1'); + const scalarCardDataTable = fixture.debugElement.query( + By.directive(ScalarCardDataTable) + ); - const data = - scalarCardDataTable.componentInstance.getTimeSelectionTableData(); - expect(data.length).toEqual(2); - expect(data[0].run).toEqual('run1'); - expect(data[1].run).toEqual('run2'); - })); + const data = + scalarCardDataTable.componentInstance.getTimeSelectionTableData(); + expect(data.length).toEqual(2); + expect(data[0].run).toEqual('run1'); + expect(data[1].run).toEqual('run2'); + })); + }); }); describe('toggleTableExpanded', () => {