-
Notifications
You must be signed in to change notification settings - Fork 557
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
bugfix: similarity search only works on default_group_slice #3912
bugfix: similarity search only works on default_group_slice #3912
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release/v0.23.2 #3912 +/- ##
==================================================
Coverage ? 15.86%
==================================================
Files ? 731
Lines ? 81757
Branches ? 1093
==================================================
Hits ? 12974
Misses ? 68783
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
42cf587
to
d6b0f93
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 🚀
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.
@lanzhenw this is great work! 🥇
I see three issues here and I think you have fixed the main one but two pending.
- when a current active slice doesn't have a similarity computed, we still show the "Type anything" input. This causes confusion as the user can type "car" for example and the loading spinner will spin forever and a network error occurs without being surfaced to the client. The reason is that current slice is "left" for example but only "right" slice has similarity computed.
I think I know a fix, will push something for it.
- If you compute similarity for "right slice". then search for "car". then switch back to left slice, app crashes.
@lanzhenw Can you look into this when you get a chance. end of video shows the crash
Video is app after your fix and my fix (1) above
Screen.Recording.2023-12-12.at.4.56.31.PM.mov
…ll slices have similarity computed
let methods = get(datasetAtom)?.brainMethods || []; | ||
const isGroupDataset = get(isGroup); | ||
const activeSlice = get(currentSlice(get(isModalActive))); | ||
|
||
if (isGroupDataset && activeSlice) { | ||
methods = methods.filter(({ viewStages }) => { | ||
return viewStages.some((vs) => { | ||
const { _cls, kwargs } = JSON.parse(vs); | ||
if (_cls === "fiftyone.core.stages.SelectGroupSlices") { | ||
const sliceValue = kwargs.filter( | ||
(kwarg: string[]) => kwarg[0] === "slices" | ||
)?.[0]?.[1]; | ||
if (sliceValue && sliceValue !== activeSlice) return false; | ||
} | ||
return 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.
@benjaminpkane @lanzhenw added this filters out a method if the activeSlice is not the one one with the similarity computed.
Shows help message for the active slice without similarity computed
Screen.Recording.2023-12-12.at.4.56.31.PM.mov
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.
Thank you! 🍨
What changes are proposed in this pull request?
Pass in the active session group slice as a parameter for similarity sort requests to update samples
Fixes a bug: similarity search only works on default_group_slice
similarity.mov
How is this patch tested? If it is not, please explain why.
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyone
Python library changes