Skip to content
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

Jump to bottom

Rename firstThreeColumns #4159

Merged
merged 2 commits into from
Jun 23, 2023
Merged

Rename firstThreeColumns #4159

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
48f18f7
Rename "firstThreeColumns"
julieg18 Jun 21, 2023
1e0034a
Merge branch 'use-metrics-and-params-in-table-tooltips' into rename-f…
julieg18 Jun 23, 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
8 changes: 4 additions & 4 deletions extension/src/experiments/columns/model.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,7 +282,7 @@ describe('ColumnsModel', () => {
)
await model.transformAndSet(outputFixture)

expect(model.getFirstThreeColumnOrder()).toStrictEqual([
expect(model.getSummaryColumnOrder()).toStrictEqual([
Copy link
Contributor Author

@julieg18 julieg18 Jun 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Figured that summaryColumns is simpler than firstThreeMetricAndParamColumns. Plus, if we adjust what these summary columns include we won't need to nename everything again. Though there might be a better term than summary.

'params:params.yaml:dvc_logs_dir',
'params:params.yaml:process.threshold',
'params:params.yaml:process.test_arg',
Expand All @@ -292,7 +292,7 @@ describe('ColumnsModel', () => {

model.toggleStatus('params:params.yaml:dvc_logs_dir')

expect(model.getFirstThreeColumnOrder()).toStrictEqual([
expect(model.getSummaryColumnOrder()).toStrictEqual([
'params:params.yaml:process.threshold',
'params:params.yaml:process.test_arg',
'params:params.yaml:dropout',
Expand All @@ -309,7 +309,7 @@ describe('ColumnsModel', () => {
)
await model.transformAndSet(outputFixture)

expect(model.getFirstThreeColumnOrder()).toStrictEqual([
expect(model.getSummaryColumnOrder()).toStrictEqual([
'params:params.yaml:code_names',
'params:params.yaml:epochs',
'params:params.yaml:learning_rate',
Expand All @@ -320,7 +320,7 @@ describe('ColumnsModel', () => {

model.toggleStatus('params:params.yaml:code_names')

expect(model.getFirstThreeColumnOrder()).toStrictEqual([
expect(model.getSummaryColumnOrder()).toStrictEqual([
'params:params.yaml:epochs',
'params:params.yaml:learning_rate',
'params:params.yaml:dvc_logs_dir',
Expand Down
3 changes: 1 addition & 2 deletions extension/src/experiments/columns/model.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,8 @@ export class ColumnsModel extends PathSelectionModel<Column> {
return this.columnOrderState
}

public getFirstThreeColumnOrder(): string[] {
public getSummaryColumnOrder(): string[] {
const acc: SummaryAcc = { metrics: [], params: [] }

for (const path of this.columnOrderState) {
const reachedMaxSummaryOrderLength =
acc.metrics.length >= 3 && acc.params.length >= 3
Expand Down
16 changes: 8 additions & 8 deletions extension/src/experiments/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,7 +340,7 @@ export class Experiments extends BaseRepository<TableData> {

const selected = await pickExperimentsToPlot(
experiments,
this.columns.getFirstThreeColumnOrder()
this.columns.getSummaryColumnOrder()
)
if (!selected) {
return
Expand All @@ -365,30 +365,30 @@ export class Experiments extends BaseRepository<TableData> {
public pickCommitOrExperiment() {
return pickExperiment(
this.experiments.getCommitsAndExperiments(),
this.getFirstThreeColumnOrder()
this.getSummaryColumnOrder()
)
}

public pickRunningExperiments() {
return pickExperiments(
this.experiments.getRunningExperiments(),
this.getFirstThreeColumnOrder(),
this.getSummaryColumnOrder(),
Title.SELECT_EXPERIMENTS_STOP
)
}

public pickExperimentsToRemove() {
return pickExperiments(
this.experiments.getExperimentsAndQueued(),
this.getFirstThreeColumnOrder(),
this.getSummaryColumnOrder(),
Title.SELECT_EXPERIMENTS_REMOVE
)
}

public pickExperimentsToPush() {
return pickExperiments(
this.experiments.getExperiments(),
this.getFirstThreeColumnOrder(),
this.getSummaryColumnOrder(),
Title.SELECT_EXPERIMENTS_PUSH
)
}
Expand Down Expand Up @@ -502,8 +502,8 @@ export class Experiments extends BaseRepository<TableData> {
return this.experiments.getCliError()
}

public getFirstThreeColumnOrder() {
return this.columns.getFirstThreeColumnOrder()
public getSummaryColumnOrder() {
return this.columns.getSummaryColumnOrder()
}

public getColumnTerminalNodes() {
Expand Down Expand Up @@ -584,7 +584,7 @@ export class Experiments extends BaseRepository<TableData> {

return await pickExperiment(
this.experiments.getCombinedList(),
this.getFirstThreeColumnOrder(),
this.getSummaryColumnOrder(),
Title.SELECT_BASE_EXPERIMENT
)
}
Expand Down
39 changes: 18 additions & 21 deletions extension/src/experiments/model/quickPick.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,18 +20,18 @@ type QuickPickItemAccumulator = {

const getItem = (
experiment: Experiment,
firstThreeColumnOrder: string[]
summaryColumnOrder: string[]
): QuickPickItemWithValue<Experiment | undefined> => ({
detail: getColumnPathsQuickPickDetail(experiment, firstThreeColumnOrder),
detail: getColumnPathsQuickPickDetail(experiment, summaryColumnOrder),
label: experiment.label,
value: experiment
})

const getItemWithDescription = (
experiment: Experiment,
firstThreeColumnOrder: string[]
summaryColumnOrder: string[]
) => {
const item = getItem(experiment, firstThreeColumnOrder)
const item = getItem(experiment, summaryColumnOrder)
if (experiment.description) {
item.description = `${experiment.commit ? '$(git-commit)' : ''}${
experiment.description
Expand All @@ -43,10 +43,10 @@ const getItemWithDescription = (
const collectItem = (
acc: QuickPickItemAccumulator,
experiment: Experiment,
firstThreeColumnOrder: string[],
summaryColumnOrder: string[],
transformer = getItem
) => {
const item = transformer(experiment, firstThreeColumnOrder)
const item = transformer(experiment, summaryColumnOrder)
acc.items.push(item)
if (experiment.selected) {
acc.selectedItems.push(item)
Expand All @@ -56,32 +56,29 @@ const collectItem = (

const collectItems = (
experiments: Experiment[],
firstThreeColumnOrder: string[]
summaryColumnOrder: string[]
) => {
const acc: QuickPickItemAccumulator = {
items: [],
selectedItems: []
}

for (const experiment of experiments) {
collectItem(acc, experiment, firstThreeColumnOrder, getItemWithDescription)
collectItem(acc, experiment, summaryColumnOrder, getItemWithDescription)
}

return acc
}

export const pickExperimentsToPlot = (
experiments: Experiment[],
firstThreeColumnOrder: string[]
summaryColumnOrder: string[]
): Promise<Experiment[] | undefined> => {
if (!definedAndNonEmpty(experiments)) {
return Toast.showError(noExperimentsToSelect)
}

const { items, selectedItems } = collectItems(
experiments,
firstThreeColumnOrder
)
const { items, selectedItems } = collectItems(experiments, summaryColumnOrder)

return quickPickLimitedValues<Experiment | undefined>(
items,
Expand All @@ -101,14 +98,14 @@ type ExperimentItem = {

const getExperimentItems = (
experiments: Experiment[],
firstThreeColumnOrder: string[]
summaryColumnOrder: string[]
): ExperimentItem[] =>
experiments.map(experiment => {
const { label, id, description, commit } = experiment
return {
description:
description && `${commit ? '$(git-commit)' : ''}${description}`,
detail: getColumnPathsQuickPickDetail(experiment, firstThreeColumnOrder),
detail: getColumnPathsQuickPickDetail(experiment, summaryColumnOrder),
label,
value: id
}
Expand All @@ -121,15 +118,15 @@ const pickExperimentOrExperiments = <
T extends QuickPickExperiment | QuickPickExperiments
>(
experiments: Experiment[],
firstThreeColumnOrder: string[],
summaryColumnOrder: string[],
title: Title,
quickPick: T
): ReturnType<T> | Promise<undefined> => {
if (!definedAndNonEmpty(experiments)) {
return Toast.showError(noExperimentsToSelect)
}

const items = getExperimentItems(experiments, firstThreeColumnOrder)
const items = getExperimentItems(experiments, summaryColumnOrder)

return quickPick(items, {
matchOnDescription: true,
Expand All @@ -140,24 +137,24 @@ const pickExperimentOrExperiments = <

export const pickExperiment = (
experiments: Experiment[],
firstThreeColumnOrder: string[],
summaryColumnOrder: string[],
title: Title = Title.SELECT_EXPERIMENT
): Thenable<string | undefined> =>
pickExperimentOrExperiments<QuickPickExperiment>(
experiments,
firstThreeColumnOrder,
summaryColumnOrder,
title,
quickPickValue
)

export const pickExperiments = (
experiments: Experiment[],
firstThreeColumnOrder: string[],
summaryColumnOrder: string[],
title: Title = Title.SELECT_EXPERIMENTS
): Thenable<string[] | undefined> =>
pickExperimentOrExperiments<QuickPickExperiments>(
experiments,
firstThreeColumnOrder,
summaryColumnOrder,
title,
quickPickManyValues
)
10 changes: 5 additions & 5 deletions extension/src/experiments/model/tree.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ const {
mockedGetCommitExperiments,
mockedGetCliError,
mockedGetDvcRoots,
mockedGetFirstThreeColumnOrder,
mockedGetSummaryColumnOrder,
mockedGetWorkspaceAndCommits
} = buildMockedExperiments()

Expand Down Expand Up @@ -179,7 +179,7 @@ describe('ExperimentsTree', () => {
.mockReturnValueOnce(experiments)

mockedGetDvcRoots.mockReturnValueOnce(['repo'])
mockedGetFirstThreeColumnOrder.mockReturnValue([])
mockedGetSummaryColumnOrder.mockReturnValue([])

const children = await experimentsTree.getChildren()

Expand Down Expand Up @@ -294,7 +294,7 @@ describe('ExperimentsTree', () => {
}
]
mockedGetCommitExperiments.mockReturnValueOnce(experimentsByCommit)
mockedGetFirstThreeColumnOrder.mockReturnValue([])
mockedGetSummaryColumnOrder.mockReturnValue([])

const children = await experimentsTree.getChildren(commit)

Expand Down Expand Up @@ -391,7 +391,7 @@ describe('ExperimentsTree', () => {
.mockReturnValueOnce(experiments)
.mockReturnValueOnce(experiments)
mockedGetDvcRoots.mockReturnValueOnce(['repo'])
mockedGetFirstThreeColumnOrder.mockReturnValue([
mockedGetSummaryColumnOrder.mockReturnValue([
'params:params.yaml:epochs',
'params:params.yaml:featurize.random_value'
])
Expand Down Expand Up @@ -469,7 +469,7 @@ describe('ExperimentsTree', () => {
.mockReturnValueOnce(experiments)
.mockReturnValueOnce(experiments)
mockedGetDvcRoots.mockReturnValueOnce(['repo'])
mockedGetFirstThreeColumnOrder.mockReturnValue([])
mockedGetSummaryColumnOrder.mockReturnValue([])

const experimentsTree = new ExperimentsTree(
mockedExperiments,
Expand Down
12 changes: 6 additions & 6 deletions extension/src/experiments/model/tree.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ export class ExperimentsTree
tooltip: this.getTooltip(
experiment.error,
experiment,
this.experiments.getRepository(dvcRoot).getFirstThreeColumnOrder()
this.experiments.getRepository(dvcRoot).getSummaryColumnOrder()
),
type: experiment.type
}
Expand Down Expand Up @@ -343,8 +343,8 @@ export class ExperimentsTree
return [...this.view.selection]
}

private getTooltipTable(experiment: Experiment, firstThreeColumns: string[]) {
const data = getDataFromColumnPaths(experiment, firstThreeColumns)
private getTooltipTable(experiment: Experiment, summaryColumns: string[]) {
const data = getDataFromColumnPaths(experiment, summaryColumns)
.map(
({ truncatedValue: value, columnPath }) =>
`| ${truncateFromLeft(columnPath, 30)} | ${value} |\n`
Expand All @@ -356,14 +356,14 @@ export class ExperimentsTree
private getTooltip(
error: string | undefined,
experiment: Experiment,
firstThreeColumns: string[]
summaryColumns: string[]
) {
if (!error) {
if (firstThreeColumns.length === 0) {
if (summaryColumns.length === 0) {
return
}

return this.getTooltipTable(experiment, firstThreeColumns)
return this.getTooltipTable(experiment, summaryColumns)
}

return getErrorTooltip(error)
Expand Down
8 changes: 4 additions & 4 deletions extension/src/plots/model/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,15 @@ describe('plotsModel', () => {
DEFAULT_SECTION_NB_ITEMS_PER_ROW_OR_WIDTH
})
const mockedGetSelectedRevisions = jest.fn()
const mockedGetFirstThreeColumnOrder = jest.fn()
mockedGetFirstThreeColumnOrder.mockReturnValue([])
const mockedGetSummaryColumnOrder = jest.fn()
mockedGetSummaryColumnOrder.mockReturnValue([])

beforeEach(() => {
model = new PlotsModel(
exampleDvcRoot,
{
getFirstThreeColumnOrder: mockedGetFirstThreeColumnOrder,
getSelectedRevisions: mockedGetSelectedRevisions,
getSummaryColumnOrder: mockedGetSummaryColumnOrder,
isReady: () => Promise.resolve(undefined)
} as unknown as Experiments,
{
Expand All @@ -68,8 +68,8 @@ describe('plotsModel', () => {
model = new PlotsModel(
exampleDvcRoot,
{
getFirstThreeColumnOrder: mockedGetFirstThreeColumnOrder,
getSelectedRevisions: mockedGetSelectedRevisions,
getSummaryColumnOrder: mockedGetSummaryColumnOrder,
isReady: () => Promise.resolve(undefined)
} as unknown as Experiments,
{ getImageErrors: () => undefined } as unknown as ErrorsModel,
Expand Down
12 changes: 6 additions & 6 deletions extension/src/plots/model/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import {
collectImageUrl,
collectIdShas
} from './collect'
import { getRevisionFirstThreeColumns } from './util'
import { getRevisionSummaryColumns } from './util'
import {
checkForCustomPlotOptions,
cleanupOldOrderValue,
Expand Down Expand Up @@ -206,12 +206,12 @@ export class PlotsModel extends ModelWithPersistence {
displayColor,
errors: this.errors.getRevisionErrors(id),
fetched: this.fetchedRevs.has(id),
firstThreeColumns: getRevisionFirstThreeColumns(
this.experiments.getFirstThreeColumnOrder(),
experiment
),
id,
label
label,
summaryColumns: getRevisionSummaryColumns(
this.experiments.getSummaryColumnOrder(),
experiment
)
}

if (commit) {
Expand Down
Loading