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

Fix typo in rouge types #364

Merged
merged 5 commits into from
Nov 25, 2022
Merged

Fix typo in rouge types #364

merged 5 commits into from
Nov 25, 2022

Conversation

davebulaval
Copy link
Contributor

@davebulaval davebulaval commented Nov 17, 2022

Minor typo fix in the code and doc example that does not align with the writing style (rougeLsum -> rougeLSum).

@lvwerra
Copy link
Member

lvwerra commented Nov 25, 2022

The name of the variable inside rouge_score uses a lower-case s (see here):

elif rouge_type == "rougeLsum":

So I think we should leave it as it was. Why did you want to change it?

@davebulaval
Copy link
Contributor Author

davebulaval commented Nov 25, 2022

The doc state otherwise. Whatever name you want to use, it is just not uniformized.

"rougeLSum"`: rougeLSum splits text using `"\n"`.

@lvwerra
Copy link
Member

lvwerra commented Nov 25, 2022

Ok I see, then let's go with the lower-case version since that's follows the code.

metrics/rouge/rouge.py Outdated Show resolved Hide resolved
metrics/rouge/rouge.py Outdated Show resolved Hide resolved
metrics/rouge/rouge.py Outdated Show resolved Hide resolved
metrics/rouge/rouge.py Outdated Show resolved Hide resolved
@davebulaval
Copy link
Contributor Author

Updated.

P.S. I feel like rouseLsum is an odd writing style (rougeL but you add another word, and now is it rougeLsum (same comments for variable names rouge_Lsum but also rouge_L)), but hey it is your code base.

Copy link
Member

@lvwerra lvwerra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks good! Note that this is not our convention but the follows the underlying library (rouge_score from Google Research).

@lvwerra lvwerra merged commit 57fe685 into huggingface:main Nov 25, 2022
awinml added a commit to awinml/evaluate that referenced this pull request Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants