-
Notifications
You must be signed in to change notification settings - Fork 73
Kid metric added #435
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
Kid metric added #435
Conversation
begumcig
left a comment
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 so much for implementing KID, Minette, it already looks amazing! 💜 I just left one small comment: I think we could handle the special case in compute() a bit more cleanly (maybe by adding a kid_compute to the TorchMetrics enum?), so that the compute() function stays as generic as possible. Other than that, it’s already looking great and pretty much ready to go! 💜💜
| result = self.metric.compute() | ||
|
|
||
| # Handle KID which returns a tuple (mean, std) | ||
| if self.metric_name == "kid" and isinstance(result, tuple) and len(result) == 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.
I don't really have a suggestion for this yet but I think we should not do conditional statements in the compute function based on the metric name, as this can get messy quite easily, how do you feel?
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.
Very good point, and I agree. Having conditionals in compute() based on the metric name can get messy fast. So I cleaned it up a bit:
- Took out the if
self.metric_name == "kid"check fromcompute(). - Added a
kid_computefunction to handle KID’s tuple return. - Updated the TorchMetrics enum so KID has a 4th element for
kid_compute(others stay as 3). - Now
compute()just usesself.compute_fnfrom the enum if it’s there, otherwise falls back to the defaultself.metric.compute().
So now the compute() method is generic and clean, no metric-specific conditionals. Each metric can have its own compute logic in the enum, just like how we handle the update functions.
What do you think, does this look like a good direction?
sharpenb
left a comment
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 looks good! Left a single comment
|
This PR has been inactive for 10 days and is now marked as stale. |
begumcig
left a comment
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.
Looks super good Minette! I made a small comment about the blank line but feel free to merge! Amazing job you are a queen 👸
|
This PR has been inactive for 10 days and is now marked as stale. |
2620cea to
2acf02e
Compare
Description
Added KID (Kernel Inception Distance) metric
Related Issue
Fixes #(issue number)
Type of Change
How Has This Been Tested?
The KID metric implementation has been tested with the following test suite:
Main KID Test (
test_kid):subset_sizeparameter (must be smaller than number of samples)test_kid[LAION256-cpu]andtest_kid[LAION256-cuda]- both PASSEDCall Type Tests (
test_check_call_type):gt_yfor single,pairwise_gt_yfor pairwise)test_check_call_type[single-kid]andtest_check_call_type[pairwise-kid]- both PASSEDAll Torch Metrics Tests:
uv run pytest tests/evaluation/test_torch_metrics.py -vChecklist
Additional Notes