Fix image by step plots with bounding boxes #5313
Conversation
julieg18
changed the base branch from
add-bb-backend-logic
to
improve-bounding-box-frontend
| @@ -94,7 +94,9 @@ export type ComparisonPlotClass = { | |||
| } | |||
|
|
|||
| export type ComparisonPlotClasses = { | |||
| [revision: string]: { [path: string]: ComparisonPlotClass[] } | |||
| [revision: string]: { | |||
| [path: string]: { [imgInd: number]: ComparisonPlotClass[] } | |||
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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[][] }
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 😅.
| const mockBoxes = { | ||
| car: [{ box: { bottom: 0, left: 0, right: 0, top: 0 }, score: 0.5 }] | ||
| } | ||
| const imgDataWithBoundingBoxes = plotsDiffFixture.data[ |
There was a problem hiding this comment.
Another option is adding a new image by step plot with bounding boxes plot to our base comparison fixture.
| @@ -94,7 +94,9 @@ export type ComparisonPlotClass = { | |||
| } | |||
|
|
|||
| export type ComparisonPlotClasses = { | |||
| [revision: string]: { [path: string]: ComparisonPlotClass[] } | |||
| [revision: string]: { | |||
| [path: string]: { [imgInd: number]: ComparisonPlotClass[] } | |||
There was a problem hiding this comment.
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[][] }
main<- #5227 <- #5241 <- #5250 <- #5305 <- thisFixes a bug with bounding boxes not being shown in every image in multi img plots.