-
Notifications
You must be signed in to change notification settings - Fork 66
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
feat(tuner): add miner v1 #180
Conversation
Codecov Report
@@ Coverage Diff @@
## main #180 +/- ##
==========================================
+ Coverage 86.45% 86.65% +0.19%
==========================================
Files 25 26 +1
Lines 1211 1236 +25
==========================================
+ Hits 1047 1071 +24
- Misses 164 165 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Co-authored-by: Tadej Svetina <tadej.svetina@jina.ai>
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 good, just one final comment
return [ | ||
(left[0], right[0], 1) if left[1] == right[1] else (left[0], right[0], -1) | ||
for left, right in combinations(enumerate(labels), 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.
Sorry to be nit-picky here, but can you break down this return statement? it's hard to read in current form
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.
formated by black, break down ci will fail
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.
What I mean is to re-format this, so first constrict the list, then return it. This inline if-else
should only be used if it can fit comfortably in one line
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.
the break-up version has been commented here: #180 (comment), won't apply
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.
No, the comment is irrelevant to this, please apply
this PR implemented:
rest to do: