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

Jump to bottom

Fix image by step plots with bounding boxes #5313

Closed
wants to merge 7 commits into from
Closed

Fix image by step plots with bounding boxes #5313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
158f1cb
first iteration
julieg18 Feb 19, 2024
dab423f
fix ci test
julieg18 Feb 20, 2024
ce8eee7
Merge branch 'improve-bounding-box-frontend' into fix-bb-img-by-step-…
julieg18 Feb 20, 2024
2b1d445
Merge branch 'improve-bounding-box-frontend' into fix-bb-img-by-step-…
julieg18 Feb 20, 2024
1c33606
Improve test
julieg18 Feb 20, 2024
966218d
Merge branch 'improve-bounding-box-frontend' into fix-bb-img-by-step-…
julieg18 Feb 20, 2024
abc37e1
fix typo
julieg18 Feb 22, 2024
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
52 changes: 39 additions & 13 deletions extension/src/plots/model/collect.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
}
}

Expand All @@ -520,7 +546,7 @@ export const collectSelectedComparisonPlotClasses = ({
}

for (const id of selectedRevisionIds) {
collectedSelectedPathComparisonPlotClasses({
collectSelectedPathComparisonPlotClasses({
acc,
comparisonData,
id,
Expand Down
4 changes: 3 additions & 1 deletion extension/src/plots/webview/contract.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,9 @@ export type ComparisonPlotClass = {
}

export type ComparisonPlotClasses = {
[revision: string]: { [path: string]: ComparisonPlotClass[] }
[revision: string]: {
[path: string]: { [imgInd: number]: ComparisonPlotClass[] }
Copy link
Contributor Author

@julieg18 julieg18 Feb 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In image by step plots case, path is a directory and could hold multiple images' class coordinates. I've amended this by adding a ind key.

Another option that we could do is an nested array::

[revision: string]: { [path: string]: Array<ComparisonPlotClass[] | null>  }

But it felt like collection code would be a bit more complicated (would have to filter out created arrays that were filled with only null while keeping the ones that have at least one img with classes while ensuring we don't accidentally break the order of the imgs' classes).

Copy link
Contributor

Choose a reason for hiding this comment

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

You know the context better than I do, so don't change it if it doesn't make sense or if it's still more complicated, but would using an empty array instead of null make thing simpler? So:

[revision: string]: { [path: string]: ComparisonPlotClass[][] }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using an empty array to represent an image that has no classes is doable and I'm happy to change if it benefits code simplicity or code optimization! But we would still face the same issue of needing to add an extra check for seeing if an array is just filled with empty arrays...

Though now that I look over this code again, I think the check would be doable without adding an extra loop (what I originally thought and was trying to avoid). I'll give it a shot and see how it goes!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to use a nested array! @sroy3, can you take another look at this PR when you have some time since I've basically altered most of my original changes 😅.

}
}

export interface PlotsComparisonData {
Expand Down
11 changes: 9 additions & 2 deletions extension/src/test/fixtures/plotsDiff/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1039,11 +1039,13 @@ export const collectPlotClasses = ({
imgLabels,
imgAnnotations,
id,
path
path,
imgInd
}: {
plotClasses: ComparisonPlotClasses
imgLabels: string[]
imgAnnotations: { [label: string]: BoundingBox[] }
imgInd: number
id: string
path: string
}) => {
Expand All @@ -1057,7 +1059,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 = (
Expand Down Expand Up @@ -1125,6 +1131,7 @@ export const getComparisonWebviewMessage = (
plotClasses,
imgAnnotations: annotations,
imgLabels,
imgInd: img.ind || 0,
id,
path
})
Expand Down
226 changes: 153 additions & 73 deletions extension/src/test/suite/plots/index.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -259,6 +260,158 @@ suite('Plots Test Suite', () => {
})
})

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 mockAnnotations = {
car: [{ box: { bottom: 0, left: 0, right: 0, top: 0 }, score: 0.5 }]
}
const imgDataWithBoundingBoxes = plotsDiffFixture.data[
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another option is adding a new image by step plot with bounding boxes plot to our base comparison fixture.

multiImgPath
].map(img => {
const [rev] = img.revisions
if (rev === 'main' || rev === 'exp-e7a67') {
return {
...img,
annotations: mockAnnotations
}
}

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: mockAnnotations.car, label: 'car' }]
}
},
main: {
...comparisonPlotsFixture.plotClasses.main,
[multiImgDirPath]: {
'5': [{ boxes: mockAnnotations.car, label: 'car' }]
}
}
}

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 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 a section resized message from the webview', async () => {
const { mockMessageReceived, plotsModel } = await buildPlotsWebview({
disposer: disposable
Expand Down Expand Up @@ -1278,79 +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]: classes.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()
mockMessageReceived.fire({
payload: {
label: toggledLabel,
path: boundingBoxPlot.path,
selected: false
},
type: MessageFromWebviewType.TOGGLE_COMPARISON_CLASS
})

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,
Expand Down
Loading
Loading