Skip to content
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 #686

Closed
stancld opened this issue Dec 15, 2021 · 18 comments · Fixed by #696
Closed

Unify the input order for text (NLG) metrics #686

stancld opened this issue Dec 15, 2021 · 18 comments · Fixed by #696
Labels
API / design enhancement New feature or request Important milestonish
Milestone

Comments

@stancld
Copy link
Contributor

stancld commented Dec 15, 2021

🚀 Enhancement

Unify the input order for text (NLG) metrics

Motivation

We aim to unify text (NLG) metrics in a way that all of them will allow evaluating a hypothesis against multiple references. Nonetheless, there remains another distinction as some metrics presume preds, targets input order, while some expect targets, preds order. With respect to the whole TM ecosystem, we will go with preds, targets.

Pitch

Change input order for the following NLG metrics:

  • BLEUScore - B.C (add a warning suggested here)
  • CHRFScore - non B.C. (a new metrics in v0.7)
  • SacreBLEUScore - B.C (add a warning suggested here)
  • TER - non B.C. (a new metrics in v0.7)

Standardize naming convention to have: predictions and targets everywhere.

Alternatives

Keep it as it is.

@SkafteNicki @Borda @ashutoshml

@stancld stancld added the enhancement New feature or request label Dec 15, 2021
@Borda Borda added this to the v0.7 milestone Dec 16, 2021
@Borda
Copy link
Member

Borda commented Dec 16, 2021

I would try to make it for this 0.7

@SkafteNicki
Copy link
Member

If this is as simple as changing the order in a couple of metrics, then lets fit it into 0.7

@stancld
Copy link
Contributor Author

stancld commented Dec 17, 2021

Yes, I think it's a simple change that affects only ROUGEScore and BERTScore:]

@ashutoshml
Copy link
Contributor

@stancld Incase you aren't working on this, I can take it up.

@stancld
Copy link
Contributor Author

stancld commented Dec 18, 2021

@ashutoshml I've been finishing some other stuff, so it'd be great if you can have a look! O:]

@SkafteNicki
Copy link
Member

@stancld is there a reason why we should standardize to
target, preds
instead of
preds, target
as the remaining codebase?

@stancld
Copy link
Contributor Author

stancld commented Dec 18, 2021

@SkafteNicki I considered that mainly w.r.t. text metrics. Should we rather convert all NLP metrics to preds, target instead? I guess it makes perfect sense. (Sorry for the confusion :| )

@stancld
Copy link
Contributor Author

stancld commented Dec 18, 2021

And actually I see that in the TM version 0.6.x there were only BLEUScore/SacreBLEUScore which had target, preds pattern @SkafteNicki ... In that case, it makes really perfect sense to have everything as target, preds.

@ashutoshml
Copy link
Contributor

ashutoshml commented Dec 18, 2021

Also, naming standardization would help
targets or references?
preds or hypothesis?

I personally would prefer references and predictions.

@Borda
Copy link
Member

Borda commented Dec 18, 2021

I think that standardization would be needed, for example, if you have a collection and one of the metrics has its inputs in reverse order, could be bad...

@ashutoshml
Copy link
Contributor

In case we decide on the order targets, preds, the current PR is ready for review.
If preds, targets, we might need to change the requirements/description of the issue, and change all files, other than the ones I have currently changed.

@stancld
Copy link
Contributor Author

stancld commented Dec 22, 2021

@SkafteNicki So I guess we will go for preds, targets order for all metrics? :]
cc: @ashutoshml

@ashutoshml
Copy link
Contributor

Slightly confused.
What should be the final order then? targets, preds or preds, targets?

@stancld
Copy link
Contributor Author

stancld commented Dec 23, 2021

As also suggested in #687, we will go with preds, targets. I'll re-formulate the issue. Sorry for the confusion @ashutoshml :]

@ashutoshml
Copy link
Contributor

@stancld Thanks for updating the issue. I'll start working on it this weekend.

@ashutoshml
Copy link
Contributor

@stancld: Should I modify all the files in torchmetrics/text to have predictions and targets or only the ones you've mentioned in the description of this issue?

@mathemusician
Copy link
Contributor

@ashutoshml @stancld Now I'm really confused. I thought it was preds and targets, not predictions and targets?

@ashutoshml
Copy link
Contributor

@mathemusician Use predictions and targets as naming.
The earlier comments were primarily for the order of inputs by the metrics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API / design enhancement New feature or request Important milestonish
Projects
None yet
5 participants