Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF #696

ashutoshml · 2021-12-28T08:32:20Z

What does this PR do?

Fixes #686

Before submitting

Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
Did you read the contributor guideline, Pull Request section?
Did you make sure to update the docs?
Did you write any new necessary tests?

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.

codecov · 2021-12-28T08:36:22Z

Codecov Report

Merging #696 (1711a17) into master (df4d3c8) will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #696   +/-   ##
=====================================
  Coverage      95%    95%           
=====================================
  Files         166    166           
  Lines        6413   6421    +8     
=====================================
+ Hits         6105   6114    +9     
+ Misses        308    307    -1

Borda

lets use everywhere preds and targets

tests/text/test_bleu.py

Borda · 2022-01-03T08:12:59Z

@ashutoshml how is it going here, what is missing?
@stancld could you assist to get it to merge quickly...

stancld · 2022-01-03T08:21:49Z

@ashutoshml how is it going here, what is missing? @stancld could you assist to get it to merge quickly...

@Borda I think we were not fully sure with the naming. But @ashutoshml, it's preds, target everywhere for sure, so ping me whenever this PR is ready for review :] Sorry for some confusion

Borda · 2022-01-03T08:37:53Z

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
cc: @justusschock @SkafteNicki

ashutoshml · 2022-01-03T09:13:22Z

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 cc: @justusschock @SkafteNicki

If the naming convention is not an issue, then the PR is ready for review. The order issue has been fixed.

Borda · 2022-01-03T09:39:35Z

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... :]

ashutoshml · 2022-01-03T10:03:50Z

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.

…standardization

ashutoshml · 2022-01-03T14:18:37Z

@Borda @stancld Please have a look

torchmetrics/functional/text/bleu.py

torchmetrics/functional/text/sacre_bleu.py

torchmetrics/text/bleu.py

Borda · 2022-01-03T14:56:40Z

@Borda @stancld Please have a look

cool, @ashutoshml do you want to prepare the next one with rename arguments to be TM-wide consistent or @stancld?

stancld

LGTM! Nice work, thanks a lot @ashutoshml

ashutoshml · 2022-01-03T16:32:28Z

@Borda @stancld Please have a look

cool, @ashutoshml do you want to prepare the next one with rename arguments to be TM-wide consistent or @stancld?

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.

Borda · 2022-01-03T16:40:17Z

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? 🐰

stancld · 2022-01-03T19:58:21Z

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 :]

stancld · 2022-01-03T20:21:45Z

@Borda @SkafteNicki

Those failing tests seem to be unrelated to this PR:

ERROR torchmetrics/audio/pesq.py - ValueError: numpy.ndarray size changed, ma...
ERROR torchmetrics/functional/audio/pesq.py - ValueError: numpy.ndarray size ...
ERROR tests/audio/test_pesq.py - ValueError: numpy.ndarray size changed, may ...
ERROR tests/audio/test_pesq.py - ValueError: numpy.ndarray size changed, may ...

Can it be merged then? :]

Borda · 2022-01-03T20:36:59Z

Those failing tests seem to be unrelated to this PR:

do they fail also on master? :]

stancld · 2022-01-03T20:42:56Z

Those failing tests seem to be unrelated to this PR:

do they fail also on master? :]

With the most recent commit #705, these tests are currently failing on the master too. I can have a look at them

ashutoshml · 2022-01-04T02:05:10Z

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 :]

@stancld I'll take cer, ter, wer, sacrebleu, rouge, squad. Kindly let me know when you create an issue and assign it to me.

ashutoshml added 4 commits December 26, 2021 21:53

Stage 1: Standardize BLEU and CHRF

766d8fb

Standardize TER metric

1e13521

Standardize SacreBLEU

18e76e9

Add warnings for breaking change files

b057fe0

ashutoshml changed the title ~~Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF~~ [WIP] Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF Dec 28, 2021

ashutoshml added 2 commits December 28, 2021 18:11

Update docstrings

fc482a7

Update docstring + CHANGELOG.md

0a48f77

ashutoshml marked this pull request as ready for review December 28, 2021 14:57

ashutoshml requested review from ananyahjha93, Borda, ethanwharris, justusschock, SeanNaren, SkafteNicki and tchaton as code owners December 28, 2021 14:57

ashutoshml changed the title ~~[WIP] Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF~~ Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF Dec 28, 2021

ashutoshml marked this pull request as draft December 28, 2021 17:51

ashutoshml changed the title ~~Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF~~ [WIP] Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF Dec 28, 2021

