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 support for rgb overlays #2483
Conversation
Codecov ReportBase: 48.18% // Head: 61.73% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #2483 +/- ##
============================================
+ Coverage 48.18% 61.73% +13.55%
============================================
Files 186 207 +21
Lines 29099 39884 +10785
Branches 240 242 +2
============================================
+ Hits 14020 24623 +10603
- Misses 15079 15261 +182
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
3df4a7c
to
6c1f6e2
Compare
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.
@sashankaryal how's this PR coming? A couple things I noticed when testing just now:
- The tooltip does not render correctly when viewing a dataset without mask targets explicitly defined in the App (does not consistency appear; and when it does seems to be showing the wrong data?)
I also noticed that pixel values that do not appear in the provided mask_targets
are currently being rendered. Is that intentional? Or are you planning to switch to the currently documented behavior that pixels not in masks_targets
are not rendered? After testing just now and experiencing the current behavior, I'm thinking it could be fine to go ahead and always render all non-zero classes and just show pixel
value when a label
is not available.
I can see value both ways:
- If only
mask_targets
are painted, it would give a simple way to change what is rendered by editingmask_targets
- If all non-zero values are painted, then there is never any confusion why user's data is not appearing
3b07f77
to
588c93f
Compare
Addressed in the new commits |
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.
Generally the code looks good. Nice job!
throw new Error("mask targets is invalid"); | ||
} | ||
|
||
return Object.keys(maskTargets)[0]?.startsWith("#") === true; |
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.
This will run quite often, could we cache this somehow?
app/packages/looker/src/worker.ts
Outdated
DETECTIONS, | ||
]); | ||
// defined as labels that can have on-disk overlays | ||
const DENSE_LABELS = new Set([SEGMENTATION, HEATMAP, DETECTION, DETECTIONS]); |
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.
Maybe define in @fiftyone/utilities
for reuse?
app/packages/looker/src/worker.ts
Outdated
const r = targets[i * mapData.channels]; | ||
const g = targets[i * mapData.channels + 1]; | ||
const b = targets[i * mapData.channels + 2]; |
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.
There is a fair bit of shared complexity in theses functions, like these for loops. You might be able to share some code to improve readability
|
||
if (!isRgbMaskTargets(maskTargets)) { | ||
// getting color here is computationally inefficient, return no color for this edge case | ||
return rgbSegmentationInfoWithoutColor; |
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.
Can you elaborate here? I'm not sure I understand
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.
This is for the edge case when the mask is multi-channel (RGB) but the mask targets is not RGB (or is missing). In this scenario, we render the RGB mask as is but the tooltip will behave slightly differently in two respects:
- The ribbon color won't reflect the segmentation color, it'll be static in the entire canvas (a shade of white, I think).
- We won't display any "pixel" info.
This comment is re: (1); while it's possible to get the color of the underlying mask, it involves parsing the RGB buffer. In the happy path case, the RGB buffer is reduced into a targets lookup cable and we don't have this problem.
588c93f
to
8130059
Compare
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.
LGTM
Resolves #1730.
Change log
evaluate_segmentations()
objects_to_segmentations()
transform_segmentations()
utility to transform existing masks between grayscale <> RGBExample usage