Adds common selectors for shared hparams

[hoonji]

Motivation for features / changes

Displaying shared hparam columns in the runs and scalar tables requires new selectors.

Technical description of changes

Adds two new common selectors that will be used by both runs and scalar tables:

• getSelectableColumns
• getGroupedColumns

Also updates getDashboardDisplayedHparamColumns to only return relevant hparams (i.e. those with specs defined by selected experiments)

Detailed steps to verify changes work correctly (as executed by you)

Unit tests pass

[rileyajones]

This is my opinion, feel free to push back.

We don't use the pattern of passing a selector to a selector anywhere else in the TB codebase. Due to this I expect this to confuse future devs coming to the project.
I would rather see this selectors projector moved to a utility function and shared by a series of selectors (not selector factories) created to select each type.

The result might look something like this

function groupHeaders(tableHeaders: ColumnHeader[], hparamHeaders: []) {
   // The projector function from the getGroupedColumns selector
}

export const selectGroupedRunTableHeaders = createSelector(
  getRunsTableHeaders,
  getDashboardDisplayedHparamColumns,
  groupHeaders
);

export const selectGroupedDataTableHeaders = createSelector(
  getDataTableHeaders,
  getDashboardDisplayedHparamColumns,
  groupHeaders
);

[hoonji]

I would rather see this selectors projector moved to a utility function and shared by a series of selectors

This is indeed a much simpler abstraction than passing around selectors as function arguments. I created a groupColumns helper similar to what you described. I put it in /widgets/data_table/utils.ts because it doesn't really belong in the /metrics directory.

[hoonji]

I also moved the runs and metrics selectors to their respective directories, and added a simple selector test for each.

[rileyajones]

I'm not sure this approach of running a series of filters, then dumping everything which doesn't match at the end will scale if you end up with a handful of additional confitions.

Suggested change

	return [
	...tableHeaders.filter((header) => header.type === 'RUN'),
	...tableHeaders.filter(
	(header) =>
	header.type === 'CUSTOM' && header.name === 'experimentAlias'
	),
	...hparamHeaders,
	...tableHeaders.filter(
	(header) =>
	header.type !== 'RUN' &&
	!(header.type === 'CUSTOM' && header.name === 'experimentAlias')
	),
	];
	const headersGroups: Record<string, ColumnHeader[]> = {
	RUN: [],
	EXPERIMENT_ALIAS: [],
	OTHER: [],
	};
	tableHeaders.forEach((header) => {
	if (header.type === 'RUN') {
	headersGroups['RUN'].push(header);
	return;
	}
	if (header.type === 'CUSTOM' && header.name === 'experimentAlias') {
	headerGroups['EXPERIMENT_ALIAS'].push(header);
	return;
	}
	headersGroups['OTHER'].push(header);
	})
	return [
	...headerGroups['RUN'],
	...headerGroups['EXPERIMENT_ALIAS'],
	...hparamHeaders,
	...headerGroups['OTHER'],

[hoonji]

I was also wondering about a more robust way to do this and you gave me an idea. I implemented something along these lines in /widgets/data_table/utils.ts:groupColumns, PTAL!

[hoonji]

Removed dependency on store to prevent a circular dependency. All required symbols can be imported from /metrics_types instead.

[rileyajones]

Thank you for setting up the proper mapped type, I was hoping you would do that when I left my previous comment about this structure.

If you use a record here (instead of a map) you won't need that null check

Suggested change

	const headerGroups = new Map<ColumnGroup, ColumnHeader[]>([
	['RUN', []],
	['EXPERIMENT_ALIAS', []],
	['HPARAM', []],
	['OTHER', []],
	]);
	columns.forEach((column) => {
	headerGroups.get(columnToGroup(column))?.push(column);
	});
	return Array.from(headerGroups.values()).flat();
	const headerGroups: Record<ColumnGroup, ColumnHeader[]> = {
	RUN: [],
	EXPERIMENT_ALIAS: [],
	HPARAM: [],
	OTHER: [],
	};
	columns.forEach((column) => {
	headerGroups[columnToGroup(column)].push(column);
	});
	return Object.values(headerGroups).flat();

[hoonji]

In this case I think we need Map here to guarantee key order [1] - I added a comment to clarify this

[1] (see "Key Order") https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map#objects_vs._maps

[rileyajones]

nit: I think this should be an enum as it is a bit more consistent with the rest of our code (though I personally do like string literals)

Suggested change

	export type ColumnGroup = 'RUN' | 'EXPERIMENT_ALIAS' | 'HPARAM' | 'OTHER';
	export enum ColumnGroup {
	RUN,
	EXPERIMENT_ALIAS,
	HPARAM,
	OTHER
	}

[hoonji]

Done! I'll prefer enums going forward.