metrics: add BLEU

[ydcjeff]

What does this PR do?

Fixes #1301 (issue)

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 your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
• Did you make sure to update the documentation with your changes?
• Did you write any new necessary tests?
• Did you verify new and existing tests pass locally with your changes?
• If you made a notable change (that affects users), did you update the CHANGELOG?

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 🙃

[pep8speaks]

Hello @ydcjeff! Thanks for updating this PR.

• In the file pytorch_lightning/metrics/functional/nlp.py:

Line 27:49: E203 whitespace before ':'

• In the file tests/metrics/test_nlp.py:

Line 22:98: E231 missing whitespace after ','

Comment last updated at 2020-07-20 15:03:11 UTC

[ydcjeff]

Hello @justusschock,
I have opened the PR. Also, where should I put my code under metrics package as I wrote it in a separate file?

[codecov]

Codecov Report

Merging #2535 into master will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #2535   +/-   ##
======================================
  Coverage      91%     91%           
======================================
  Files          70      72    +2     
  Lines        5778    5831   +53     
======================================
+ Hits         5270    5323   +53     
  Misses        508     508           

[SkafteNicki]

Since bleu is a metric specific to nlp, could you move your code to a file called nlp.py. I guess we down the line will get more field-specific metrics.
Additionally,

• add link to metric in this file https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pytorch_lightning/metrics/__init__.py
• add reference in the docs, in this file https://github.com/PyTorchLightning/pytorch-lightning/blob/master/docs/source/metrics.rst

[Borda]

is there another standard implementation so we can compare our results with theirs in tests?

[justusschock]

I requested some changes. It is basically all your math, that should use torch operations instead of math package ops.

Your current implementation should go under metrics/functional/sequence.py

Once we finished iterating over the functional interface, we also need to add a module interface.

@williamFalcon is there a way to directly calculate these on tensors? if you have to convert it back to strings first, we always have a GPU sync, which we want to avoid.

[justusschock]

can you remove these lines?

[ydcjeff]

I have refactored with torch.Tensor, added smooth argument and tested with nltk.

And also I added nltk in test.txt for testing.

[Borda]

rather use torch.allclose(...)

[Borda]

pls see my comments, but we are on a good way...

[Borda]

this circleci test failed as there is only installing from base.txt and test.txt while torchtext is in extra.txt

I thought that we are renaming this sequence module to nlp? @justusschock
you shall add this module do ignore for Conda and CIrcleCI tests :]

[ydcjeff]

I thought that we are renaming this sequence module to nlp? @justusschock

so nlp or sequence?

[ydcjeff]

you shall add this module do ignore for Conda and CIrcleCI tests :]

how shall I add?

[williamFalcon]

this should be:

from p...metrics.nlp import Bleu

[ydcjeff]

this should be:

from p...metrics.nlp import Bleu

Okay

[ydcjeff]

I guess this is ready to review/go. @williamFalcon @Borda

[williamFalcon]

ummm. so this is a wrapper on torchtext? i think we need our own implementation

[ydcjeff]

Ahhh, I have implemented from scratch before and referenced the implmentation a little from torchtext.

Then, @justusschock sugguested to base on torchtext since it is in pytorch ecosystem.

So I refactored with torchtext.
@williamFalcon

[justusschock]

@williamFalcon : @Borda and me agreed, that we don't need to duplicate this if it is already present within torchtext since basically everyone that will use this will also have torchtext installed and it was already an optional dependency.

[williamFalcon]

i guess the point of metrics here is to centralize all metrics. otherwise we coule have said the same about sklearn.

we want our metrics package to be the reference implementation for any metric.

So, i would say, implement it here from scratch and test against torchtext for performance? the reason is that we want to give our community the flexibility to modify it as best practices change and i know bleu is one of those hotly debated metrics in terms of implementation details

[justusschock]

With sklearn it's more about GPU performance/syncs :)

But I see your point. Then I'm sorry @ydcjeff :D But your code should still be available here :) SO just copy/paste it in :D

[ydcjeff]

@williamFalcon I have re-implemented from scratch, anything you like to add on?

[ydcjeff]

It's ready to be reviewed

[justusschock]

LGTM.

Just some minor comments (mainly on typing)

[williamFalcon]

@ydcjeff awesome!!