-
Notifications
You must be signed in to change notification settings - Fork 391
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
Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF #696
Conversation
Codecov Report
@@ Coverage Diff @@
## master #696 +/- ##
=====================================
Coverage 95% 95%
=====================================
Files 166 166
Lines 6413 6421 +8
=====================================
+ Hits 6105 6114 +9
+ Misses 308 307 -1 |
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.
lets use everywhere preds
and targets
@ashutoshml how is it going here, what is missing? |
@Borda I think we were not fully sure with the naming. But @ashutoshml, it's |
Let's keep it moving, can we limit this PR just to switching the order as it is stated in the PR title and do the rename package-wide in another PR as it will require another deprecation/compatibility edits 🐰 @stancld @ashutoshml |
If the naming convention is not an issue, then the PR is ready for review. The order issue has been fixed. |
yes, but in such a case pls ever all name changes or you need to also include all deprecation for argument's name changes... :] |
I see. I'll revert all naming to previous ones then, and only modify the order. I will make the changes latest by tomorrow. |
cool, @ashutoshml do you want to prepare the next one with rename arguments to be TM-wide consistent or @stancld? |
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! Nice work, thanks a lot @ashutoshml
That might need changing all the files. Would it be ok to split the metrics into multiple PRs? 4-4-4 or 6-6 since there are 12 text metrics. |
sure, that sounds good to me... if you split it 6-6 you can also divide work so 6 for @ashutoshml and 6 for @stancld? 🐰 |
@ashutoshml Just let me know which metrics you are taking and I'll handle the remaining ones :] |
Those failing tests seem to be unrelated to this PR:
Can it be merged then? :] |
do they fail also on master? :] |
With the most recent commit #705, these tests are currently failing on the |
@stancld I'll take |
What does this PR do?
Fixes #686
Before submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃
Yes.