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

Clean up column path logic in custom state #3460

Merged
merged 2 commits into from
Mar 14, 2023
Merged

Clean up column path logic in custom state #3460

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
beeb5c3
check that weird error shows remotely
julieg18 Mar 14, 2023
d8c50bd
Fix error
julieg18 Mar 14, 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
18 changes: 12 additions & 6 deletions extension/src/plots/model/collect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ import { extractColumns } from '../../experiments/columns/extract'
import {
decodeColumn,
appendColumnToPath,
splitColumnPath
splitColumnPath,
FILE_SEPARATOR
} from '../../experiments/columns/paths'
import {
ColumnType,
Expand Down Expand Up @@ -270,18 +271,23 @@ const collectMetricVsParamPlot = (
param: string,
experiments: Experiment[]
): MetricVsParamPlot => {
const splitUpMetricPath = splitColumnPath(metric)
const splitUpParamPath = splitColumnPath(param)
const splitUpMetricPath = splitColumnPath(
ColumnType.METRICS + FILE_SEPARATOR + metric
)
const splitUpParamPath = splitColumnPath(
ColumnType.PARAMS + FILE_SEPARATOR + param
)
const plotData: MetricVsParamPlot = {
id: getCustomPlotId(metric, param),
metric: metric.slice(ColumnType.METRICS.length + 1),
param: param.slice(ColumnType.PARAMS.length + 1),
metric,
param,
type: CustomPlotType.METRIC_VS_PARAM,
values: []
}

for (const experiment of experiments) {
const metricValue = get(experiment, splitUpMetricPath) as number | undefined

const paramValue = get(experiment, splitUpParamPath) as number | undefined

if (metricValue !== undefined && paramValue !== undefined) {
Expand All @@ -305,7 +311,7 @@ export const collectCustomPlots = (
.map((plotOrderValue): CustomPlot => {
if (isCheckpointValue(plotOrderValue.type)) {
const { metric } = plotOrderValue
return checkpointPlots[metric.slice(ColumnType.METRICS.length + 1)]
return checkpointPlots[metric]
}
const { metric, param } = plotOrderValue
return collectMetricVsParamPlot(metric, param, experiments)
Expand Down
21 changes: 21 additions & 0 deletions extension/src/plots/model/custom.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { ColumnType } from '../../experiments/webview/contract'
import { CheckpointPlot, CustomPlot, CustomPlotType } from '../webview/contract'

export const CHECKPOINTS_PARAM = 'epoch'
Expand All @@ -23,3 +24,23 @@ export const doesCustomPlotAlreadyExist = (
order.some(value => {
return value.param === param && value.metric === metric
})

const removeColumnTypeFromPath = (
columnPath: string,
type: string,
fileSep: string
) =>
columnPath.startsWith(type + fileSep)
? columnPath.slice(type.length + 1)
: columnPath

export const cleanupOldOrderValue = (
{ param, metric, type }: CustomPlotsOrderValue,
fileSep: string
): CustomPlotsOrderValue => ({
// previous column paths have the "TYPE:" prefix
metric: removeColumnTypeFromPath(metric, ColumnType.METRICS, fileSep),
param: removeColumnTypeFromPath(param, ColumnType.PARAMS, fileSep),
// previous values didn't have a type
type: type || CustomPlotType.METRIC_VS_PARAM
})
19 changes: 9 additions & 10 deletions extension/src/plots/model/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,11 @@ import {
collectCustomPlotData
} from './collect'
import { getRevisionFirstThreeColumns } from './util'
import { CustomPlotsOrderValue, isCheckpointPlot } from './custom'
import {
cleanupOldOrderValue,
CustomPlotsOrderValue,
isCheckpointPlot
} from './custom'
import {
CheckpointPlot,
ComparisonPlots,
Expand All @@ -31,8 +35,7 @@ import {
CustomPlot,
ColorScale,
DEFAULT_HEIGHT,
DEFAULT_NB_ITEMS_PER_ROW,
CustomPlotType
DEFAULT_NB_ITEMS_PER_ROW
} from '../webview/contract'
import {
ExperimentsOutput,
Expand All @@ -53,6 +56,7 @@ import {
MultiSourceVariations
} from '../multiSource/collect'
import { isDvcError } from '../../cli/dvc/reader'
import { FILE_SEPARATOR } from '../../experiments/columns/paths'

export type CustomCheckpointPlots = { [metric: string]: CheckpointPlot }

Expand Down Expand Up @@ -186,13 +190,8 @@ export class PlotsModel extends ModelWithPersistence {
}

public getCustomPlotsOrder() {
return this.customPlotsOrder.map(
({ type, ...rest }) =>
({
...rest,
// type is possibly undefined if state holds an older version of custom plots
type: type || CustomPlotType.METRIC_VS_PARAM
} as CustomPlotsOrderValue)
return this.customPlotsOrder.map(value =>
cleanupOldOrderValue(value, FILE_SEPARATOR)
)
}

Expand Down
15 changes: 4 additions & 11 deletions extension/src/plots/webview/messages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,6 @@ import {
pickMetricAndParam
} from '../model/quickPick'
import { Title } from '../../vscode/title'
import { ColumnType } from '../../experiments/webview/contract'
import { FILE_SEPARATOR } from '../../experiments/columns/paths'
import { reorderObjectList } from '../../util/array'
import {
CHECKPOINTS_PARAM,
Expand Down Expand Up @@ -271,19 +269,14 @@ export class WebviewMessages {
return
}

const buildMetricOrParamPath = (type: string, path: string) =>
type + FILE_SEPARATOR + path

const newOrder: CustomPlotsOrderValue[] = reorderObjectList(
plotIds,
customPlots,
'id'
).map(plot => ({
metric: buildMetricOrParamPath(ColumnType.METRICS, plot.metric),
param: isCheckpointValue(plot.type)
? plot.param
: buildMetricOrParamPath(ColumnType.PARAMS, plot.param),
type: plot.type
).map(({ metric, param, type }) => ({
metric,
param,
type
}))
this.plots.setCustomPlotsOrder(newOrder)
this.sendCustomPlotsAndEvent(EventName.VIEWS_REORDER_PLOTS_CUSTOM)
Expand Down
16 changes: 8 additions & 8 deletions extension/src/test/fixtures/expShow/base/customPlots.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,22 @@ import {

export const customPlotsOrderFixture: CustomPlotsOrderValue[] = [
{
metric: 'metrics:summary.json:loss',
param: 'params:params.yaml:dropout',
metric: 'summary.json:loss',
param: 'params.yaml:dropout',
type: CustomPlotType.METRIC_VS_PARAM
},
{
metric: 'metrics:summary.json:accuracy',
param: 'params:params.yaml:epochs',
metric: 'summary.json:accuracy',
param: 'params.yaml:epochs',
type: CustomPlotType.METRIC_VS_PARAM
},
{
metric: 'metrics:summary.json:loss',
metric: 'summary.json:loss',
param: CHECKPOINTS_PARAM,
type: CustomPlotType.CHECKPOINT
},
{
metric: 'metrics:summary.json:accuracy',
metric: 'summary.json:accuracy',
param: CHECKPOINTS_PARAM,
type: CustomPlotType.CHECKPOINT
}
Expand Down Expand Up @@ -125,7 +125,7 @@ const data: CustomPlotsData = {
},
plots: [
{
id: 'custom-metrics:summary.json:loss-params:params.yaml:dropout',
id: 'custom-summary.json:loss-params.yaml:dropout',
metric: 'summary.json:loss',
param: 'params.yaml:dropout',
type: CustomPlotType.METRIC_VS_PARAM,
Expand All @@ -149,7 +149,7 @@ const data: CustomPlotsData = {
yTitle: 'summary.json:loss'
},
{
id: 'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
id: 'custom-summary.json:accuracy-params.yaml:epochs',
metric: 'summary.json:accuracy',
param: 'params.yaml:epochs',
type: CustomPlotType.METRIC_VS_PARAM,
Expand Down
24 changes: 12 additions & 12 deletions extension/src/test/suite/plots/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -431,8 +431,8 @@ suite('Plots Test Suite', () => {
const webview = await plots.showWebview()

const mockNewCustomPlotsOrder = [
'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
'custom-metrics:summary.json:loss-params:params.yaml:dropout'
'custom-summary.json:accuracy-params.yaml:epochs',
'custom-summary.json:loss-params.yaml:dropout'
]

stub(plotsModel, 'getCustomPlots')
Expand All @@ -454,13 +454,13 @@ suite('Plots Test Suite', () => {
expect(mockSetCustomPlotsOrder).to.be.calledOnce
expect(mockSetCustomPlotsOrder).to.be.calledWithExactly([
{
metric: 'metrics:summary.json:accuracy',
param: 'params:params.yaml:epochs',
metric: 'summary.json:accuracy',
param: 'params.yaml:epochs',
type: CustomPlotType.METRIC_VS_PARAM
},
{
metric: 'metrics:summary.json:loss',
param: 'params:params.yaml:dropout',
metric: 'summary.json:loss',
param: 'params.yaml:dropout',
type: CustomPlotType.METRIC_VS_PARAM
}
])
Expand Down Expand Up @@ -781,8 +781,8 @@ suite('Plots Test Suite', () => {
const mockGetMetric = stub(customPlotQuickPickUtil, 'pickMetric')

const mockMetricVsParamOrderValue = {
metric: 'metrics:summary.json:accuracy',
param: 'params:params.yaml:dropout',
metric: 'summary.json:accuracy',
param: 'params.yaml:dropout',
type: CustomPlotType.METRIC_VS_PARAM
}

Expand Down Expand Up @@ -825,7 +825,7 @@ suite('Plots Test Suite', () => {
)

const mockCheckpointsOrderValue = {
metric: 'metrics:summary.json:val_loss',
metric: 'summary.json:val_loss',
param: CHECKPOINTS_PARAM,
type: CustomPlotType.CHECKPOINT
}
Expand Down Expand Up @@ -877,15 +877,15 @@ suite('Plots Test Suite', () => {
mockSelectCustomPlots.callsFake(() => {
resolve(undefined)
return Promise.resolve([
'custom-metrics:summary.json:loss-params:params.yaml:dropout'
'custom-summary.json:loss-params.yaml:dropout'
])
})
)

stub(plotsModel, 'getCustomPlotsOrder').returns([
{
metric: 'metrics:summary.json:loss',
param: 'params:params.yaml:dropout',
metric: 'summary.json:loss',
param: 'params.yaml:dropout',
type: CustomPlotType.METRIC_VS_PARAM
}
])
Expand Down
34 changes: 17 additions & 17 deletions webview/src/plots/components/App.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -645,8 +645,8 @@ describe('App', () => {
let plots = screen.getAllByTestId(/summary\.json/)

expect(plots.map(plot => plot.id)).toStrictEqual([
'custom-metrics:summary.json:loss-params:params.yaml:dropout',
'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
'custom-summary.json:loss-params.yaml:dropout',
'custom-summary.json:accuracy-params.yaml:epochs',
'custom-summary.json:loss-epoch',
'custom-summary.json:accuracy-epoch'
])
Expand All @@ -656,8 +656,8 @@ describe('App', () => {
plots = screen.getAllByTestId(/summary\.json/)

expect(plots.map(plot => plot.id)).toStrictEqual([
'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
'custom-metrics:summary.json:loss-params:params.yaml:dropout',
'custom-summary.json:accuracy-params.yaml:epochs',
'custom-summary.json:loss-params.yaml:dropout',
'custom-summary.json:loss-epoch',
'custom-summary.json:accuracy-epoch'
])
Expand All @@ -671,8 +671,8 @@ describe('App', () => {

const plots = screen.getAllByTestId(/summary\.json/)
expect(plots.map(plot => plot.id)).toStrictEqual([
'custom-metrics:summary.json:loss-params:params.yaml:dropout',
'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
'custom-summary.json:loss-params.yaml:dropout',
'custom-summary.json:accuracy-params.yaml:epochs',
'custom-summary.json:loss-epoch',
'custom-summary.json:accuracy-epoch'
])
Expand All @@ -683,8 +683,8 @@ describe('App', () => {

const expectedOrder = [
'custom-summary.json:loss-epoch',
'custom-metrics:summary.json:loss-params:params.yaml:dropout',
'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
'custom-summary.json:loss-params.yaml:dropout',
'custom-summary.json:accuracy-params.yaml:epochs',
'custom-summary.json:accuracy-epoch'
]

Expand All @@ -710,8 +710,8 @@ describe('App', () => {
expect(
screen.getAllByTestId(/summary\.json/).map(plot => plot.id)
).toStrictEqual([
'custom-metrics:summary.json:loss-params:params.yaml:dropout',
'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
'custom-summary.json:loss-params.yaml:dropout',
'custom-summary.json:accuracy-params.yaml:epochs',
'custom-summary.json:loss-epoch'
])

Expand All @@ -722,8 +722,8 @@ describe('App', () => {
expect(
screen.getAllByTestId(/summary\.json/).map(plot => plot.id)
).toStrictEqual([
'custom-metrics:summary.json:loss-params:params.yaml:dropout',
'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
'custom-summary.json:loss-params.yaml:dropout',
'custom-summary.json:accuracy-params.yaml:epochs',
'custom-summary.json:loss-epoch',
'custom-summary.json:accuracy-epoch'
])
Expand All @@ -738,8 +738,8 @@ describe('App', () => {
expect(
screen.getAllByTestId(/summary\.json/).map(plot => plot.id)
).toStrictEqual([
'custom-metrics:summary.json:loss-params:params.yaml:dropout',
'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
'custom-summary.json:loss-params.yaml:dropout',
'custom-summary.json:accuracy-params.yaml:epochs',
'custom-summary.json:loss-epoch',
'custom-summary.json:accuracy-epoch'
])
Expand All @@ -754,7 +754,7 @@ describe('App', () => {
expect(
screen.getAllByTestId(/summary\.json/).map(plot => plot.id)
).toStrictEqual([
'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
'custom-summary.json:accuracy-params.yaml:epochs',
'custom-summary.json:loss-epoch',
'custom-summary.json:accuracy-epoch'
])
Expand Down Expand Up @@ -997,10 +997,10 @@ describe('App', () => {
dragAndDrop(plots[0], screen.getByTestId('custom-plots'))

const expectedOrder = [
'custom-metrics:summary.json:accuracy-params:params.yaml:epochs',
'custom-summary.json:accuracy-params.yaml:epochs',
'custom-summary.json:loss-epoch',
'custom-summary.json:accuracy-epoch',
'custom-metrics:summary.json:loss-params:params.yaml:dropout'
'custom-summary.json:loss-params.yaml:dropout'
]

expect(
Expand Down