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 X Field Selection to Plot Wizard #4797

Merged
merged 42 commits into from
Oct 13, 2023
Merged

Add Multiple X Field Selection to Plot Wizard #4797

Show file tree
Hide file tree
Changes from 38 commits
Commits
Show all changes
42 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
3b5bc20
first iteration
julieg18 Oct 10, 2023
5933faa
Move error handling to chain pr
julieg18 Oct 10, 2023
135582a
update quick pick util
julieg18 Oct 10, 2023
6dbd3dd
Merge branch 'main' into allow-plot-wizard-multi-x-selection
julieg18 Oct 11, 2023
973e684
Improve types
julieg18 Oct 11, 2023
749c2f9
Add tests for new quick pick util
julieg18 Oct 11, 2023
880a6c0
Add check for empty x fields
julieg18 Oct 11, 2023
c6349bb
Minor refactor
julieg18 Oct 11, 2023
e5be873
Merge branch 'main' into allow-plot-wizard-multi-x-selection
julieg18 Oct 11, 2023
1d892f8
Resolve review comments
julieg18 Oct 12, 2023
9741260
Fix typo
julieg18 Oct 12, 2023
7033ec6
Add error handling for Plot Wizard plots with x fields (#4798)
julieg18 Oct 13, 2023
402d6a2
Merge branch 'main' into allow-plot-wizard-multi-x-selection
julieg18 Oct 13, 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: 5 additions & 3 deletions extension/src/fileSystem/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,13 +218,15 @@ 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
const xFiles = Object.keys(x)
const firstXFile = xFiles[0]
const oneFileUsed =
yFiles.length === 1 && xFiles.length === 1 && yFiles[0] === firstXFile

return {
[title]: {
template,
x: oneFileUsed ? xKey : x,
x: oneFileUsed ? x[firstXFile] : x,
y
}
}
Expand Down
205 changes: 145 additions & 60 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,11 +2,7 @@ import { QuickPickItemKind } from 'vscode'
import { pickPlotConfiguration } from './quickPick'
import { getInput } from '../vscode/inputBox'
import { pickFiles } from '../vscode/resourcePicker'
import {
quickPickOne,
quickPickValue,
quickPickManyValues
} from '../vscode/quickPick'
import { quickPickOne, quickPickUserOrderedValues } from '../vscode/quickPick'
import { getFileExtension, loadDataFiles } from '../fileSystem'
import { Title } from '../vscode/title'
import { Toast } from '../vscode/toast'
Expand All @@ -16,8 +12,7 @@ const mockedLoadDataFiles = jest.mocked(loadDataFiles)
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 mockedQuickPickUserOrderedValues = jest.mocked(quickPickUserOrderedValues)
const mockedToast = jest.mocked(Toast)
const mockedShowError = jest.fn()
mockedToast.showError = mockedShowError
Expand Down Expand Up @@ -264,13 +259,14 @@ describe('pickPlotConfiguration', () => {
{ data: mockValidData, file: '/file.json' }
])
mockedQuickPickOne.mockResolvedValueOnce('simple')
mockedQuickPickValue.mockResolvedValueOnce({
file: 'file.json',
key: 'actual'
})
mockedQuickPickValues.mockResolvedValueOnce([
{ file: 'file.json', key: 'prob' }
])
mockedQuickPickUserOrderedValues
.mockResolvedValueOnce([
{
file: 'file.json',
key: 'actual'
}
])
.mockResolvedValueOnce([{ file: 'file.json', key: 'prob' }])
mockedGetInput.mockResolvedValueOnce('Simple Plot')

const result = await pickPlotConfiguration('/')
Expand All @@ -290,7 +286,8 @@ describe('pickPlotConfiguration', () => {
],
'Pick a Plot Template'
)
expect(mockedQuickPickValue).toHaveBeenCalledWith(
expect(mockedQuickPickUserOrderedValues).toHaveBeenNthCalledWith(
1,
[
{
kind: QuickPickItemKind.Separator,
Expand All @@ -302,7 +299,8 @@ describe('pickPlotConfiguration', () => {
],
{ title: Title.SELECT_PLOT_X_METRIC }
)
expect(mockedQuickPickValues).toHaveBeenCalledWith(
expect(mockedQuickPickUserOrderedValues).toHaveBeenNthCalledWith(
2,
[
{
kind: QuickPickItemKind.Separator,
Expand Down Expand Up @@ -338,17 +336,14 @@ describe('pickPlotConfiguration', () => {
])
mockedQuickPickOne.mockResolvedValueOnce('simple')
mockedGetInput.mockResolvedValueOnce('simple_plot')
mockedQuickPickValue.mockResolvedValueOnce({
file: 'file.json',
key: 'actual'
})
mockedQuickPickValues.mockResolvedValueOnce([
{ file: 'file2.json', key: 'prob' }
])
mockedQuickPickUserOrderedValues
.mockResolvedValueOnce([{ file: 'file.json', key: 'actual' }])
.mockResolvedValueOnce([{ file: 'file2.json', key: 'prob' }])

const result = await pickPlotConfiguration('/')

expect(mockedQuickPickValue).toHaveBeenCalledWith(
expect(mockedQuickPickUserOrderedValues).toHaveBeenNthCalledWith(
1,
[
{
kind: QuickPickItemKind.Separator,
Expand All @@ -367,7 +362,8 @@ describe('pickPlotConfiguration', () => {
],
{ title: Title.SELECT_PLOT_X_METRIC }
)
expect(mockedQuickPickValues).toHaveBeenCalledWith(
expect(mockedQuickPickUserOrderedValues).toHaveBeenNthCalledWith(
2,
[
{
kind: QuickPickItemKind.Separator,
Expand Down Expand Up @@ -406,19 +402,23 @@ describe('pickPlotConfiguration', () => {
])
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' }
])
mockedQuickPickUserOrderedValues
.mockResolvedValueOnce([
{
file: 'file.json',
key: 'actual'
}
])
.mockResolvedValueOnce([
{ file: 'file2.json', key: 'prob' },
{ file: 'file2.json', key: 'actual' },
{ file: 'file2.json', key: 'step' }
])

const result = await pickPlotConfiguration('/')

expect(mockedQuickPickValue).toHaveBeenCalledWith(
expect(mockedQuickPickUserOrderedValues).toHaveBeenNthCalledWith(
1,
[
{
kind: QuickPickItemKind.Separator,
Expand All @@ -438,7 +438,8 @@ describe('pickPlotConfiguration', () => {
],
{ title: Title.SELECT_PLOT_X_METRIC }
)
expect(mockedQuickPickValues).toHaveBeenCalledWith(
expect(mockedQuickPickUserOrderedValues).toHaveBeenNthCalledWith(
2,
[
{
kind: QuickPickItemKind.Separator,
Expand Down Expand Up @@ -467,6 +468,83 @@ describe('pickPlotConfiguration', () => {
})
})

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

const result = await pickPlotConfiguration('/')

expect(mockedQuickPickUserOrderedValues).toHaveBeenNthCalledWith(
1,
[
{
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' } }
],
{ title: Title.SELECT_PLOT_X_METRIC }
)
expect(mockedQuickPickUserOrderedValues).toHaveBeenNthCalledWith(
2,
[
{
kind: QuickPickItemKind.Separator,
label: 'file.json',
value: undefined
},
{ label: 'actual', value: { file: 'file.json', key: 'actual' } },
{
kind: QuickPickItemKind.Separator,
label: 'file2.json',
value: undefined
},
{ label: 'prob', value: { file: 'file2.json', key: 'prob' } }
],
{
title: Title.SELECT_PLOT_Y_METRIC
}
)
expect(result).toStrictEqual({
template: 'simple',
title: 'simple_plot',
x: { 'file.json': 'prob', 'file2.json': 'actual' },
y: { 'file.json': 'actual', 'file2.json': 'prob' }
})
})

it('should return early if the user does not pick a template', async () => {
mockedPickFiles.mockResolvedValueOnce(['/file.json'])
mockedLoadDataFiles.mockResolvedValueOnce([
Expand All @@ -482,40 +560,44 @@ describe('pickPlotConfiguration', () => {
})

it('should return early if the user does not pick a x field', async () => {
mockedPickFiles.mockResolvedValueOnce(['/file.json'])
mockedLoadDataFiles.mockResolvedValueOnce([
mockedPickFiles.mockResolvedValue(['/file.json'])
mockedLoadDataFiles.mockResolvedValue([
{ data: mockValidData, file: 'file.json' }
])
mockedQuickPickOne.mockResolvedValueOnce('simple')
mockedGetInput.mockResolvedValueOnce('simple_plot')
mockedQuickPickValue.mockResolvedValueOnce(undefined)
mockedQuickPickOne.mockResolvedValue('simple')
mockedGetInput.mockResolvedValue('simple_plot')
mockedQuickPickUserOrderedValues
.mockResolvedValueOnce(undefined)
.mockResolvedValueOnce([])

const result = await pickPlotConfiguration('/')
const undefinedResult = await pickPlotConfiguration('/')

expect(mockedQuickPickValue).toHaveBeenCalledTimes(1)
expect(result).toStrictEqual(undefined)
})
expect(mockedQuickPickUserOrderedValues).toHaveBeenCalledTimes(1)
expect(undefinedResult).toStrictEqual(undefined)

const noFieldsResult = await pickPlotConfiguration('/')

expect(mockedQuickPickUserOrderedValues).toHaveBeenCalledTimes(2)
expect(noFieldsResult).toStrictEqual(undefined)
})
it('should return early if the user does not pick a y field', async () => {
mockedPickFiles.mockResolvedValue(['/file.json'])
mockedLoadDataFiles.mockResolvedValue([
{ data: mockValidData, file: 'file.json' }
])
mockedQuickPickOne.mockResolvedValue('simple')
mockedGetInput.mockResolvedValue('simple_plot')
mockedQuickPickValue.mockResolvedValue('actual')
mockedQuickPickValues
mockedQuickPickUserOrderedValues
.mockResolvedValueOnce(undefined)
.mockResolvedValueOnce([])
Copy link
Contributor Author

@julieg18 julieg18 Oct 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This test is actually incorrectly written. We need mockedQuickPickValues to have 4 mockResolvedValueOnce calls. Fixed in #4798


const undefinedResult = await pickPlotConfiguration('/')

expect(mockedQuickPickValues).toHaveBeenCalledTimes(1)
expect(mockedQuickPickUserOrderedValues).toHaveBeenCalledTimes(1)
expect(undefinedResult).toStrictEqual(undefined)

const emptyFieldsResult = await pickPlotConfiguration('/')

expect(mockedQuickPickValues).toHaveBeenCalledTimes(2)
expect(mockedQuickPickUserOrderedValues).toHaveBeenCalledTimes(2)
expect(emptyFieldsResult).toStrictEqual(undefined)
})

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

const result = await pickPlotConfiguration('/')
Expand Down
24 changes: 10 additions & 14 deletions extension/src/pipeline/quickPick.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,14 @@ import {
isValueTree
} from '../cli/dvc/contract'
import { getFileExtension, loadDataFiles } from '../fileSystem'
import {
quickPickManyValues,
quickPickOne,
quickPickValue
} from '../vscode/quickPick'
import { quickPickOne, quickPickUserOrderedValues } from '../vscode/quickPick'
import { pickFiles } from '../vscode/resourcePicker'
import { Title } from '../vscode/title'
import { Toast } from '../vscode/toast'
import { getInput } from '../vscode/inputBox'

export type PlotConfigData = {
x: { [file: string]: string }
x: { [file: string]: string[] | string }
julieg18 marked this conversation as resolved.
Show resolved Hide resolved
template: string
title: string
y: { [file: string]: string[] | string }
Expand All @@ -37,7 +33,7 @@ const pickDataFiles = (): Promise<string[] | undefined> =>
const formatFieldQuickPickValues = (
values: { file: string; key: string }[]
julieg18 marked this conversation as resolved.
Show resolved Hide resolved
) => {
const formattedFields: PlotConfigData['y'] = {}
const formattedFields: { [file: string]: string[] | string } = {}

for (const { file, key } of values) {
if (!formattedFields[file]) {
Expand Down Expand Up @@ -79,16 +75,16 @@ const pickFields = async (
)
}

const xValue = await quickPickValue(items, {
const xValues = (await quickPickUserOrderedValues(items, {
title: Title.SELECT_PLOT_X_METRIC
})
})) as { file: string; key: string }[] | undefined
julieg18 marked this conversation as resolved.
Show resolved Hide resolved

if (!xValue) {
if (!xValues || xValues.length === 0) {
return
}

const yValues = (await quickPickManyValues(
items.filter(item => !isEqual(item.value, xValue)),
const yValues = (await quickPickUserOrderedValues(
items.filter(item => xValues.every(val => !isEqual(val, item.value))),
julieg18 marked this conversation as resolved.
Show resolved Hide resolved
{ title: Title.SELECT_PLOT_Y_METRIC }
)) as { file: string; key: string }[] | undefined

Expand All @@ -98,10 +94,10 @@ const pickFields = async (

return {
fields: {
x: { [xValue.file]: xValue.key },
x: formatFieldQuickPickValues(xValues),
y: formatFieldQuickPickValues(yValues)
},
firstXKey: xValue.key,
firstXKey: xValues[0].key,
firstYKey: yValues[0].key
}
}
Expand Down
Loading
Loading