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

Add torchmetrics' own implementation of Rouge score metrics #443

Merged
merged 43 commits into from
Aug 17, 2021

Conversation

stancld
Copy link
Contributor

@stancld stancld commented Aug 12, 2021

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?

What does this PR do?

This PR is a part of the endeavour of #433 to implement own metrics and minimize third-party dependencies. It adds the own implementation of Rouge score metrics.

In this PR, the rouge-score package (https://pypi.org/project/rouge-score/) is replaced with torchmetrics' own implementation. The behaviour of this new implementation should be equivalent tothe aforementioned package.

Furthermore, this PR strips the use of the decimal_places argument as this seems not to be used by any other metric. TODO: Add some depreciation warning (I will need a help here as I haven't found any own Warning class as it is the case in the pytorch-lightning.

Future TODOs for another PR: Replace nltk package (I will need to dig a bit deeper to find out if there's a way)

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.

Reviewers: @Borda @SkafteNicki

Did you have fun?

Make sure you had fun coding 🙃

@pep8speaks
Copy link

pep8speaks commented Aug 12, 2021

Hello @stancld! Thanks for updating this PR.

Line 308:38: E203 whitespace before ':'

Comment last updated at 2021-08-13 12:11:13 UTC

@Borda Borda added the enhancement New feature or request label Aug 12, 2021
@codecov
Copy link

codecov bot commented Aug 12, 2021

Codecov Report

Merging #443 (dcc2898) into master (9df5f88) will increase coverage by 0.15%.
The diff coverage is 60.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #443      +/-   ##
==========================================
+ Coverage   74.79%   74.94%   +0.15%     
==========================================
  Files         129      129              
  Lines        4178     4227      +49     
==========================================
+ Hits         3125     3168      +43     
- Misses       1053     1059       +6     
Flag Coverage Δ
Linux 74.94% <60.39%> (+0.15%) ⬆️
Windows 74.94% <60.39%> (+0.15%) ⬆️
cpu 74.94% <60.39%> (+0.15%) ⬆️
macOS 74.94% <60.39%> (+0.15%) ⬆️
pytest 74.94% <60.39%> (+0.15%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
torchmetrics/text/rouge.py 26.19% <13.33%> (-5.52%) ⬇️
torchmetrics/functional/text/rouge.py 63.63% <68.60%> (+28.34%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9df5f88...dcc2898. Read the comment docs.

@stancld stancld marked this pull request as ready for review August 12, 2021 12:35
@Borda Borda mentioned this pull request Aug 15, 2021
4 tasks
torchmetrics/functional/text/rouge.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/rouge.py Outdated Show resolved Hide resolved
@mergify mergify bot added the ready label Aug 15, 2021
Copy link
Member

@SkafteNicki SkafteNicki left a comment

Choose a reason for hiding this comment

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

Looks great :]
Remember to add note in changelog

torchmetrics/text/rouge.py Outdated Show resolved Hide resolved
torchmetrics/text/rouge.py Outdated Show resolved Hide resolved
@Borda Borda requested a review from SkafteNicki August 16, 2021 10:13
@Borda Borda enabled auto-merge (squash) August 16, 2021 10:15
@mergify mergify bot removed the ready label Aug 16, 2021
@SkafteNicki SkafteNicki modified the milestones: v0.5, v0.6 Aug 16, 2021
@mergify mergify bot added the ready label Aug 16, 2021
@mergify mergify bot requested a review from ethanwharris as a code owner August 17, 2021 09:46
@Borda Borda merged commit a2712eb into Lightning-AI:master Aug 17, 2021
@Borda Borda modified the milestones: v0.6, v0.5 Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants