From 158f1cb4c5ea8d965b606b333acd6f5d5a50df5e Mon Sep 17 00:00:00 2001 From: julieg18 Date: Mon, 19 Feb 2024 11:24:02 -0600 Subject: [PATCH 1/4] first iteration --- extension/src/plots/model/collect.ts | 52 +++++++++++++----- extension/src/plots/webview/contract.ts | 4 +- .../src/test/fixtures/plotsDiff/index.ts | 11 +++- extension/src/test/suite/plots/index.test.ts | 53 ++++++++++++++++++- .../cell/ComparisonTableBoundingBoxImg.tsx | 16 ++++-- .../cell/ComparisonTableCell.tsx | 4 +- .../cell/ComparisonTableMultiCell.tsx | 1 + 7 files changed, 117 insertions(+), 24 deletions(-) diff --git a/extension/src/plots/model/collect.ts b/extension/src/plots/model/collect.ts index 07ac9ae175..016c9caa10 100644 --- a/extension/src/plots/model/collect.ts +++ b/extension/src/plots/model/collect.ts @@ -468,7 +468,41 @@ const getSelectedImgComparisonPlotClasses = ({ return imgClasses } -const collectedSelectedPathComparisonPlotClasses = ({ +const collectSelectedImgComparisonPlotClasses = ({ + acc, + id, + img, + path, + selectedClassLabels +}: { + acc: ComparisonPlotClasses + selectedClassLabels: string[] + img: ImagePlot + path: string + id: string +}) => { + const imgClasses = getSelectedImgComparisonPlotClasses({ + img, + path, + selectedClassLabels + }) + + if (imgClasses.length === 0) { + return + } + + if (!acc[id]) { + acc[id] = {} + } + + if (!acc[id][path]) { + acc[id][path] = {} + } + + acc[id][path][img.ind || 0] = imgClasses +} + +const collectSelectedPathComparisonPlotClasses = ({ acc, id, comparisonData, @@ -482,21 +516,13 @@ const collectedSelectedPathComparisonPlotClasses = ({ id: string }) => { for (const img of comparisonData[id][path]) { - const imgClasses = getSelectedImgComparisonPlotClasses({ + collectSelectedImgComparisonPlotClasses({ + acc, + id, img, path, selectedClassLabels }) - - if (imgClasses.length === 0) { - return - } - - if (!acc[id]) { - acc[id] = {} - } - - acc[id][path] = imgClasses } } @@ -520,7 +546,7 @@ export const collectSelectedComparisonPlotClasses = ({ } for (const id of selectedRevisionIds) { - collectedSelectedPathComparisonPlotClasses({ + collectSelectedPathComparisonPlotClasses({ acc, comparisonData, id, diff --git a/extension/src/plots/webview/contract.ts b/extension/src/plots/webview/contract.ts index dd0e3fe612..2d9839c4b1 100644 --- a/extension/src/plots/webview/contract.ts +++ b/extension/src/plots/webview/contract.ts @@ -94,7 +94,9 @@ export type ComparisonPlotClass = { } export type ComparisonPlotClasses = { - [revision: string]: { [path: string]: ComparisonPlotClass[] } + [revision: string]: { + [path: string]: { [imgInd: number]: ComparisonPlotClass[] } + } } export interface PlotsComparisonData { diff --git a/extension/src/test/fixtures/plotsDiff/index.ts b/extension/src/test/fixtures/plotsDiff/index.ts index 3d1acbdf4e..1ca279e224 100644 --- a/extension/src/test/fixtures/plotsDiff/index.ts +++ b/extension/src/test/fixtures/plotsDiff/index.ts @@ -1009,11 +1009,13 @@ export const collectPlotClasses = ({ imgLabels, imgBoxes, id, - path + path, + imgInd }: { plotClasses: ComparisonPlotClasses imgLabels: string[] imgBoxes: { [label: string]: BoundingBox[] } + imgInd: number id: string path: string }) => { @@ -1027,7 +1029,11 @@ export const collectPlotClasses = ({ plotClasses[id] = {} } - plotClasses[id][path] = Object.values(classAcc) + if (!plotClasses[id][path]) { + plotClasses[id][path] = {} + } + + plotClasses[id][path][imgInd] = Object.values(classAcc) } export const collectClassLabels = ( @@ -1095,6 +1101,7 @@ export const getComparisonWebviewMessage = ( plotClasses, imgBoxes: boxes, imgLabels, + imgInd: img.ind || 0, id, path }) diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index 74d4e64166..c7914b0df7 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -259,6 +259,55 @@ suite('Plots Test Suite', () => { }) }) + it('should handle a plots diff output with a comparison multi plot containing bounding boxes', async () => { + const multiImgPath = join('plots', 'image', '5.jpg') + const mockBoxes = { + car: [{ box: { bottom: 0, left: 0, right: 0, top: 0 }, score: 0.5 }] + } + const imgDataWithBoundingBoxes = plotsDiffFixture.data[multiImgPath].map( + img => { + const [rev] = img.revisions + if (rev === 'main' || rev === 'exp-e7a67') { + return { + ...img, + boxes: mockBoxes + } + } + + return img + } + ) + const { messageSpy } = await buildPlotsWebview({ + disposer: disposable, + plotsDiff: { + ...plotsDiffFixture, + data: { + ...plotsDiffFixture.data, + [multiImgPath]: imgDataWithBoundingBoxes + } + } + }) + + const comparisonPlotClasses = + messageSpy.lastCall.firstArg.comparison.plotClasses + + expect(comparisonPlotClasses).deep.equal({ + ...comparisonPlotsFixture.plotClasses, + 'exp-e7a67': { + ...comparisonPlotsFixture.plotClasses['exp-e7a67'], + [join('plots', 'image')]: { + '5': [{ boxes: mockBoxes.car, label: 'car' }] + } + }, + main: { + ...comparisonPlotsFixture.plotClasses.main, + [join('plots', 'image')]: { + '5': [{ boxes: mockBoxes.car, label: 'car' }] + } + } + }) + }) + it('should handle a section resized message from the webview', async () => { const { mockMessageReceived, plotsModel } = await buildPlotsWebview({ disposer: disposable @@ -1292,8 +1341,8 @@ suite('Plots Test Suite', () => { comparisonPlotsFixture.plotClasses )) { const classes = classesByPath[boundingBoxPlot.path] - filteredPlotsClasses[id] = { - [boundingBoxPlot.path]: classes.filter( + filteredPlotsClasses[id][0] = { + [boundingBoxPlot.path]: classes[0].filter( ({ label }) => label !== toggledLabel ) } diff --git a/webview/src/plots/components/comparisonTable/cell/ComparisonTableBoundingBoxImg.tsx b/webview/src/plots/components/comparisonTable/cell/ComparisonTableBoundingBoxImg.tsx index b2f761d554..1a5e8ccf2b 100644 --- a/webview/src/plots/components/comparisonTable/cell/ComparisonTableBoundingBoxImg.tsx +++ b/webview/src/plots/components/comparisonTable/cell/ComparisonTableBoundingBoxImg.tsx @@ -11,9 +11,14 @@ import { PlotsState } from '../../../store' const plotClassesSelector = (state: PlotsState) => state.comparison.plotClasses const classesSelector = createSelector( - [plotClassesSelector, (_, id: string) => id, (_, id, path: string) => path], - (plotClasses: ComparisonPlotClasses, id: string, path: string) => - plotClasses[id]?.[path] || [] + [ + plotClassesSelector, + (_, id: string) => id, + (_, id, path: string) => path, + (_, id, path, ind: number) => ind + ], + (plotClasses: ComparisonPlotClasses, id: string, path: string, ind: number) => + plotClasses[id]?.[path]?.[ind] || [] ) export const ComparisonTableBoundingBoxImg: React.FC<{ @@ -22,9 +27,10 @@ export const ComparisonTableBoundingBoxImg: React.FC<{ path: string classDetails: ComparisonClassDetails alt: string -}> = ({ alt, classDetails, id, src, path }) => { + ind?: number +}> = ({ alt, classDetails, id, src, path, ind = 0 }) => { const classes = useSelector((state: PlotsState) => - classesSelector(state, id, path) + classesSelector(state, id, path, ind) ) const [naturalWidth, setNaturalWidth] = useState(0) const [naturalHeight, setNaturalHeight] = useState(0) diff --git a/webview/src/plots/components/comparisonTable/cell/ComparisonTableCell.tsx b/webview/src/plots/components/comparisonTable/cell/ComparisonTableCell.tsx index 48404c6512..72e4124c53 100644 --- a/webview/src/plots/components/comparisonTable/cell/ComparisonTableCell.tsx +++ b/webview/src/plots/components/comparisonTable/cell/ComparisonTableCell.tsx @@ -14,7 +14,8 @@ export const ComparisonTableCell: React.FC<{ plot: ComparisonPlot classDetails: ComparisonClassDetails imgAlt?: string -}> = ({ path, plot, imgAlt, classDetails }) => { + imgInd?: number +}> = ({ path, plot, imgAlt, classDetails, imgInd }) => { const plotImg = plot.imgs[0] const loading = plotImg.loading @@ -42,6 +43,7 @@ export const ComparisonTableCell: React.FC<{ path={path} classDetails={classDetails} alt={alt} + ind={imgInd} /> ) : (
Date: Mon, 19 Feb 2024 18:07:12 -0600 Subject: [PATCH 2/4] fix ci test --- extension/src/test/suite/plots/index.test.ts | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index c7914b0df7..508d985b8c 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -259,7 +259,7 @@ suite('Plots Test Suite', () => { }) }) - it('should handle a plots diff output with a comparison multi plot containing bounding boxes', async () => { + it('should handle a plots diff output with a comparison multi plot containing classes', async () => { const multiImgPath = join('plots', 'image', '5.jpg') const mockBoxes = { car: [{ box: { bottom: 0, left: 0, right: 0, top: 0 }, score: 0.5 }] @@ -1341,10 +1341,10 @@ suite('Plots Test Suite', () => { comparisonPlotsFixture.plotClasses )) { const classes = classesByPath[boundingBoxPlot.path] - filteredPlotsClasses[id][0] = { - [boundingBoxPlot.path]: classes[0].filter( - ({ label }) => label !== toggledLabel - ) + filteredPlotsClasses[id] = { + [boundingBoxPlot.path]: { + 0: classes[0].filter(({ label }) => label !== toggledLabel) + } } } @@ -1367,6 +1367,9 @@ suite('Plots Test Suite', () => { const toggleComparisonClassSpy = spy(plotsModel, 'toggleComparisonClass') messageSpy.resetHistory() + + const messageSent = waitForSpyCall(messageSpy, messageSpy.callCount) + mockMessageReceived.fire({ payload: { label: toggledLabel, @@ -1376,6 +1379,8 @@ suite('Plots Test Suite', () => { type: MessageFromWebviewType.TOGGLE_COMPARISON_CLASS }) + await messageSent + expect(toggleComparisonClassSpy).to.be.called expect(toggleComparisonClassSpy).to.be.calledWithExactly( boundingBoxPlot.path, From 1c33606405e019d7568e8de7edcd24233100cd68 Mon Sep 17 00:00:00 2001 From: julieg18 Date: Tue, 20 Feb 2024 11:20:42 -0600 Subject: [PATCH 3/4] Improve test --- extension/src/test/suite/plots/index.test.ts | 244 ++++++++++--------- 1 file changed, 135 insertions(+), 109 deletions(-) diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index 508d985b8c..e219b2c2ab 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -56,6 +56,7 @@ import * as PlotsCollectUtils from '../../../plots/model/collect' import { Operator } from '../../../experiments/model/filterBy' import * as External from '../../../vscode/external' import { PlotPath } from '../../../plots/paths/collect' +import { getBoundingBoxColor } from '../../../common/colors' suite('Plots Test Suite', () => { const disposable = Disposable.fn() @@ -259,13 +260,16 @@ suite('Plots Test Suite', () => { }) }) - it('should handle a plots diff output with a comparison multi plot containing classes', async () => { - const multiImgPath = join('plots', 'image', '5.jpg') - const mockBoxes = { - car: [{ box: { bottom: 0, left: 0, right: 0, top: 0 }, score: 0.5 }] - } - const imgDataWithBoundingBoxes = plotsDiffFixture.data[multiImgPath].map( - img => { + describe('Bounding Box Plots', () => { + it('should handle image by step plots with bounding boxes', async () => { + const multiImgDirPath = join('plots', 'image') + const multiImgPath = join('plots', 'image', '5.jpg') + const mockBoxes = { + car: [{ box: { bottom: 0, left: 0, right: 0, top: 0 }, score: 0.5 }] + } + const imgDataWithBoundingBoxes = plotsDiffFixture.data[ + multiImgPath + ].map(img => { const [rev] = img.revisions if (rev === 'main' || rev === 'exp-e7a67') { return { @@ -275,37 +279,137 @@ suite('Plots Test Suite', () => { } return img + }) + + const { messageSpy } = await buildPlotsWebview({ + disposer: disposable, + plotsDiff: { + ...plotsDiffFixture, + data: { + ...plotsDiffFixture.data, + [multiImgPath]: imgDataWithBoundingBoxes + } + } + }) + + const plotsWithNewClasses = comparisonPlotsFixture.plots.map(plot => { + if (plot.path !== multiImgDirPath) { + return plot + } + + return { + ...plot, + classDetails: { + car: { color: getBoundingBoxColor(0), selected: true } + } + } + }) + + const plotsClassesWithNewClasses = { + ...comparisonPlotsFixture.plotClasses, + 'exp-e7a67': { + ...comparisonPlotsFixture.plotClasses['exp-e7a67'], + [multiImgDirPath]: { + '5': [{ boxes: mockBoxes.car, label: 'car' }] + } + }, + main: { + ...comparisonPlotsFixture.plotClasses.main, + [multiImgDirPath]: { + '5': [{ boxes: mockBoxes.car, label: 'car' }] + } + } } - ) - const { messageSpy } = await buildPlotsWebview({ - disposer: disposable, - plotsDiff: { - ...plotsDiffFixture, - data: { - ...plotsDiffFixture.data, - [multiImgPath]: imgDataWithBoundingBoxes + + expect(messageSpy).to.be.calledWithMatch({ + comparison: { + ...comparisonPlotsFixture, + plotClasses: plotsClassesWithNewClasses, + plots: plotsWithNewClasses + } + }) + }).timeout(WEBVIEW_TEST_TIMEOUT) + + it('should handle an toggle comparison class message from the webview', async () => { + const { messageSpy, mockMessageReceived, plotsModel } = + await buildPlotsWebview({ + disposer: disposable, + plotsDiff: plotsDiffFixture + }) + const toggledLabel = 'car' + const boundingBoxPlot = comparisonPlotsFixture.plots[4] + + const filteredPlotsClasses: ComparisonPlotClasses = {} + for (const [id, classesByPath] of Object.entries( + comparisonPlotsFixture.plotClasses + )) { + const classes = classesByPath[boundingBoxPlot.path] + filteredPlotsClasses[id] = { + [boundingBoxPlot.path]: { + 0: classes[0].filter(({ label }) => label !== toggledLabel) + } } } - }) - const comparisonPlotClasses = - messageSpy.lastCall.firstArg.comparison.plotClasses + const filteredPlots = comparisonPlotsFixture.plots.map(plot => { + if (plot.path !== boundingBoxPlot.path) { + return plot + } - expect(comparisonPlotClasses).deep.equal({ - ...comparisonPlotsFixture.plotClasses, - 'exp-e7a67': { - ...comparisonPlotsFixture.plotClasses['exp-e7a67'], - [join('plots', 'image')]: { - '5': [{ boxes: mockBoxes.car, label: 'car' }] + const { color } = plot.classDetails[toggledLabel] + return { + ...plot, + classDetails: { + ...plot.classDetails, + [toggledLabel]: { color, selected: false } + } } - }, - main: { - ...comparisonPlotsFixture.plotClasses.main, - [join('plots', 'image')]: { - '5': [{ boxes: mockBoxes.car, label: 'car' }] + }) + + const mockSendTelemetryEvent = stub(Telemetry, 'sendTelemetryEvent') + const toggleComparisonClassSpy = spy( + plotsModel, + 'toggleComparisonClass' + ) + + messageSpy.resetHistory() + + const messageSent = waitForSpyCall(messageSpy, messageSpy.callCount) + + mockMessageReceived.fire({ + payload: { + label: toggledLabel, + path: boundingBoxPlot.path, + selected: false + }, + type: MessageFromWebviewType.TOGGLE_COMPARISON_CLASS + }) + + await messageSent + + expect(toggleComparisonClassSpy).to.be.called + expect(toggleComparisonClassSpy).to.be.calledWithExactly( + boundingBoxPlot.path, + 'car', + false + ) + expect( + messageSpy, + "should update the webview's comparison classes" + ).to.be.calledWithExactly({ + comparison: { + ...comparisonPlotsFixture, + plotClasses: filteredPlotsClasses, + plots: filteredPlots } - } - }) + }) + expect(mockSendTelemetryEvent).to.be.called + expect(mockSendTelemetryEvent).to.be.calledWithExactly( + EventName.VIEWS_PLOTS_TOGGLE_COMPARISON_CLASS, + undefined, + undefined + ) + }).timeout(WEBVIEW_TEST_TIMEOUT) }) it('should handle a section resized message from the webview', async () => { @@ -1327,84 +1431,6 @@ suite('Plots Test Suite', () => { ) }).timeout(WEBVIEW_TEST_TIMEOUT) - it('should handle an toggle comparison class message from the webview', async () => { - const { messageSpy, mockMessageReceived, plotsModel } = - await buildPlotsWebview({ - disposer: disposable, - plotsDiff: plotsDiffFixture - }) - const toggledLabel = 'car' - const boundingBoxPlot = comparisonPlotsFixture.plots[4] - - const filteredPlotsClasses: ComparisonPlotClasses = {} - for (const [id, classesByPath] of Object.entries( - comparisonPlotsFixture.plotClasses - )) { - const classes = classesByPath[boundingBoxPlot.path] - filteredPlotsClasses[id] = { - [boundingBoxPlot.path]: { - 0: classes[0].filter(({ label }) => label !== toggledLabel) - } - } - } - - const filteredPlots = comparisonPlotsFixture.plots.map(plot => { - if (plot.path !== boundingBoxPlot.path) { - return plot - } - - const { color } = plot.classDetails[toggledLabel] - return { - ...plot, - classDetails: { - ...plot.classDetails, - [toggledLabel]: { color, selected: false } - } - } - }) - - const mockSendTelemetryEvent = stub(Telemetry, 'sendTelemetryEvent') - const toggleComparisonClassSpy = spy(plotsModel, 'toggleComparisonClass') - - messageSpy.resetHistory() - - const messageSent = waitForSpyCall(messageSpy, messageSpy.callCount) - - mockMessageReceived.fire({ - payload: { - label: toggledLabel, - path: boundingBoxPlot.path, - selected: false - }, - type: MessageFromWebviewType.TOGGLE_COMPARISON_CLASS - }) - - await messageSent - - expect(toggleComparisonClassSpy).to.be.called - expect(toggleComparisonClassSpy).to.be.calledWithExactly( - boundingBoxPlot.path, - 'car', - false - ) - expect( - messageSpy, - "should update the webview's comparison classes" - ).to.be.calledWithExactly({ - comparison: { - ...comparisonPlotsFixture, - plotClasses: filteredPlotsClasses, - plots: filteredPlots - } - }) - expect(mockSendTelemetryEvent).to.be.called - expect(mockSendTelemetryEvent).to.be.calledWithExactly( - EventName.VIEWS_PLOTS_TOGGLE_COMPARISON_CLASS, - undefined, - undefined - ) - }).timeout(WEBVIEW_TEST_TIMEOUT) - it('should handle an add plot message from the webview', async () => { const { mockMessageReceived } = await buildPlotsWebview({ disposer: disposable, From abc37e1d1b00ef392bfa532db984ce2503b3716e Mon Sep 17 00:00:00 2001 From: julieg18 Date: Thu, 22 Feb 2024 10:14:59 -0600 Subject: [PATCH 4/4] fix typo --- extension/src/test/suite/plots/index.test.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/extension/src/test/suite/plots/index.test.ts b/extension/src/test/suite/plots/index.test.ts index e219b2c2ab..0935fee322 100644 --- a/extension/src/test/suite/plots/index.test.ts +++ b/extension/src/test/suite/plots/index.test.ts @@ -264,7 +264,7 @@ suite('Plots Test Suite', () => { it('should handle image by step plots with bounding boxes', async () => { const multiImgDirPath = join('plots', 'image') const multiImgPath = join('plots', 'image', '5.jpg') - const mockBoxes = { + const mockAnnotations = { car: [{ box: { bottom: 0, left: 0, right: 0, top: 0 }, score: 0.5 }] } const imgDataWithBoundingBoxes = plotsDiffFixture.data[ @@ -274,7 +274,7 @@ suite('Plots Test Suite', () => { if (rev === 'main' || rev === 'exp-e7a67') { return { ...img, - boxes: mockBoxes + annotations: mockAnnotations } } @@ -310,13 +310,13 @@ suite('Plots Test Suite', () => { 'exp-e7a67': { ...comparisonPlotsFixture.plotClasses['exp-e7a67'], [multiImgDirPath]: { - '5': [{ boxes: mockBoxes.car, label: 'car' }] + '5': [{ boxes: mockAnnotations.car, label: 'car' }] } }, main: { ...comparisonPlotsFixture.plotClasses.main, [multiImgDirPath]: { - '5': [{ boxes: mockBoxes.car, label: 'car' }] + '5': [{ boxes: mockAnnotations.car, label: 'car' }] } } }