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

Expose evaluations and metrics #319

Merged
merged 11 commits into from
Feb 25, 2025
Merged

Expose evaluations and metrics #319

merged 11 commits into from
Feb 25, 2025

Conversation

brandon-groundlight
Copy link
Collaborator

Makes metrics and evaluations available

Copy link
Contributor

@CoreyEWood CoreyEWood left a comment

Choose a reason for hiding this comment

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

Left some comments about docstrings/return types but functionality-wise this seems good.

Comment on lines 1003 to 1010
"""
Get a specific evaluation for a detector

:param detector: the detector to get the evaluation for
:param evaluation_id: the evaluation id to get

:return: The DetectorEvaluation object
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the docstring here is wrong - it doesn't take an evaluation_id. It also returns a dict, should it be returning a DetectorEvaluation object?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for checking the docstrings, I left an older version in after I updated the function. Returning a dict is a little bit of a break from our usual format but this exposes the feature until we better formalize what should be in the evaluation.


:param detector: the detector to get the metrics for

:return: The DetectorMetrics object
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here about the return type.

assert metrics["summary"]["unconfident_counts"] is not None
assert metrics["summary"]["total_iqs"] is not None

# NOTE: this implicitly tests how quickly the evaluation is made available
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any worry that this test will be flaky due to evaluation sometimes taking too long?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

possibly, it's something I've got my eye one. If it gets in the way of the CICD pipeline I'll raise the amount of time we give to get an evaluation. But tests were passing on my local tests for the time being

@brandon-groundlight brandon-groundlight merged commit 401bd14 into main Feb 25, 2025
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.

2 participants