Borda added API / design enhancement New feature or request labels Dec 28, 2021

Borda added this to the v0.7 milestone Dec 28, 2021

Borda reviewed Dec 28, 2021

View reviewed changes

tests/text/test_bleu.py Outdated Show resolved Hide resolved

tests/text/test_bleu.py Outdated Show resolved Hide resolved

Merge branch 'master' into standardization

1514d86

ashutoshml marked this pull request as ready for review January 3, 2022 09:14

ashutoshml changed the title ~~[WIP] Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF~~ Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF Jan 3, 2022

ashutoshml marked this pull request as draft January 3, 2022 10:04

ashutoshml changed the title ~~Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF~~ [WIP] Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF Jan 3, 2022

ashutoshml added 2 commits January 3, 2022 19:15

Update order and keep naming unchanged

9b6a541

Merge branch 'standardization' of github.com:ashutoshml/metrics into …

7ed5188

…standardization

ashutoshml changed the title ~~[WIP] Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF~~ Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF Jan 3, 2022

ashutoshml marked this pull request as ready for review January 3, 2022 14:03

Borda approved these changes Jan 3, 2022

View reviewed changes

torchmetrics/functional/text/bleu.py Outdated Show resolved Hide resolved

torchmetrics/functional/text/sacre_bleu.py Outdated Show resolved Hide resolved

torchmetrics/text/bleu.py Outdated Show resolved Hide resolved

Apply suggestions from code review

2f5e461

Borda requested a review from stancld January 3, 2022 14:54

Borda enabled auto-merge (squash) January 3, 2022 14:55

stancld approved these changes Jan 3, 2022

View reviewed changes

stancld mentioned this pull request Jan 3, 2022

Clean text tests - remove input_order argument #704

Closed

mergify bot added the ready label Jan 3, 2022

Merge branch 'master' into standardization

1711a17

awaelchli approved these changes Jan 4, 2022

View reviewed changes

Borda disabled auto-merge January 4, 2022 19:16

Borda merged commit a66ee8a into Lightning-AI:master Jan 4, 2022

ashutoshml deleted the standardization branch January 4, 2022 22:36

jscottcronin mentioned this pull request Jan 5, 2022

Removed input_order from text unit tests #717

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF #696

Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF #696

ashutoshml commented Dec 28, 2021 •

edited

Loading

codecov bot commented Dec 28, 2021 •

edited

Loading

Borda left a comment

Borda commented Jan 3, 2022

stancld commented Jan 3, 2022

Borda commented Jan 3, 2022 •

edited

Loading

ashutoshml commented Jan 3, 2022

Borda commented Jan 3, 2022

ashutoshml commented Jan 3, 2022

ashutoshml commented Jan 3, 2022

Borda commented Jan 3, 2022

stancld left a comment

ashutoshml commented Jan 3, 2022

Borda commented Jan 3, 2022 •

edited

Loading

stancld commented Jan 3, 2022

stancld commented Jan 3, 2022 •

edited by Borda

Loading

Borda commented Jan 3, 2022

stancld commented Jan 3, 2022

ashutoshml commented Jan 4, 2022

Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF #696

Unify the input order for text (NLG) metrics - BLEU, SacreBLEU, TER, CHRF #696

Conversation

ashutoshml commented Dec 28, 2021 • edited Loading

What does this PR do?

Before submitting

PR review

Did you have fun?

codecov bot commented Dec 28, 2021 • edited Loading

Codecov Report

Borda left a comment

Choose a reason for hiding this comment

Borda commented Jan 3, 2022

stancld commented Jan 3, 2022

Borda commented Jan 3, 2022 • edited Loading

ashutoshml commented Jan 3, 2022

Borda commented Jan 3, 2022

ashutoshml commented Jan 3, 2022

ashutoshml commented Jan 3, 2022

Borda commented Jan 3, 2022

stancld left a comment

Choose a reason for hiding this comment

ashutoshml commented Jan 3, 2022

Borda commented Jan 3, 2022 • edited Loading

stancld commented Jan 3, 2022

stancld commented Jan 3, 2022 • edited by Borda Loading

Borda commented Jan 3, 2022

stancld commented Jan 3, 2022

ashutoshml commented Jan 4, 2022

ashutoshml commented Dec 28, 2021 •

edited

Loading

codecov bot commented Dec 28, 2021 •

edited

Loading

Borda commented Jan 3, 2022 •

edited

Loading

Borda commented Jan 3, 2022 •

edited

Loading

stancld commented Jan 3, 2022 •

edited by Borda

Loading