Skip to content

Commit 5851790

Browse files
julieg18julieg18
authored
Save comparison multi plot image values across sessions (#4476)
1 parent c2c0cb8 commit 5851790

File tree

13 files changed

+188
-7
lines changed

13 files changed

+188
-7
lines changed

‎extension/src/persistence/constants.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ export enum PersistenceKey {
1818
PLOT_SECTION_COLLAPSED = 'plotSectionCollapsed:',
1919
PLOT_SELECTED_METRICS = 'plotSelectedMetrics:',
2020
PLOTS_SMOOTH_PLOT_VALUES = 'plotSmoothPlotValues:',
21+
PLOTS_COMPARISON_MULTI_PLOT_VALUES = 'plotComparisonMultiPlotValues:',
2122
PLOT_TEMPLATE_ORDER = 'plotTemplateOrder:',
2223
SHOW_ONLY_CHANGED = 'columnsShowOnlyChanged:'
2324
}

‎extension/src/plots/model/index.ts‎

Lines changed: 28 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,8 @@ import {
3333
DEFAULT_NB_ITEMS_PER_ROW,
3434
PlotHeight,
3535
SmoothPlotValues,
36-
ImagePlot
36+
ImagePlot,
37+
ComparisonMultiPlotValues
3738
} from '../webview/contract'
3839
import {
3940
EXPERIMENT_WORKSPACE_ID,
@@ -80,6 +81,7 @@ export class PlotsModel extends ModelWithPersistence {
8081

8182
private comparisonData: ComparisonData = {}
8283
private comparisonOrder: string[]
84+
private comparisonMultiPlotValues: ComparisonMultiPlotValues = {}
8385
private smoothPlotValues: SmoothPlotValues = {}
8486

8587
private revisionData: RevisionData = {}
@@ -113,6 +115,10 @@ export class PlotsModel extends ModelWithPersistence {
113115
PersistenceKey.PLOTS_SMOOTH_PLOT_VALUES,
114116
{}
115117
)
118+
this.comparisonMultiPlotValues = this.revive(
119+
PersistenceKey.PLOTS_COMPARISON_MULTI_PLOT_VALUES,
120+
{}
121+
)
116122

117123
this.cleanupOutdatedCustomPlotsState()
118124
this.cleanupOutdatedTrendsState()
@@ -307,6 +313,26 @@ export class PlotsModel extends ModelWithPersistence {
307313
this.persist(PersistenceKey.PLOT_COMPARISON_ORDER, this.comparisonOrder)
308314
}
309315

316+
public setComparisonMultiPlotValue(
317+
revision: string,
318+
path: string,
319+
value: number
320+
) {
321+
if (!this.comparisonMultiPlotValues[revision]) {
322+
this.comparisonMultiPlotValues[revision] = {}
323+
}
324+
325+
this.comparisonMultiPlotValues[revision][path] = value
326+
this.persist(
327+
PersistenceKey.PLOTS_COMPARISON_MULTI_PLOT_VALUES,
328+
this.comparisonMultiPlotValues
329+
)
330+
}
331+
332+
public getComparisonMultiPlotValues() {
333+
return this.comparisonMultiPlotValues
334+
}
335+
310336
public getSelectedRevisionIds() {
311337
return this.experiments.getSelectedRevisions().map(({ id }) => id)
312338
}
@@ -394,6 +420,7 @@ export class PlotsModel extends ModelWithPersistence {
394420
...this.comparisonData,
395421
...comparisonData
396422
}
423+
397424
this.revisionData = {
398425
...this.revisionData,
399426
...revisionData

‎extension/src/plots/webview/contract.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,11 +66,16 @@ export type Revision = {
6666
label: string
6767
}
6868

69+
export type ComparisonMultiPlotValues = {
70+
[revision: string]: { [path: string]: number }
71+
}
72+
6973
export interface PlotsComparisonData {
7074
plots: ComparisonPlots
7175
width: number
7276
height: PlotHeight
7377
revisions: Revision[]
78+
multiPlotValues: ComparisonMultiPlotValues
7479
}
7580

7681
export type CustomPlotValues = {

‎extension/src/plots/webview/messages.ts‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,12 @@ export class WebviewMessages {
102102
return this.selectPlotsFromWebview()
103103
case MessageFromWebviewType.SELECT_EXPERIMENTS:
104104
return this.selectExperimentsFromWebview()
105+
case MessageFromWebviewType.SET_COMPARISON_MULTI_PLOT_VALUE:
106+
return this.setComparisonMultiPlotValue(
107+
message.payload.revision,
108+
message.payload.path,
109+
message.payload.value
110+
)
105111
case MessageFromWebviewType.REMOVE_CUSTOM_PLOTS:
106112
return commands.executeCommand(
107113
RegisteredCommands.PLOTS_CUSTOM_REMOVE,
@@ -224,6 +230,20 @@ export class WebviewMessages {
224230
)
225231
}
226232

233+
private setComparisonMultiPlotValue(
234+
revision: string,
235+
path: string,
236+
value: number
237+
) {
238+
this.plots.setComparisonMultiPlotValue(revision, path, value)
239+
this.sendComparisonPlots()
240+
sendTelemetryEvent(
241+
EventName.VIEWS_PLOTS_SET_COMPARISON_MULTI_PLOT_VALUE,
242+
undefined,
243+
undefined
244+
)
245+
}
246+
227247
private setTemplateOrder(order: PlotsTemplatesReordered) {
228248
this.paths.setTemplateOrder(order)
229249
this.sendTemplatePlots()
@@ -345,6 +365,7 @@ export class WebviewMessages {
345365

346366
return {
347367
height: this.plots.getHeight(PlotsSection.COMPARISON_TABLE),
368+
multiPlotValues: this.plots.getComparisonMultiPlotValues(),
348369
plots: comparison.map(({ path, revisions }) => {
349370
return { path, revisions: this.getRevisionsWithCorrectUrls(revisions) }
350371
}),

‎extension/src/telemetry/constants.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,8 @@ export const EventName = Object.assign(
9292
VIEWS_PLOTS_SECTION_TOGGLE: 'views.plots.toggleSection',
9393
VIEWS_PLOTS_SELECT_EXPERIMENTS: 'view.plots.selectExperiments',
9494
VIEWS_PLOTS_SELECT_PLOTS: 'view.plots.selectPlots',
95+
VIEWS_PLOTS_SET_COMPARISON_MULTI_PLOT_VALUE:
96+
'view.plots.setComparisonMultiPlotValue',
9597
VIEWS_PLOTS_SET_SMOOTH_PLOT_VALUE: 'view.plots.setSmoothPlotValues',
9698
VIEWS_PLOTS_ZOOM_PLOT: 'views.plots.zoomPlot',
9799
VIEWS_REORDER_PLOTS_CUSTOM: 'views.plots.customReordered',
@@ -296,6 +298,7 @@ export interface IEventNamePropertyMapping {
296298
[EventName.VIEWS_PLOTS_ZOOM_PLOT]: { isImage: boolean }
297299
[EventName.VIEWS_REORDER_PLOTS_CUSTOM]: undefined
298300
[EventName.VIEWS_REORDER_PLOTS_TEMPLATES]: undefined
301+
[EventName.VIEWS_PLOTS_SET_COMPARISON_MULTI_PLOT_VALUE]: undefined
299302
[EventName.VIEWS_PLOTS_SET_SMOOTH_PLOT_VALUE]: undefined
300303

301304
[EventName.VIEWS_PLOTS_PATH_TREE_OPENED]: DvcRootCount

‎extension/src/test/fixtures/plotsDiff/index.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -811,6 +811,7 @@ export const getComparisonWebviewMessage = (
811811

812812
return {
813813
revisions: getRevisions(),
814+
multiPlotValues: {},
814815
plots: Object.values(plotAcc),
815816
width: DEFAULT_PLOT_WIDTH,
816817
height: DEFAULT_PLOT_HEIGHT

‎extension/src/test/suite/plots/index.test.ts‎

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1065,6 +1065,42 @@ suite('Plots Test Suite', () => {
10651065
)
10661066
})
10671067

1068+
it('should handle an update comparison multi plot value message from the webview', async () => {
1069+
const { mockMessageReceived, plotsModel } = await buildPlotsWebview({
1070+
disposer: disposable,
1071+
plotsDiff: plotsDiffFixture
1072+
})
1073+
const multiImg = comparisonPlotsFixture.plots[3]
1074+
1075+
const mockSendTelemetryEvent = stub(Telemetry, 'sendTelemetryEvent')
1076+
const mockSetComparisonMultiPlotValue = stub(
1077+
plotsModel,
1078+
'setComparisonMultiPlotValue'
1079+
)
1080+
1081+
mockMessageReceived.fire({
1082+
payload: {
1083+
path: multiImg.path,
1084+
revision: 'main',
1085+
value: 5
1086+
},
1087+
type: MessageFromWebviewType.SET_COMPARISON_MULTI_PLOT_VALUE
1088+
})
1089+
1090+
expect(mockSendTelemetryEvent).to.be.called
1091+
expect(mockSendTelemetryEvent).to.be.calledWithExactly(
1092+
EventName.VIEWS_PLOTS_SET_COMPARISON_MULTI_PLOT_VALUE,
1093+
undefined,
1094+
undefined
1095+
)
1096+
expect(mockSetComparisonMultiPlotValue).to.be.called
1097+
expect(mockSetComparisonMultiPlotValue).to.be.calledWithExactly(
1098+
'main',
1099+
multiImg.path,
1100+
5
1101+
)
1102+
})
1103+
10681104
it('should handle the CLI throwing an error', async () => {
10691105
const { data, errorsModel, mockPlotsDiff, plots, plotsModel } =
10701106
await buildPlots({ disposer: disposable, plotsDiff: plotsDiffFixture })

‎extension/src/webview/contract.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ export enum MessageFromWebviewType {
4444
RESIZE_COLUMN = 'resize-column',
4545
RESIZE_PLOTS = 'resize-plots',
4646
SAVE_STUDIO_TOKEN = 'save-studio-token',
47+
SET_COMPARISON_MULTI_PLOT_VALUE = 'update-comparison-multi-plot-value',
4748
SET_SMOOTH_PLOT_VALUE = 'update-smooth-plot-value',
4849
SHOW_EXPERIMENT_LOGS = 'show-experiment-logs',
4950
SHOW_WALKTHROUGH = 'show-walkthrough',
@@ -211,6 +212,10 @@ export type MessageFromWebview =
211212
type: MessageFromWebviewType.REORDER_PLOTS_COMPARISON_ROWS
212213
payload: string[]
213214
}
215+
| {
216+
type: MessageFromWebviewType.SET_COMPARISON_MULTI_PLOT_VALUE
217+
payload: { path: string; revision: string; value: number }
218+
}
214219
| {
215220
type: MessageFromWebviewType.REORDER_PLOTS_CUSTOM
216221
payload: string[]

‎webview/src/plots/components/App.test.tsx‎

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -225,6 +225,7 @@ describe('App', () => {
225225
renderAppWithOptionalData({
226226
comparison: {
227227
height: DEFAULT_PLOT_HEIGHT,
228+
multiPlotValues: {},
228229
plots: [
229230
{
230231
path: 'training/plots/images/misclassified.jpg',
@@ -279,6 +280,7 @@ describe('App', () => {
279280
renderAppWithOptionalData({
280281
comparison: {
281282
height: DEFAULT_PLOT_HEIGHT,
283+
multiPlotValues: {},
282284
plots: [
283285
{
284286
path: 'training/plots/images/image',
@@ -1757,6 +1759,25 @@ describe('App', () => {
17571759

17581760
const workspaceImgs =
17591761
comparisonTableFixture.plots[3].revisions.workspace.imgs
1762+
const multiImgPlots = screen.getAllByTestId('multi-image-cell')
1763+
const slider = within(multiImgPlots[0]).getByRole('slider')
1764+
const workspaceImgEl = within(multiImgPlots[0]).getByRole('img')
1765+
1766+
expect(workspaceImgEl).toHaveAttribute('src', workspaceImgs[0].url)
1767+
1768+
fireEvent.change(slider, { target: { value: 3 } })
1769+
1770+
expect(workspaceImgEl).toHaveAttribute('src', workspaceImgs[3].url)
1771+
})
1772+
1773+
it('should send a message when the slider changes', async () => {
1774+
renderAppWithOptionalData({
1775+
comparison: comparisonTableFixture
1776+
})
1777+
1778+
const multiImg = comparisonTableFixture.plots[3]
1779+
const workspacePlot = multiImg.revisions.workspace
1780+
const workspaceImgs = workspacePlot.imgs
17601781

17611782
const multiImgPlots = screen.getAllByTestId('multi-image-cell')
17621783
const slider = within(multiImgPlots[0]).getByRole('slider')
@@ -1766,6 +1787,34 @@ describe('App', () => {
17661787

17671788
fireEvent.change(slider, { target: { value: 3 } })
17681789

1790+
await waitFor(
1791+
() => {
1792+
expect(mockPostMessage).toHaveBeenCalledWith({
1793+
payload: {
1794+
path: multiImg.path,
1795+
revision: workspacePlot.id,
1796+
value: 3
1797+
},
1798+
type: MessageFromWebviewType.SET_COMPARISON_MULTI_PLOT_VALUE
1799+
})
1800+
},
1801+
{ timeout: 1000 }
1802+
)
1803+
})
1804+
1805+
it('should set default slider value if given a saved value', () => {
1806+
const multiImg = comparisonTableFixture.plots[3]
1807+
renderAppWithOptionalData({
1808+
comparison: {
1809+
...comparisonTableFixture,
1810+
multiPlotValues: { workspace: { [multiImg.path]: 3 } }
1811+
}
1812+
})
1813+
1814+
const workspaceImgs = multiImg.revisions.workspace.imgs
1815+
const multiImgPlots = screen.getAllByTestId('multi-image-cell')
1816+
const workspaceImgEl = within(multiImgPlots[0]).getByRole('img')
1817+
17691818
expect(workspaceImgEl).toHaveAttribute('src', workspaceImgs[3].url)
17701819
})
17711820

@@ -1795,7 +1844,6 @@ describe('App', () => {
17951844
})
17961845

17971846
const mainImgs = comparisonTableFixture.plots[3].revisions.main.imgs
1798-
17991847
const multiImgPlots = screen.getAllByTestId('multi-image-cell')
18001848
const slider = within(multiImgPlots[1]).getByRole('slider')
18011849

‎webview/src/plots/components/comparisonTable/cell/ComparisonTableMultiCell.tsx‎

Lines changed: 26 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,16 +1,23 @@
1-
import React, { useCallback, useState } from 'react'
2-
import { useDispatch } from 'react-redux'
1+
import React, { useEffect, useCallback, useRef, useState } from 'react'
2+
import { useDispatch, useSelector } from 'react-redux'
33
import { ComparisonPlot } from 'dvc/src/plots/webview/contract'
44
import { ComparisonTableCell } from './ComparisonTableCell'
55
import styles from '../styles.module.scss'
66
import { changeDisabledDragIds } from '../comparisonTableSlice'
7+
import { setComparisonMultiPlotValue } from '../../../util/messages'
8+
import { PlotsState } from '../../../store'
79

810
export const ComparisonTableMultiCell: React.FC<{
911
path: string
1012
plot: ComparisonPlot
1113
}> = ({ path, plot }) => {
12-
const [currentStep, setCurrentStep] = useState<number>(0)
14+
const values = useSelector(
15+
(state: PlotsState) => state.comparison.multiPlotValues
16+
)
17+
const [currentStep, setCurrentStep] = useState(values?.[plot.id]?.[path] || 0)
1318
const dispatch = useDispatch()
19+
const maxStep = plot.imgs.length - 1
20+
const changeDebounceTimer = useRef(0)
1421

1522
const addDisabled = useCallback(() => {
1623
dispatch(changeDisabledDragIds([path]))
@@ -20,6 +27,16 @@ export const ComparisonTableMultiCell: React.FC<{
2027
dispatch(changeDisabledDragIds([]))
2128
}, [dispatch])
2229

30+
useEffect(() => {
31+
window.clearTimeout(changeDebounceTimer.current)
32+
changeDebounceTimer.current = window.setTimeout(() => {
33+
if (currentStep === values?.[plot.id]?.[path]) {
34+
return
35+
}
36+
setComparisonMultiPlotValue(path, plot.id, currentStep)
37+
}, 500)
38+
}, [values, path, plot.id, currentStep])
39+
2340
return (
2441
<div data-testid="multi-image-cell" className={styles.multiImageWrapper}>
2542
<ComparisonTableCell
@@ -45,10 +62,14 @@ export const ComparisonTableMultiCell: React.FC<{
4562
<input
4663
name={`${plot.id}-step`}
4764
min="0"
48-
max={plot.imgs.length - 1}
49-
value={currentStep}
65+
max={maxStep}
5066
type="range"
67+
value={currentStep}
5168
onChange={event => {
69+
if (!event.target) {
70+
return
71+
}
72+
5273
setCurrentStep(Number(event.target.value))
5374
}}
5475
/>

0 commit comments

Comments
 (0)