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

Untokenized Bleu score to stay consistent with all the other text metrics #640

Merged
merged 21 commits into from
Dec 6, 2021

Conversation

mrleu
Copy link
Contributor

@mrleu mrleu commented Nov 25, 2021

What does this PR do?

Fixes #600

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?

Yep! My first time contributing to pytorch lightning. Very fun :) Please let me know if there's anything that needs to be updated!

Make sure you had fun coding 🙃

@codecov
Copy link

codecov bot commented Nov 25, 2021

Codecov Report

Merging #640 (22dd331) into master (eccfe83) will increase coverage by 0%.
The diff coverage is 92%.

@@          Coverage Diff          @@
##           master   #640   +/-   ##
=====================================
  Coverage      95%    95%           
=====================================
  Files         163    163           
  Lines        5994   5998    +4     
=====================================
+ Hits         5704   5710    +6     
+ Misses        290    288    -2     

@mrleu mrleu force-pushed the feature/600_bleu_consistency branch from ac138da to 07b25b0 Compare November 25, 2021 07:50
@SkafteNicki
Copy link
Member

@stancld please take a look :]

@SkafteNicki SkafteNicki added the enhancement New feature or request label Nov 25, 2021
@SkafteNicki SkafteNicki added this to the v0.7 milestone Nov 25, 2021
tests/text/test_bleu.py Outdated Show resolved Hide resolved
tests/text/test_bleu.py Outdated Show resolved Hide resolved
@mrleu mrleu marked this pull request as draft November 25, 2021 18:43
tests/text/test_bleu.py Outdated Show resolved Hide resolved
@mrleu mrleu requested a review from SkafteNicki November 25, 2021 19:01
@mrleu mrleu force-pushed the feature/600_bleu_consistency branch from 6227f1c to 11819a1 Compare November 25, 2021 20:01
@mergify mergify bot added the ready label Nov 30, 2021
@mrleu mrleu force-pushed the feature/600_bleu_consistency branch from c78c00e to 2f6f955 Compare November 30, 2021 22:01
@Borda
Copy link
Member

Borda commented Dec 3, 2021

@SkafteNicki ^^ 🐰

@mrleu
Copy link
Contributor Author

mrleu commented Dec 3, 2021

Ready for review 🙇🏻

@stancld
Copy link
Contributor

stancld commented Dec 3, 2021

Ready for review 🙇🏻

LGTM! Let's wait for the tests :]

@mrleu
Copy link
Contributor Author

mrleu commented Dec 3, 2021

Looks like it failed at https://github.com/PyTorchLightning/metrics/runs/4413710583?check_suite_focus=true#step:13:551, but that looks orthogonal towards this PR?

@Borda
Copy link
Member

Borda commented Dec 4, 2021

Looks like it failed at https://github.com/PyTorchLightning/metrics/runs/4413710583?check_suite_focus=true#step:13:551, but that looks orthogonal towards this PR?

yes seems we have some failures on master too... @SkafteNicki mind having a look?

@Borda Borda enabled auto-merge (squash) December 4, 2021 21:53
@Borda
Copy link
Member

Borda commented Dec 4, 2021

@SkafteNicki @justusschock ^^ 🐰

@mergify mergify bot removed the ready label Dec 4, 2021
@mergify mergify bot added the ready label Dec 6, 2021
@Borda Borda merged commit 8d5b8ba into Lightning-AI:master Dec 6, 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.

Inconsistent input for BLEUScore compared to other metrics
4 participants