-
Notifications
You must be signed in to change notification settings - Fork 29
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.
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 backend logic for bounding box plots #5250
Changes from 17 commits
d32537c
f6fb6c2
169384b
91c905e
9045ab0
1e76753
2c85d58
ef0b553
0692a99
8cace73
ba4bdd0
b1abf5d
dd431e1
d240272
4da57cb
d09e5f0
646306a
86767f6
7fce713
0176f1a
ff95253
c6e59f0
06b6f5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,11 @@ import { | |
CustomPlotData, | ||
CustomPlotValues, | ||
ComparisonRevisionData, | ||
ComparisonPlotImg | ||
ComparisonPlotImg, | ||
ComparisonClassDetails, | ||
ComparisonPlotClasses, | ||
ComparisonClassesSelected, | ||
ComparisonPlotClass | ||
} from '../webview/contract' | ||
import { | ||
AnchorDefinitions, | ||
|
@@ -23,7 +27,9 @@ import { | |
PlotOutput, | ||
PlotsOutput, | ||
PlotsType, | ||
TemplatePlotOutput | ||
TemplatePlotOutput, | ||
ImagePlotOutput, | ||
BoundingBox | ||
} from '../../cli/dvc/contract' | ||
import { splitColumnPath } from '../../experiments/columns/paths' | ||
import { ColumnType, Experiment } from '../../experiments/webview/contract' | ||
|
@@ -205,10 +211,26 @@ const getMultiImageInd = (path: string) => { | |
return Number(fileName) | ||
} | ||
|
||
const collectImageBoundingBoxes = ( | ||
boundingBoxes: { label: string; box: BoundingBox }[] | ||
): { [label: string]: BoundingBox[] } => { | ||
const acc: { [label: string]: BoundingBox[] } = {} | ||
|
||
for (const { label, box } of boundingBoxes) { | ||
if (!acc[label]) { | ||
acc[label] = [] | ||
sroy3 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
acc[label].push(box) | ||
} | ||
|
||
return acc | ||
} | ||
|
||
const collectImageData = ( | ||
acc: ComparisonData, | ||
path: string, | ||
plot: ImagePlot | ||
plot: ImagePlotOutput | ||
) => { | ||
const isMultiImgPlot = MULTI_IMAGE_PATH_REG.test(path) | ||
const pathLabel = isMultiImgPlot ? getMultiImagePath(path) : path | ||
|
@@ -226,12 +248,20 @@ const collectImageData = ( | |
acc[id][pathLabel] = [] | ||
} | ||
|
||
const imgPlot: ImagePlot = { ...plot } | ||
const imgPlot: ImagePlot = { | ||
revisions: plot.revisions, | ||
type: plot.type, | ||
url: plot.url | ||
} | ||
|
||
if (isMultiImgPlot) { | ||
imgPlot.ind = getMultiImageInd(path) | ||
} | ||
|
||
if (plot.boundingBoxes) { | ||
imgPlot.boundingBoxes = collectImageBoundingBoxes(plot.boundingBoxes) | ||
} | ||
|
||
acc[id][pathLabel].push(imgPlot) | ||
} | ||
|
||
|
@@ -312,60 +342,120 @@ export const collectData = (output: PlotsOutput): DataAccumulator => { | |
return acc | ||
} | ||
|
||
type ComparisonPlotsAcc = { path: string; revisions: ComparisonRevisionData }[] | ||
type ComparisonPlotsAcc = { | ||
classDetails: ComparisonClassDetails | ||
path: string | ||
revisions: ComparisonRevisionData | ||
}[] | ||
|
||
type GetComparisonPlotImg = ( | ||
img: ImagePlot, | ||
id: string, | ||
path: string | ||
) => ComparisonPlotImg | ||
|
||
export const boundingBoxColors = [ | ||
'#ff3838', | ||
'#ff9d97', | ||
'#ff701f', | ||
'#ffb21d', | ||
'#cfd231', | ||
'#48f90a', | ||
'#92cc17', | ||
'#3ddb86', | ||
'#1a9334', | ||
'#00d4bb', | ||
'#2c99a8', | ||
'#00c2ff', | ||
'#344593', | ||
'#6473ff', | ||
'#0018ec', | ||
'#8438ff', | ||
'#520085', | ||
'#cb38ff', | ||
'#ff95c8', | ||
'#ff37c7' | ||
] | ||
julieg18 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
const collectSelectedPlotImgClassLabels = ( | ||
boundingBoxClassLabels: Set<string>, | ||
imgs: ImagePlot[] = [] | ||
) => { | ||
for (const img of imgs) { | ||
if (!img.boundingBoxes) { | ||
continue | ||
} | ||
|
||
for (const label of Object.keys(img.boundingBoxes)) { | ||
boundingBoxClassLabels.add(label) | ||
} | ||
} | ||
} | ||
|
||
const collectSelectedPathComparisonPlots = ({ | ||
acc, | ||
comparisonData, | ||
comparisonClassesSelected, | ||
path, | ||
selectedRevisionIds, | ||
getComparisonPlotImg | ||
}: { | ||
acc: ComparisonPlotsAcc | ||
comparisonData: ComparisonData | ||
comparisonClassesSelected: ComparisonClassesSelected | ||
path: string | ||
selectedRevisionIds: string[] | ||
getComparisonPlotImg: GetComparisonPlotImg | ||
}) => { | ||
const boundingBoxClassLabels = new Set<string>() | ||
const pathRevisions = { | ||
classDetails: {} as ComparisonClassDetails, | ||
path, | ||
revisions: {} as ComparisonRevisionData | ||
} | ||
|
||
for (const id of selectedRevisionIds) { | ||
const imgs = comparisonData[id]?.[path] | ||
const imgs: ImagePlot[] | undefined = comparisonData[id]?.[path] | ||
|
||
pathRevisions.revisions[id] = { | ||
id, | ||
imgs: imgs | ||
? imgs.map(img => getComparisonPlotImg(img, id, path)) | ||
: [{ errors: undefined, loading: false, url: undefined }] | ||
} | ||
collectSelectedPlotImgClassLabels(boundingBoxClassLabels, imgs) | ||
} | ||
|
||
for (const [ind, label] of [...boundingBoxClassLabels].entries()) { | ||
julieg18 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
const selectedState = comparisonClassesSelected[path]?.[label] | ||
pathRevisions.classDetails[label] = { | ||
color: boundingBoxColors[ind % boundingBoxColors.length], | ||
selected: selectedState === undefined ? true : selectedState | ||
} | ||
} | ||
|
||
acc.push(pathRevisions) | ||
} | ||
|
||
export const collectSelectedComparisonPlots = ({ | ||
comparisonData, | ||
comparisonClassesSelected, | ||
paths, | ||
selectedRevisionIds, | ||
getComparisonPlotImg | ||
}: { | ||
comparisonData: ComparisonData | ||
comparisonClassesSelected: ComparisonClassesSelected | ||
paths: string[] | ||
selectedRevisionIds: string[] | ||
getComparisonPlotImg: GetComparisonPlotImg | ||
}) => { | ||
}): ComparisonPlotsAcc => { | ||
const acc: ComparisonPlotsAcc = [] | ||
|
||
for (const path of paths) { | ||
collectSelectedPathComparisonPlots({ | ||
acc, | ||
comparisonClassesSelected, | ||
comparisonData, | ||
getComparisonPlotImg, | ||
path, | ||
|
@@ -376,6 +466,93 @@ export const collectSelectedComparisonPlots = ({ | |
return acc | ||
} | ||
|
||
const getSelectedImgComparisonPlotClasses = ({ | ||
comparisonClassesSelected, | ||
img, | ||
path | ||
}: { | ||
comparisonClassesSelected: ComparisonClassesSelected | ||
img: ImagePlot | ||
path: string | ||
}) => { | ||
const imgClasses: ComparisonPlotClass[] = [] | ||
|
||
for (const [label, boxes] of Object.entries(img.boundingBoxes || {})) { | ||
const selectedState = comparisonClassesSelected[path]?.[label] | ||
|
||
if (selectedState === false) { | ||
continue | ||
} | ||
|
||
imgClasses.push({ | ||
boxes, | ||
label | ||
}) | ||
} | ||
|
||
return imgClasses | ||
} | ||
|
||
const collectedSelectedPathComparisonPlotClasses = ({ | ||
acc, | ||
comparisonClassesSelected, | ||
id, | ||
comparisonData, | ||
path | ||
}: { | ||
comparisonClassesSelected: ComparisonClassesSelected | ||
acc: ComparisonPlotClasses | ||
comparisonData: ComparisonData | ||
path: string | ||
id: string | ||
}) => { | ||
for (const img of comparisonData[id][path]) { | ||
const imgClasses = getSelectedImgComparisonPlotClasses({ | ||
comparisonClassesSelected, | ||
img, | ||
path | ||
}) | ||
|
||
if (imgClasses.length === 0) { | ||
return | ||
} | ||
|
||
if (!acc[id]) { | ||
acc[id] = {} | ||
} | ||
|
||
acc[id][path] = imgClasses | ||
} | ||
} | ||
|
||
export const collectSelectedComparisonPlotClasses = ({ | ||
comparisonClassesSelected, | ||
comparisonData, | ||
paths, | ||
selectedRevisionIds | ||
}: { | ||
comparisonClassesSelected: ComparisonClassesSelected | ||
comparisonData: ComparisonData | ||
paths: string[] | ||
selectedRevisionIds: string[] | ||
}) => { | ||
const acc: ComparisonPlotClasses = {} | ||
|
||
for (const id of selectedRevisionIds) { | ||
for (const path of paths) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What I meant by removing nested loops was not moving one inside of another function (I know realize we have 4 nested loops total), but can we change the code to drop some or at least make them more performant. I see that in the fourth nested loop, we check for selected state. Maybe for a start, we could loop only through There might be other optimizations to be made as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification!
The four loops are:
I don't see a way to get rid of the first three loops, since for us to gather a plot's classes, we need to check for classes in each single image and collect the data. Atleast the third loop will likely just be a single item array. We could get rid of the fourth loop by passing all image box data to the frontend and have Another alternative is having TL;DRI'm not seeing a good way to get rid of any of our four loops. I'll review the loops and see what we can do to optimize them for now. @sroy3, what do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Reviewed optimizing the loops! Instead of looping over every revision/path in
Not seeing any other optimizations besides the two ideas I mentioned in my previous comment (have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One thing I'm seeing is this:
Why do we have to go through four loops to return nothing when we know it before hand? I would make this the first loop (the selected classes). If there are no other possible optimizations, then I guess it's fine to keep as is. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Aren't we already getting the selected classes in the first loop? In fact the line
Good catch on |
||
collectedSelectedPathComparisonPlotClasses({ | ||
acc, | ||
comparisonClassesSelected, | ||
comparisonData, | ||
id, | ||
path | ||
}) | ||
} | ||
} | ||
|
||
return acc | ||
} | ||
|
||
export type TemplateDetailsAccumulator = { | ||
[path: string]: { | ||
content: TopLevelSpec | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I based the output off of iterative/dvclive#766 (comment) description, but this will most likely change and we will need to adjust accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to
top
,bottom
,right
,left
based off the top-left corner after a discussion with Alex (see his comment):