Skip to content

HParams: Update scalar card to show columns based on selector #6405

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 2 commits into from
May 24, 2023
Merged

HParams: Update scalar card to show columns based on selector #6405

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cc30a1b
update scalar card to show columns based on selector
rileyajones May 24, 2023
b50eeef
add comment
rileyajones May 24, 2023
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
1 change: 1 addition & 0 deletions tensorboard/webapp/metrics/store/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ tf_ts_library(
"//tensorboard/webapp/types",
"//tensorboard/webapp/util:dom",
"//tensorboard/webapp/util:lang",
"//tensorboard/webapp/util:memoize",
"//tensorboard/webapp/util:ngrx",
"//tensorboard/webapp/util:types",
"//tensorboard/webapp/widgets/card_fob:types",
Expand Down
42 changes: 28 additions & 14 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 @@ -48,6 +48,7 @@ import {
} from './metrics_types';
import {ColumnHeader, DataTableMode} from '../../widgets/data_table/types';
import {Extent} from '../../widgets/line_chart_v2/lib/public_types';
import {memoize} from '../../util/memoize';

const selectMetricsState =
createFeatureSelector<MetricsState>(METRICS_FEATURE_KEY);
Expand Down Expand Up @@ -405,20 +406,6 @@ export const getMetricsStepMinMax = createSelector(
}
);

export const getSingleSelectionHeaders = createSelector(
selectMetricsState,
(state: MetricsState): ColumnHeader[] => {
return state.singleSelectionHeaders;
}
);

export const getRangeSelectionHeaders = createSelector(
selectMetricsState,
(state: MetricsState): ColumnHeader[] => {
return state.rangeSelectionHeaders;
}
);

