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

Add Multiple Y Field Selection to Plot Wizard #4787

Merged
merged 32 commits into from
Oct 11, 2023
Merged

Add Multiple Y Field Selection to Plot Wizard #4787

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
55a3172
first iteration
julieg18 Oct 2, 2023
68953fc
adjust filter
julieg18 Oct 2, 2023
e410b53
Add new tests
julieg18 Oct 2, 2023
1a86d95
Add missing test for dvc.yaml
julieg18 Oct 2, 2023
f81ef2f
resourcePicker adjustments
julieg18 Oct 2, 2023
a6b4a91
Adjust how we handle failed files
julieg18 Oct 3, 2023
3d405fb
Improve error handling
julieg18 Oct 3, 2023
76df14f
Fix broken pickFiles
julieg18 Oct 3, 2023
22c64c5
Use relative path instead of just filename in quick pick
julieg18 Oct 3, 2023
da06e65
Delete unneeded function
julieg18 Oct 3, 2023
b6af87c
Resolve some comments:
julieg18 Oct 4, 2023
bd3e211
Improve text
julieg18 Oct 4, 2023
52defff
wip solution
julieg18 Oct 4, 2023
ec182c3
fix broken tests
julieg18 Oct 5, 2023
54d675e
Merge branch 'main' into improve-plot-wizard-err-handling
julieg18 Oct 5, 2023
528ec62
Clean up and add more tests
julieg18 Oct 5, 2023
0d287a2
Create more solid split between single and multi files
julieg18 Oct 6, 2023
bb688f9
Change function to constant
julieg18 Oct 6, 2023
86556fc
Stop returning array in `validateFileNames`
julieg18 Oct 6, 2023
d850b22
Resolve review comments
julieg18 Oct 9, 2023
be782ee
Merge branch 'main' into improve-plot-wizard-err-handling
julieg18 Oct 9, 2023
cf57f3e
Add Title Option to Plot Wizard
julieg18 Oct 9, 2023
89fc205
Add Multiple Y Field Selection in Plot Wizard
julieg18 Oct 9, 2023
20f6ce7
Only format y values once in `quickPick.ts`
julieg18 Oct 9, 2023
8666917
Merge branch 'main' into add-title-opt-to-plot-wizard
julieg18 Oct 9, 2023
9a185b0
Fix typo
julieg18 Oct 9, 2023
861fe52
Update default title
julieg18 Oct 10, 2023
f067599
Merge branch 'add-title-opt-to-plot-wizard' into allow-plot-wizard-mu…
julieg18 Oct 10, 2023
29657e4
Keep x the same type as y
julieg18 Oct 10, 2023
3d56e51
Merge branch 'main' into allow-plot-wizard-multi-y-selection
julieg18 Oct 10, 2023
71930c0
Clean up
julieg18 Oct 11, 2023
39d255e
Merge branch 'main' into allow-plot-wizard-multi-y-selection
julieg18 Oct 11, 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
20 changes: 10 additions & 10 deletions extension/src/fileSystem/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -625,8 +625,8 @@ describe('addPlotToDvcYamlFile', () => {
addPlotToDvcYamlFile('/', {
template: 'simple',
title: 'Simple Plot',
x: { file: 'data.json', key: 'epochs' },
y: { file: 'data.json', key: 'accuracy' }
x: { 'data.json': 'epochs' },
y: { 'data.json': 'accuracy' }
})

expect(mockedWriteFileSync).toHaveBeenCalledWith(
Expand Down Expand Up @@ -654,8 +654,8 @@ describe('addPlotToDvcYamlFile', () => {
addPlotToDvcYamlFile('/', {
template: 'simple',
title: 'simple_plot',
x: { file: 'data.json', key: 'epochs' },
y: { file: 'acc.json', key: 'accuracy' }
x: { 'data.json': 'epochs' },
y: { 'acc.json': 'accuracy' }
})

expect(mockedWriteFileSync).toHaveBeenCalledWith(
Expand All @@ -673,8 +673,8 @@ describe('addPlotToDvcYamlFile', () => {
addPlotToDvcYamlFile('/', {
template: 'simple',
title: 'Simple Plot',
x: { file: 'data.json', key: 'epochs' },
y: { file: 'data.json', key: 'accuracy' }
x: { 'data.json': 'epochs' },
y: { 'data.json': 'accuracy' }
})

mockDvcYamlContent.splice(7, 0, ...mockPlotYamlContent)
Expand All @@ -696,8 +696,8 @@ describe('addPlotToDvcYamlFile', () => {
addPlotToDvcYamlFile('/', {
template: 'simple',
title: 'Simple Plot',
x: { file: 'data.json', key: 'epochs' },
y: { file: 'data.json', key: 'accuracy' }
x: { 'data.json': 'epochs' },
y: { 'data.json': 'accuracy' }
})

expect(mockedWriteFileSync).toHaveBeenCalledWith(
Expand Down Expand Up @@ -734,8 +734,8 @@ describe('addPlotToDvcYamlFile', () => {
addPlotToDvcYamlFile('/', {
template: 'simple',
title: 'simple_plot',
x: { file: 'data.json', key: 'epochs' },
y: { file: 'data.json', key: 'accuracy' }
x: { 'data.json': 'epochs' },
y: { 'data.json': 'accuracy' }
})

expect(mockedWriteFileSync).toHaveBeenCalledWith(
Expand Down
9 changes: 7 additions & 2 deletions extension/src/fileSystem/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,11 +216,16 @@ const loadYamlAsDoc = (

const getPlotYamlObj = (plot: PlotConfigData) => {
const { x, y, template, title } = plot

const yFiles = Object.keys(y)
const [xFile, xKey] = Object.entries(x)[0]
const oneFileUsed = yFiles.length === 1 && yFiles[0] === xFile

return {
[title]: {
template,
x: x.file === y.file ? x.key : { [x.file]: x.key },
y: { [y.file]: y.key }
x: oneFileUsed ? xKey : x,
y
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In plot examples, you typically use brackets for a file's multiple fields:

  - test_vs_train_loss:
      x: epoch
      y:
        training_data.csv: [test_loss, train_loss]

But our yaml library only turns arrays into - lists:

  - test_vs_train_loss:
      x: epoch
      y:
        training_data.csv: 
          - test_loss
          - train_loss

Copy link
Member

Choose a reason for hiding this comment

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

[Q] Does this format still work? I think this is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

[Q] Does this format still work? I think this is fine.

Yes, this format still works!

}
}
}
Expand Down
144 changes: 117 additions & 27 deletions extension/src/pipeline/quickPick.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@ import { QuickPickItemKind } from 'vscode'
import { pickPlotConfiguration } from './quickPick'
import { getInput } from '../vscode/inputBox'
import { pickFiles } from '../vscode/resourcePicker'
import { quickPickOne, quickPickValue } from '../vscode/quickPick'
import {
quickPickOne,
quickPickValue,
quickPickManyValues
} from '../vscode/quickPick'
import { getFileExtension, loadDataFiles } from '../fileSystem'
import { Title } from '../vscode/title'
import { Toast } from '../vscode/toast'
Expand All @@ -13,6 +17,7 @@ const mockedGetFileExt = jest.mocked(getFileExtension)
const mockedGetInput = jest.mocked(getInput)
const mockedQuickPickOne = jest.mocked(quickPickOne)
const mockedQuickPickValue = jest.mocked(quickPickValue)
const mockedQuickPickValues = jest.mocked(quickPickManyValues)
const mockedToast = jest.mocked(Toast)
const mockedShowError = jest.fn()
mockedToast.showError = mockedShowError
Expand Down Expand Up @@ -259,9 +264,13 @@ describe('pickPlotConfiguration', () => {
{ data: mockValidData, file: '/file.json' }
])
mockedQuickPickOne.mockResolvedValueOnce('simple')
mockedQuickPickValue
.mockResolvedValueOnce({ file: 'file.json', key: 'actual' })
.mockResolvedValueOnce({ file: 'file.json', key: 'prob' })
mockedQuickPickValue.mockResolvedValueOnce({
file: 'file.json',
key: 'actual'
})
mockedQuickPickValues.mockResolvedValueOnce([
{ file: 'file.json', key: 'prob' }
])
mockedGetInput.mockResolvedValueOnce('Simple Plot')

const result = await pickPlotConfiguration('/')
Expand All @@ -281,8 +290,7 @@ describe('pickPlotConfiguration', () => {
],
'Pick a Plot Template'
)
expect(mockedQuickPickValue).toHaveBeenNthCalledWith(
1,
expect(mockedQuickPickValue).toHaveBeenCalledWith(
[
{
kind: QuickPickItemKind.Separator,
Expand All @@ -294,8 +302,7 @@ describe('pickPlotConfiguration', () => {
],
{ title: Title.SELECT_PLOT_X_METRIC }
)
expect(mockedQuickPickValue).toHaveBeenNthCalledWith(
2,
expect(mockedQuickPickValues).toHaveBeenCalledWith(
[
{
kind: QuickPickItemKind.Separator,
Expand All @@ -315,27 +322,33 @@ describe('pickPlotConfiguration', () => {
expect(result).toStrictEqual({
template: 'simple',
title: 'Simple Plot',
x: { file: 'file.json', key: 'actual' },
y: { file: 'file.json', key: 'prob' }
x: { 'file.json': 'actual' },
y: { 'file.json': 'prob' }
})
})

it('should let the user pick a x field and y field from multiple files', async () => {
mockedPickFiles.mockResolvedValueOnce(['/file.json', '/file2.json'])
mockedLoadDataFiles.mockResolvedValueOnce([
{ data: mockValidData, file: '/file.json' },
{ data: mockValidData, file: '/file2.json' }
{
data: mockValidData,
file: '/file2.json'
}
])
mockedQuickPickOne.mockResolvedValueOnce('simple')
mockedGetInput.mockResolvedValueOnce('simple_plot')
mockedQuickPickValue
.mockResolvedValueOnce({ file: 'file.json', key: 'actual' })
.mockResolvedValueOnce({ file: 'file2.json', key: 'prob' })
mockedQuickPickValue.mockResolvedValueOnce({
file: 'file.json',
key: 'actual'
})
mockedQuickPickValues.mockResolvedValueOnce([
{ file: 'file2.json', key: 'prob' }
])

const result = await pickPlotConfiguration('/')

expect(mockedQuickPickValue).toHaveBeenNthCalledWith(
1,
expect(mockedQuickPickValue).toHaveBeenCalledWith(
[
{
kind: QuickPickItemKind.Separator,
Expand All @@ -354,8 +367,7 @@ describe('pickPlotConfiguration', () => {
],
{ title: Title.SELECT_PLOT_X_METRIC }
)
expect(mockedQuickPickValue).toHaveBeenNthCalledWith(
2,
expect(mockedQuickPickValues).toHaveBeenCalledWith(
[
{
kind: QuickPickItemKind.Separator,
Expand All @@ -378,8 +390,80 @@ describe('pickPlotConfiguration', () => {
expect(result).toStrictEqual({
template: 'simple',
title: 'simple_plot',
x: { file: 'file.json', key: 'actual' },
y: { file: 'file2.json', key: 'prob' }
x: { 'file.json': 'actual' },
y: { 'file2.json': 'prob' }
})
})

it('should let the user pick multiple y fields', async () => {
mockedPickFiles.mockResolvedValueOnce(['/file.json', '/file2.json'])
mockedLoadDataFiles.mockResolvedValueOnce([
{ data: mockValidData, file: '/file.json' },
{
data: mockValidData.map((value, ind) => ({ ...value, step: ind })),
file: '/file2.json'
}
])
mockedQuickPickOne.mockResolvedValueOnce('simple')
mockedGetInput.mockResolvedValueOnce('simple_plot')
mockedQuickPickValue.mockResolvedValueOnce({
file: 'file.json',
key: 'actual'
})
mockedQuickPickValues.mockResolvedValueOnce([
{ file: 'file2.json', key: 'prob' },
{ file: 'file2.json', key: 'actual' },
{ file: 'file2.json', key: 'step' }
])

const result = await pickPlotConfiguration('/')

expect(mockedQuickPickValue).toHaveBeenCalledWith(
[
{
kind: QuickPickItemKind.Separator,
label: 'file.json',
value: undefined
},
{ label: 'actual', value: { file: 'file.json', key: 'actual' } },
{ label: 'prob', value: { file: 'file.json', key: 'prob' } },
{
kind: QuickPickItemKind.Separator,
label: 'file2.json',
value: undefined
},
{ label: 'actual', value: { file: 'file2.json', key: 'actual' } },
{ label: 'prob', value: { file: 'file2.json', key: 'prob' } },
{ label: 'step', value: { file: 'file2.json', key: 'step' } }
],
{ title: Title.SELECT_PLOT_X_METRIC }
)
expect(mockedQuickPickValues).toHaveBeenCalledWith(
[
{
kind: QuickPickItemKind.Separator,
label: 'file.json',
value: undefined
},
{ label: 'prob', value: { file: 'file.json', key: 'prob' } },
{
kind: QuickPickItemKind.Separator,
label: 'file2.json',
value: undefined
},
{ label: 'actual', value: { file: 'file2.json', key: 'actual' } },
{ label: 'prob', value: { file: 'file2.json', key: 'prob' } },
{ label: 'step', value: { file: 'file2.json', key: 'step' } }
],
{
title: Title.SELECT_PLOT_Y_METRIC
}
)
expect(result).toStrictEqual({
template: 'simple',
title: 'simple_plot',
x: { 'file.json': 'actual' },
y: { 'file2.json': ['prob', 'actual', 'step'] }
})
})

Expand Down Expand Up @@ -419,13 +503,12 @@ describe('pickPlotConfiguration', () => {
])
mockedQuickPickOne.mockResolvedValueOnce('simple')
mockedGetInput.mockResolvedValueOnce('simple_plot')
mockedQuickPickValue
.mockResolvedValueOnce('actual')
.mockResolvedValueOnce(undefined)
mockedQuickPickValue.mockResolvedValueOnce('actual')
mockedQuickPickValues.mockResolvedValueOnce(undefined)

const result = await pickPlotConfiguration('/')

expect(mockedQuickPickValue).toHaveBeenCalledTimes(2)
expect(mockedQuickPickValues).toHaveBeenCalledTimes(1)
expect(result).toStrictEqual(undefined)
})

Expand All @@ -435,9 +518,16 @@ describe('pickPlotConfiguration', () => {
{ data: mockValidData, file: 'file.json' }
])
mockedQuickPickOne.mockResolvedValueOnce('linear')
mockedQuickPickValue
.mockResolvedValueOnce({ file: 'file.json', key: 'actual' })
.mockResolvedValueOnce({ file: 'file.json', key: 'prob' })
mockedQuickPickValue.mockResolvedValueOnce({
file: 'file.json',
key: 'actual'
})
mockedQuickPickValues.mockResolvedValueOnce([
{
file: 'file.json',
key: 'prob'
}
])
mockedGetInput.mockResolvedValueOnce(undefined)

const result = await pickPlotConfiguration('/')
Expand Down
Loading