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 normalizer, tokenizer to rouge #838

Merged
merged 19 commits into from
Feb 23, 2022
Merged

Conversation

hookSSi
Copy link
Contributor

@hookSSi hookSSi commented Feb 14, 2022

What does this PR do?

Fixes #825
I wrote some code but I don't know how to test it sorry.
plz review me hard

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 🙃

Copy link
Member

@Borda Borda left a comment

Choose a reason for hiding this comment

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

@stancld mind help with adding relevant tests? :)

torchmetrics/functional/text/rouge.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 14, 2022

Codecov Report

Merging #838 (a1116cb) into master (76646b6) will decrease coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #838   +/-   ##
=====================================
- Coverage      95%    95%   -0%     
=====================================
  Files         167    167           
  Lines        6904   6906    +2     
=====================================
- Hits         6562   6553    -9     
- Misses        342    353   +11     

@Borda Borda requested review from Borda and stancld February 16, 2022 19:13
@Borda
Copy link
Member

Borda commented Feb 16, 2022

@stancld how is it going here? :)

Copy link
Contributor

@stancld stancld left a comment

Choose a reason for hiding this comment

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

@hookSSi Thanks a lot for your contribution. I left a few minor comments but, in general, I think the PR looks quite good.
Regarding the tests.. I think we can just test default behaviour against the scenario we input the default tokenizer and normalizer manually into the module/functional metric and just verify everything goes through without problems. :]

tm_examples/rouge_score-own_normalizer_and_tokenizer.py Outdated Show resolved Hide resolved
tm_examples/rouge_score-own_normalizer_and_tokenizer.py Outdated Show resolved Hide resolved
tm_examples/rouge_score-own_normalizer_and_tokenizer.py Outdated Show resolved Hide resolved
tm_examples/rouge_score-own_normalizer_and_tokenizer.py Outdated Show resolved Hide resolved
tm_examples/rouge_score-own_normalizer_and_tokenizer.py Outdated Show resolved Hide resolved
tm_examples/rouge_score-own_normalizer_and_tokenizer.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/rouge.py Outdated Show resolved Hide resolved
tm_examples/rouge_score-own_normalizer_and_tokenizer.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/rouge.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/rouge.py Outdated Show resolved Hide resolved
@Borda
Copy link
Member

Borda commented Feb 21, 2022

@hookSSi how is it going here? have you had a change to check recent review/suggestion from @stancld 🐰

@hookSSi
Copy link
Contributor Author

hookSSi commented Feb 22, 2022

@hookSSi how is it going here? have you had a change to check recent review/suggestion from @stancld 🐰

Sorry, I'm checking on it now

@stancld
Copy link
Contributor

stancld commented Feb 22, 2022

@hookSSi how is it going here? have you had a change to check recent review/suggestion from @stancld 🐰

Sorry, I'm checking on it now

Thanks for the update (I'll go through the code once again today). May you just add some tests suggested in the code review comment?

@hookSSi
Copy link
Contributor Author

hookSSi commented Feb 23, 2022

Thanks for the update (I'll go through the code once again today). May you just add some tests suggested in the code review comment?

I added test that just checking value. Have a look and review me please.

@Borda Borda requested a review from stancld February 23, 2022 08:20
@Borda
Copy link
Member

Borda commented Feb 23, 2022

@hookSSi could you please mark addressed comments as resolved so we easily see what has been left? 🐰

@hookSSi
Copy link
Contributor Author

hookSSi commented Feb 23, 2022

@hookSSi could you please mark addressed comments as resolved so we easily see what has been left? 🐰

Oh I didn't know that I can mark these sorry about that.

tests/text/test_rouge.py Outdated Show resolved Hide resolved
tm_examples/rouge_score-own_normalizer_and_tokenizer.py Outdated Show resolved Hide resolved
tm_examples/rouge_score-own_normalizer_and_tokenizer.py Outdated Show resolved Hide resolved
tm_examples/rouge_score-own_normalizer_and_tokenizer.py Outdated Show resolved Hide resolved
tm_examples/rouge_score-own_normalizer_and_tokenizer.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/rouge.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/rouge.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/rouge.py Outdated Show resolved Hide resolved
torchmetrics/text/rouge.py Outdated Show resolved Hide resolved
@mergify mergify bot added the ready label Feb 23, 2022
Copy link
Contributor

@stancld stancld left a comment

Choose a reason for hiding this comment

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

Nitpicks

tm_examples/rouge_score-own_normalizer_and_tokenizer.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/rouge.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/rouge.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/rouge.py Outdated Show resolved Hide resolved
torchmetrics/text/rouge.py Outdated Show resolved Hide resolved
@stancld stancld merged commit dbbce0f into Lightning-AI:master Feb 23, 2022
@Borda Borda added this to the v0.8 milestone May 5, 2022
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.

Can add additional parameters in calculating scores like rouge?
3 participants