/**
* Returns value of the linked time set by user. When linked time selection is never
* set, it returns the default value which is derived from the timeseries data
Expand Down Expand Up @@ -628,3 +615,30 @@ export const getMetricsCardTimeSelection = createSelector(
);
}
);

export const getSingleSelectionHeaders = createSelector(
selectMetricsState,
(state: MetricsState): ColumnHeader[] => {
return state.singleSelectionHeaders;
}
);

export const getRangeSelectionHeaders = createSelector(
selectMetricsState,
(state: MetricsState): ColumnHeader[] => {
return state.rangeSelectionHeaders;
}
);

export const getColumnHeadersForCard = memoize((cardId: string) => {
return createSelector(
(state) => state,
getSingleSelectionHeaders,
getRangeSelectionHeaders,
(state, singleSelectionHeaders, rangeSelectionHeaders) => {
return getMetricsCardRangeSelectionEnabled(state, cardId)
Copy link
Contributor

@JamesHollyer JamesHollyer May 24, 2023

Choose a reason for hiding this comment

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

So this cannot be input to the selector because it is a selector with a prop?

Copy link
Contributor Author

@rileyajones rileyajones May 24, 2023

Choose a reason for hiding this comment

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

Yeah exactly.

? rangeSelectionHeaders
: singleSelectionHeaders;
}
);
});
172 changes: 171 additions & 1 deletion 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 @@ -26,7 +26,11 @@ import {
createTimeSeriesData,
} from '../testing';
import {HistogramMode, TooltipSort, XAxisType} from '../types';
import {DataTableMode} from '../../widgets/data_table/types';
import {
ColumnHeader,
ColumnHeaderType,
DataTableMode,
} from '../../widgets/data_table/types';
import * as selectors from './metrics_selectors';
import {CardFeatureOverride, MetricsState} from './metrics_types';

Expand Down Expand Up @@ -1533,4 +1537,170 @@ describe('metrics selectors', () => {
);
});
});

describe('getSingleSelectionHeaders', () => {
Copy link
Contributor

@JamesHollyer JamesHollyer May 24, 2023

Choose a reason for hiding this comment

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

I think the 'returns all single selection headers' and 'returns all range selection headers' are not needed. It just returns the value and there is no logic to test. That being said. I am ok with these being here.

it('returns all single selection headers', () => {
const state = appStateFromMetricsState(
buildMetricsState({
singleSelectionHeaders: [
{
type: ColumnHeaderType.COLOR,
name: 'color',
displayName: 'Color',
enabled: true,
},
{
type: ColumnHeaderType.RUN,
name: 'run',
displayName: 'My Run name',
enabled: false,
},
],
})
);
expect(selectors.getSingleSelectionHeaders(state)).toEqual([
{
type: ColumnHeaderType.COLOR,
name: 'color',
displayName: 'Color',
enabled: true,
},
{
type: ColumnHeaderType.RUN,
name: 'run',
displayName: 'My Run name',
enabled: false,
},
]);
});
});

describe('getRangeSelectionHeaders', () => {
it('returns all range selection headers', () => {
const state = appStateFromMetricsState(
buildMetricsState({
rangeSelectionHeaders: [
{
type: ColumnHeaderType.COLOR,
name: 'color',
displayName: 'Color',
enabled: true,
},
{
type: ColumnHeaderType.RUN,
name: 'run',
displayName: 'My Run name',
enabled: false,
},
],
})
);
expect(selectors.getRangeSelectionHeaders(state)).toEqual([
{
type: ColumnHeaderType.COLOR,
name: 'color',
displayName: 'Color',
enabled: true,
},
{
type: ColumnHeaderType.RUN,
name: 'run',
displayName: 'My Run name',
enabled: false,
},
]);
});
});

describe('getColumnHeadersForCard', () => {
let singleSelectionHeaders: ColumnHeader[];
let rangeSelectionHeaders: ColumnHeader[];

beforeEach(() => {
singleSelectionHeaders = [
{
type: ColumnHeaderType.COLOR,
name: 'color',
displayName: 'Color',
enabled: true,
},
{
type: ColumnHeaderType.RUN,
name: 'run',
displayName: 'My Run name',
enabled: false,
},
];
rangeSelectionHeaders = [
{
type: ColumnHeaderType.MEAN,
name: 'mean',
displayName: 'Mean',
enabled: true,
},
];
});

it('returns single selection headers when card range selection is disabled', () => {
expect(
selectors.getColumnHeadersForCard('card1')(
appStateFromMetricsState(
buildMetricsState({
singleSelectionHeaders,
rangeSelectionHeaders,
})
)
)
).toEqual(singleSelectionHeaders);
expect(
selectors.getColumnHeadersForCard('card1')(
appStateFromMetricsState(
buildMetricsState({
singleSelectionHeaders,
rangeSelectionHeaders,
cardStateMap: {
card1: {
rangeSelectionOverride:
CardFeatureOverride.OVERRIDE_AS_DISABLED,
},
},
})
)
)
).toEqual(singleSelectionHeaders);
});

it('returns range selection headers when card range selection is enabled', () => {
expect(
selectors.getColumnHeadersForCard('card1')(
appStateFromMetricsState(
buildMetricsState({
singleSelectionHeaders,
rangeSelectionHeaders,
cardStateMap: {
card1: {
rangeSelectionOverride:
CardFeatureOverride.OVERRIDE_AS_ENABLED,
},
},
})
)
)
).toEqual(rangeSelectionHeaders);
});

it('returns range selection headers when global range selection is enabled', () => {
expect(
selectors.getColumnHeadersForCard('card1')(
appStateFromMetricsState(
buildMetricsState({
singleSelectionHeaders,
rangeSelectionHeaders,
rangeSelectionEnabled: true,
})
)
)
).toEqual(rangeSelectionHeaders);
});
});
});
17 changes: 3 additions & 14 deletions tensorboard/webapp/metrics/views/card_renderer/scalar_card_container.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ import {
getRun,
getRunColorMap,
getCurrentRouteRunSelection,
getColumnHeadersForCard,
} from '../../../selectors';
import {DataLoadState} from '../../../types/data';
import {
Expand Down Expand Up @@ -89,8 +90,6 @@ import {
getMetricsScalarSmoothing,
getMetricsTooltipSort,
getMetricsXAxisType,
getRangeSelectionHeaders,
getSingleSelectionHeaders,
RunToSeries,
} from '../../store';
import {CardId, CardMetadata, HeaderEditInfo, XAxisType} from '../../types';
Expand Down Expand Up @@ -461,18 +460,8 @@ export class ScalarCardContainer implements CardRenderer, OnInit, OnDestroy {
this.cardId
);

this.columnHeaders$ = combineLatest([
this.stepOrLinkedTimeSelection$,
this.store.select(getSingleSelectionHeaders),
this.store.select(getRangeSelectionHeaders),
]).pipe(
map(([timeSelection, singleSelectionHeaders, rangeSelectionHeaders]) => {
if (!timeSelection || timeSelection.end === null) {
return singleSelectionHeaders;
} else {
return rangeSelectionHeaders;
}
})
this.columnHeaders$ = this.store.select(
getColumnHeadersForCard(this.cardId)
);

this.chartMetadataMap$ = partitionedSeries$.pipe(
Expand Down
6 changes: 6 additions & 0 deletions 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 @@ -2668,6 +2668,8 @@ describe('scalar card', () => {
describe('getTimeSelectionTableData', () => {
beforeEach(() => {
store.overrideSelector(getMetricsLinkedTimeEnabled, true);
// These tests now rely on the selector getColumnHeadersForCard which in turn
// relies on these selectors.
store.overrideSelector(getSingleSelectionHeaders, [
{
type: ColumnHeaderType.RUN,
Expand Down Expand Up @@ -2880,6 +2882,7 @@ describe('scalar card', () => {
runToSeries
);
store.overrideSelector(getMetricsRangeSelectionEnabled, true);
store.overrideSelector(getMetricsCardRangeSelectionEnabled, true);
Copy link
Contributor

@JamesHollyer JamesHollyer May 24, 2023

Choose a reason for hiding this comment

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

These test now actually use the getColumnHeadersForCard selector and override the selectors which that selector uses. I kind of like this but is not a pattern we use often. Can we at least add a comment making note of this?

store.overrideSelector(
selectors.getCurrentRouteRunSelection,
new Map([
Expand Down Expand Up @@ -2961,6 +2964,7 @@ describe('scalar card', () => {
runToSeries
);
store.overrideSelector(getMetricsRangeSelectionEnabled, true);
store.overrideSelector(getMetricsCardRangeSelectionEnabled, true);
store.overrideSelector(
selectors.getCurrentRouteRunSelection,
new Map([['run1', true]])
Expand Down Expand Up @@ -3026,6 +3030,7 @@ describe('scalar card', () => {
runToSeries
);
store.overrideSelector(getMetricsRangeSelectionEnabled, true);
store.overrideSelector(getMetricsCardRangeSelectionEnabled, true);
store.overrideSelector(
selectors.getCurrentRouteRunSelection,
new Map([['run1', true]])
Expand Down Expand Up @@ -3141,6 +3146,7 @@ describe('scalar card', () => {
runToSeries
);
store.overrideSelector(getMetricsRangeSelectionEnabled, true);
store.overrideSelector(getMetricsCardRangeSelectionEnabled, true);
store.overrideSelector(
selectors.getCurrentRouteRunSelection,
new Map([
Expand Down