-
Notifications
You must be signed in to change notification settings - Fork 1k
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
[Feature] Support RetrieverRecall metric & Add ArcFace config #1316
Conversation
Codecov ReportBase: 0.02% // Head: 86.83% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## dev-1.x #1316 +/- ##
============================================
+ Coverage 0.02% 86.83% +86.81%
============================================
Files 121 167 +46
Lines 8217 13606 +5389
Branches 1368 2170 +802
============================================
+ Hits 2 11815 +11813
+ Misses 8215 1426 -6789
- Partials 0 365 +365
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
return indices | ||
|
||
|
||
def _format_target(label, is_indices=False): |
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.
It seems unnecessary to have _format
for both pred and target, could these be merged?
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.
@Ezra-Yu What do you think?
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.
Engineering-wise, it is indeed possible to use a function. but one of them has a topk argument and need raise different errors(pred
, target
). I've tried this, but it's almost like merging the two functions together directly (there are a lot of if/else
statements), without reducing the amount of code. So I think it would be OK to keep them separate for the time being, as a private method, and have no qualms about rectifying them later.
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
Motivation
Related PR: #1019
Modification
Fix dataset.
Add arcface config.
Add RetrieverRecall metric.
Checklist
Before PR:
After PR: