voxel51 / voxelgpt

AI assistant that can query visual datasets, search the FiftyOne docs, and answer general computer vision questions
https://gpt.fiftyone.ai
Apache License 2.0
234 stars 17 forks source link

Allen non-null conf fix #74

Closed jacobmarks closed 1 year ago

jacobmarks commented 1 year ago

Merges in @allenleetc's fix in #52.

brimoor commented 1 year ago

@jacobmarks @allenleetc hmm actually I should have looked at the code before approving this. I don't really understand why limit(1).values() is being used. That's not a very robust test of whether confidences exist or not. Maybe they only don't exist for that sample.

If we really need this kind of logic, checking whether sample_collection.bounds("path.to.confidence") != [None, None] would be more robust.

jacobmarks commented 1 year ago

@brimoor that makes sense. In that case, let's change it to your suggestion. Thanks for the thorough look

allenleetc commented 1 year ago

@brimoor @jacobmarks Thanks all, yeah definitely no deep thought here, was just wanting to get the fully-qualified name to work. Using a viewstage (or aggregation I guess) at all seemed too heavy but I didn't know how else besides values to index by a fully qualified name. I guess with a robust check it is a good thing though