Skip to content
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

Utilize source field for confidence and answered #264

Merged
merged 8 commits into from
Oct 14, 2024

Conversation

brandon-groundlight
Copy link
Collaborator

@brandon-groundlight brandon-groundlight commented Oct 11, 2024

There's a bug currently for counting + multiclass where confidence can be below 0.5, which throws off previous assumptions.

@timmarkhuff
Copy link
Contributor

timmarkhuff commented Oct 11, 2024

Looks good to me. Should we have a test for timing? For example, run ask_ml and make sure it completes in some reasonable time frame. I don't know what the time frame should be. Probably something between 1 second and our default patience time of 30 seconds.

The test you added ensures that the query is answered by the time the function call completes, but I don't see anything checking if the function call returned in a reasonable amount of time and didn't just wait the full patience time.

@@ -64,21 +64,16 @@ def iq_is_confident(iq: ImageQuery, confidence_threshold: float) -> bool:
The only subtletie here is that currently confidence of None means
human label, which is treated as confident.
"""
if iq.result.confidence is None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not related to your PR but could you change subtletie to subtlety above?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A subtle change

Copy link
Member

@tyler-romero tyler-romero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, LGTM

@brandon-groundlight
Copy link
Collaborator Author

brandon-groundlight commented Oct 11, 2024

Should we have a test for timing?

We've considered this before, but we've decided that such tests should live in our other testing code, and not inside the SDK tests.

@brandon-groundlight brandon-groundlight merged commit d9a20eb into main Oct 14, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants