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.

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

Move trends plots inside of "Custom" section #3404

Merged
merged 54 commits into from
Mar 20, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
54 commits
Select commit Hold shift + click to select a range
60ae393
Move trends inside of custom
julieg18 Mar 6, 2023
6dc8813
fix typo
julieg18 Mar 6, 2023
88fe26d
fix linting
julieg18 Mar 6, 2023
c325008
Fix jest tests
julieg18 Mar 7, 2023
6216af0
Fix vscode tests
julieg18 Mar 8, 2023
e505442
Fix reordering bug
julieg18 Mar 8, 2023
8ab22c4
Resolve comments
julieg18 Mar 9, 2023
64befd1
Merge branch 'main' into move-trends-inside-custom
julieg18 Mar 9, 2023
3354b3e
Fix e2e tests
julieg18 Mar 9, 2023
bd433cb
Add accidentally deleted test
julieg18 Mar 9, 2023
b4d64b3
Rename trends plot
julieg18 Mar 10, 2023
7052633
Refactor
julieg18 Mar 10, 2023
3bd3bc1
Delete unused metrics code
julieg18 Mar 10, 2023
1a6e423
Add extra quick pick tests
julieg18 Mar 10, 2023
359a20c
Update readme
julieg18 Mar 10, 2023
c9ea4fe
Add test for creating both kinds of custom plots
julieg18 Mar 11, 2023
e08e30e
Fix possibly broken old state
julieg18 Mar 11, 2023
cd626b9
Merge branch 'main' into move-trends-inside-custom
julieg18 Mar 11, 2023
e3eba7b
Improve text
julieg18 Mar 11, 2023
74bc253
Refactor
julieg18 Mar 12, 2023
5bc8625
Merge branch 'main' into move-trends-inside-custom
julieg18 Mar 13, 2023
e62452d
Delete unneeded code
julieg18 Mar 13, 2023
d75af27
Update readme
julieg18 Mar 13, 2023
64530ab
Revert "Delete unneeded code"
julieg18 Mar 13, 2023
8d6b312
Delete unneeded code
julieg18 Mar 13, 2023
141c1ea
Consolidate CustomPlotOrderValue and CustomPlot
julieg18 Mar 13, 2023
da9791c
Delete unneeded code
julieg18 Mar 13, 2023
ace4b17
Resolve most review comments
julieg18 Mar 13, 2023
0c09b21
Merge branch 'main' into move-trends-inside-custom
julieg18 Mar 13, 2023
bf5bea8
Use only one loop with `getCustomPlotsData`
julieg18 Mar 13, 2023
7b0790a
Fix typo
julieg18 Mar 13, 2023
5bfa20f
Delete duplication code
julieg18 Mar 13, 2023
244076a
Rename argument
julieg18 Mar 14, 2023
e461677
Clean up column path logic in custom state (#3460)
julieg18 Mar 14, 2023
d5d9a9e
Merge branch 'main' into move-trends-inside-custom
julieg18 Mar 14, 2023
38ab2d6
Fix quick pick
julieg18 Mar 14, 2023
a804a0c
Fix readme typo
julieg18 Mar 14, 2023
de9a78c
Merge branch 'main' into move-trends-inside-custom
julieg18 Mar 14, 2023
3f30959
Fix checkpoint spec
julieg18 Mar 14, 2023
71ed367
Merge branch 'main' into move-trends-inside-custom
julieg18 Mar 17, 2023
7ebebdf
Replace checkpoint with custom in App.test.tsx
julieg18 Mar 17, 2023
9630fdd
Merge branch 'main' into move-trends-inside-custom
julieg18 Mar 17, 2023
40d0010
Fix "plot already exists" logic
julieg18 Mar 20, 2023
9ae29fd
Merge branch 'main' into move-trends-inside-custom
julieg18 Mar 20, 2023
1ab409c
Consolidate `collectCustomPlots` (#3466)
julieg18 Mar 20, 2023
3c92cc4
Merge branch 'main' into move-trends-inside-custom
julieg18 Mar 20, 2023
a2b9c75
Add tests for custom.ts
julieg18 Mar 20, 2023
418ee53
Fix typo in tests
julieg18 Mar 20, 2023
bd305a1
Add tests for user ending early on +/- custom plots
julieg18 Mar 20, 2023
897d60c
Merge branch 'main' into move-trends-inside-custom
julieg18 Mar 20, 2023
4032f1c
Add testing for `name || label` code
julieg18 Mar 20, 2023
ea8e4b3
Merge branch 'main' into move-trends-inside-custom
julieg18 Mar 20, 2023
c3e4d6f
Merge branch 'main' into move-trends-inside-custom
julieg18 Mar 20, 2023
2945e34
Merge branch 'main' into move-trends-inside-custom
julieg18 Mar 20, 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
278 changes: 78 additions & 200 deletions extension/src/plots/model/collect.test.ts
Original file line number Diff line number Diff line change
@@ -1,26 +1,27 @@
import { join } from 'path'
import omit from 'lodash.omit'
import isEmpty from 'lodash.isempty'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test coverage on the diff in this pull request is 91.2% (85% is the threshold).

I looked over the code and everything looked pretty covered. Any thing I could have missed to improve the coverage?

import {
collectData,
collectCheckpointPlotsData,
collectTemplates,
collectMetricOrder,
collectOverrideRevisionDetails,
collectCustomPlotsData
} from './collect'
import plotsDiffFixture from '../../test/fixtures/plotsDiff/output'
import expShowFixture from '../../test/fixtures/expShow/base/output'
import modifiedFixture from '../../test/fixtures/expShow/modified/output'
import checkpointPlotsFixture from '../../test/fixtures/expShow/base/checkpointPlots'
import customPlotsFixture from '../../test/fixtures/expShow/base/customPlots'
import customPlotsFixture, {
customPlotsOrderFixture
} from '../../test/fixtures/expShow/base/customPlots'
import {
ExperimentsOutput,
ExperimentStatus,
EXPERIMENT_WORKSPACE_ID
} from '../../cli/dvc/contract'
import { definedAndNonEmpty, sameContents } from '../../util/array'
import { TemplatePlot } from '../webview/contract'
import { sameContents } from '../../util/array'
import {
CustomPlot,
CustomPlotData,
CustomPlotType,
MetricVsParamPlotData,
TemplatePlot
} from '../webview/contract'
import { getCLICommitId } from '../../test/fixtures/plotsDiff/util'
import { SelectedExperimentWithColor } from '../../experiments/model'
import { Experiment } from '../../experiments/webview/contract'
Expand All @@ -30,229 +31,106 @@ const logsLossPath = join('logs', 'loss.tsv')
const logsLossPlot = (plotsDiffFixture[logsLossPath][0] || {}) as TemplatePlot

describe('collectCustomPlotsData', () => {
it('should return the expected data from the text fixture', () => {
it('should return the expected data from the test fixture', () => {
const expectedOutput: CustomPlot[] = customPlotsFixture.plots.map(
({ type, metric, id, values, ...plot }: CustomPlotData) =>
type === CustomPlotType.CHECKPOINT
julieg18 marked this conversation as resolved.
Show resolved Hide resolved
? {
id,
metric,
type,
values
}
: {
id,
metric,
param: (plot as MetricVsParamPlotData).param,
type,
values
}
)
const data = collectCustomPlotsData(
[
{
metric: 'metrics:summary.json:loss',
param: 'params:params.yaml:dropout'
customPlotsOrderFixture,
{
'summary.json:accuracy': {
id: 'custom-summary.json:accuracy',
metric: 'summary.json:accuracy',
type: CustomPlotType.CHECKPOINT,
values: [
{ group: 'exp-83425', iteration: 1, y: 0.40904998779296875 },
{ group: 'exp-83425', iteration: 2, y: 0.46094998717308044 },
{ group: 'exp-83425', iteration: 3, y: 0.5113166570663452 },
{ group: 'exp-83425', iteration: 4, y: 0.557449996471405 },
{ group: 'exp-83425', iteration: 5, y: 0.5926499962806702 },
{ group: 'exp-83425', iteration: 6, y: 0.5926499962806702 },
{ group: 'test-branch', iteration: 1, y: 0.4083833396434784 },
{ group: 'test-branch', iteration: 2, y: 0.4668000042438507 },
{ group: 'test-branch', iteration: 3, y: 0.4668000042438507 },
{ group: 'exp-e7a67', iteration: 1, y: 0.3723166584968567 },
{ group: 'exp-e7a67', iteration: 2, y: 0.3724166750907898 },
{ group: 'exp-e7a67', iteration: 3, y: 0.3724166750907898 }
]
},
{
metric: 'metrics:summary.json:accuracy',
param: 'params:params.yaml:epochs'
'summary.json:loss': {
id: 'custom-summary.json:loss',
metric: 'summary.json:loss',
type: CustomPlotType.CHECKPOINT,
values: [
{ group: 'exp-83425', iteration: 1, y: 1.9896177053451538 },
{ group: 'exp-83425', iteration: 2, y: 1.9329891204833984 },
{ group: 'exp-83425', iteration: 3, y: 1.8798457384109497 },
{ group: 'exp-83425', iteration: 4, y: 1.8261293172836304 },
{ group: 'exp-83425', iteration: 5, y: 1.775016188621521 },
{ group: 'exp-83425', iteration: 6, y: 1.775016188621521 },
{ group: 'test-branch', iteration: 1, y: 1.9882521629333496 },
{ group: 'test-branch', iteration: 2, y: 1.9293040037155151 },
{ group: 'test-branch', iteration: 3, y: 1.9293040037155151 },
{ group: 'exp-e7a67', iteration: 1, y: 2.020392894744873 },
{ group: 'exp-e7a67', iteration: 2, y: 2.0205044746398926 },
{ group: 'exp-e7a67', iteration: 3, y: 2.0205044746398926 }
]
}
],
},
[
{
id: '12345',
label: '123',
metrics: {
'summary.json': {
accuracy: 0.4668000042438507,
accuracy: 0.3724166750907898,
loss: 2.0205044746398926
}
},
name: 'exp-e7a67',
params: { 'params.yaml': { dropout: 0.15, epochs: 16 } }
params: { 'params.yaml': { dropout: 0.15, epochs: 2 } }
},
{
id: '12345',
label: '123',
metrics: {
'summary.json': {
accuracy: 0.3484833240509033,
accuracy: 0.4668000042438507,
loss: 1.9293040037155151
}
},
name: 'exp-83425',
params: { 'params.yaml': { dropout: 0.25, epochs: 10 } }
name: 'test-branch',
params: { 'params.yaml': { dropout: 0.122, epochs: 2 } }
},
{
id: '12345',
label: '123',
metrics: {
'summary.json': {
accuracy: 0.6768440509033,
loss: 2.298503875732422
accuracy: 0.5926499962806702,
loss: 1.775016188621521
}
},
name: 'exp-f13bca',
params: { 'params.yaml': { dropout: 0.32, epochs: 20 } }
name: 'exp-83425',
params: { 'params.yaml': { dropout: 0.124, epochs: 5 } }
}
]
)
expect(data).toStrictEqual(customPlotsFixture.plots)
})
})

describe('collectCheckpointPlotsData', () => {
it('should return the expected data from the test fixture', () => {
const data = collectCheckpointPlotsData(expShowFixture)
expect(data).toStrictEqual(
checkpointPlotsFixture.plots.map(({ id, values }) => ({ id, values }))
)
})

it('should provide a continuous series for a modified experiment', () => {
const data = collectCheckpointPlotsData(modifiedFixture)

expect(definedAndNonEmpty(data)).toBeTruthy()

for (const { values } of data || []) {
const initialExperiment = values.filter(
point => point.group === 'exp-908bd'
)
const modifiedExperiment = values.find(
point => point.group === 'exp-01b3a'
)

const lastIterationInitial = initialExperiment?.slice(-1)[0]
const firstIterationModified = modifiedExperiment

expect(lastIterationInitial).not.toStrictEqual(firstIterationModified)
expect(omit(lastIterationInitial, 'group')).toStrictEqual(
omit(firstIterationModified, 'group')
)

const baseExperiment = values.filter(point => point.group === 'exp-920fc')
const restartedExperiment = values.find(
point => point.group === 'exp-9bc1b'
)

const iterationRestartedFrom = baseExperiment?.slice(5)[0]
const firstIterationAfterRestart = restartedExperiment

expect(iterationRestartedFrom).not.toStrictEqual(
firstIterationAfterRestart
)
expect(omit(iterationRestartedFrom, 'group')).toStrictEqual(
omit(firstIterationAfterRestart, 'group')
)
}
})

it('should return undefined given no input', () => {
const data = collectCheckpointPlotsData({} as ExperimentsOutput)
expect(data).toBeUndefined()
})
})

describe('collectMetricOrder', () => {
it('should return an empty array if there is no checkpoints data', () => {
const metricOrder = collectMetricOrder(
undefined,
['metric:A', 'metric:B'],
[]
)
expect(metricOrder).toStrictEqual([])
})

it('should return an empty array if the checkpoints data is an empty array', () => {
const metricOrder = collectMetricOrder([], ['metric:A', 'metric:B'], [])
expect(metricOrder).toStrictEqual([])
})

it('should maintain the existing order if all metrics are selected', () => {
const expectedOrder = [
'metric:F',
'metric:A',
'metric:B',
'metric:E',
'metric:D',
'metric:C'
]

const metricOrder = collectMetricOrder(
[
{ id: 'metric:A', values: [] },
{ id: 'metric:B', values: [] },
{ id: 'metric:C', values: [] },
{ id: 'metric:D', values: [] },
{ id: 'metric:E', values: [] },
{ id: 'metric:F', values: [] }
],
expectedOrder,
expectedOrder
)
expect(metricOrder).toStrictEqual(expectedOrder)
})

it('should push unselected metrics to the end', () => {
const existingOrder = [
'metric:F',
'metric:A',
'metric:B',
'metric:E',
'metric:D',
'metric:C'
]

const metricOrder = collectMetricOrder(
[
{ id: 'metric:A', values: [] },
{ id: 'metric:B', values: [] },
{ id: 'metric:C', values: [] },
{ id: 'metric:D', values: [] },
{ id: 'metric:E', values: [] },
{ id: 'metric:F', values: [] }
],
existingOrder,
existingOrder.filter(metric => !['metric:A', 'metric:B'].includes(metric))
)
expect(metricOrder).toStrictEqual([
'metric:F',
'metric:E',
'metric:D',
'metric:C',
'metric:A',
'metric:B'
])
})

it('should add new metrics in the given order', () => {
const metricOrder = collectMetricOrder(
[
{ id: 'metric:C', values: [] },
{ id: 'metric:D', values: [] },
{ id: 'metric:A', values: [] },
{ id: 'metric:B', values: [] },
{ id: 'metric:E', values: [] },
{ id: 'metric:F', values: [] }
],
['metric:B', 'metric:A'],
['metric:B', 'metric:A']
)
expect(metricOrder).toStrictEqual([
'metric:B',
'metric:A',
'metric:C',
'metric:D',
'metric:E',
'metric:F'
])
})

it('should give selected metrics precedence', () => {
const metricOrder = collectMetricOrder(
[
{ id: 'metric:C', values: [] },
{ id: 'metric:D', values: [] },
{ id: 'metric:A', values: [] },
{ id: 'metric:B', values: [] },
{ id: 'metric:E', values: [] },
{ id: 'metric:F', values: [] }
],
['metric:B', 'metric:A'],
['metric:B', 'metric:A', 'metric:F']
)
expect(metricOrder).toStrictEqual([
'metric:B',
'metric:A',
'metric:F',
'metric:C',
'metric:D',
'metric:E'
])
expect(data).toStrictEqual(expectedOutput)
})
})

Expand Down
Loading