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

Added Blue Score the respective folders #360

Merged
merged 24 commits into from
Jul 19, 2021

Conversation

karthikrangasai
Copy link
Contributor

@karthikrangasai karthikrangasai commented Jul 8, 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?

Fixes #352 .

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 🙃

@codecov
Copy link

codecov bot commented Jul 8, 2021

Codecov Report

Merging #360 (6284f6a) into master (5c2069e) will decrease coverage by 0.02%.
The diff coverage is 96.20%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #360      +/-   ##
==========================================
- Coverage   96.45%   96.43%   -0.03%     
==========================================
  Files         113      117       +4     
  Lines        3691     3726      +35     
==========================================
+ Hits         3560     3593      +33     
- Misses        131      133       +2     
Flag Coverage Δ
Linux 76.70% <92.40%> (+0.16%) ⬆️
Windows 76.70% <92.40%> (+0.16%) ⬆️
cpu 96.37% <96.20%> (-0.02%) ⬇️
gpu 96.34% <96.20%> (-0.02%) ⬇️
macOS 96.37% <96.20%> (-0.02%) ⬇️
pytest 96.43% <96.20%> (-0.03%) ⬇️
python3.6 95.54% <96.20%> (-0.02%) ⬇️
python3.8 96.37% <96.20%> (-0.02%) ⬇️
python3.9 96.26% <96.20%> (-0.02%) ⬇️
torch1.3.1 95.54% <96.20%> (-0.02%) ⬇️
torch1.4.0 95.62% <96.20%> (-0.02%) ⬇️
torch1.9.0 96.26% <96.20%> (-0.02%) ⬇️

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

Impacted Files Coverage Δ
torchmetrics/functional/nlp.py 71.42% <60.00%> (-26.35%) ⬇️
torchmetrics/functional/text/bleu.py 97.91% <97.91%> (ø)
torchmetrics/__init__.py 100.00% <100.00%> (ø)
torchmetrics/functional/__init__.py 100.00% <100.00%> (ø)
torchmetrics/functional/text/__init__.py 100.00% <100.00%> (ø)
torchmetrics/text/__init__.py 100.00% <100.00%> (ø)
torchmetrics/text/bleu.py 100.00% <100.00%> (ø)
... and 1 more

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 5c2069e...6284f6a. Read the comment docs.

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.

Hi @karthikrangasai,
Looks good until now. Could you also:

  • Add corresponding tests / move the onces in tests/functiona/test_nlp.py to tests/text/test_bleu.py
  • Add /update references in the docs

torchmetrics/functional/__init__.py Outdated Show resolved Hide resolved
torchmetrics/functional/__init__.py Show resolved Hide resolved
@karthikrangasai
Copy link
Contributor Author

@SkafteNicki
Yeah, I will do the same.

@Borda Borda added enhancement New feature or request New metric labels Jul 8, 2021
@karthikrangasai
Copy link
Contributor Author

I have refactored the code such that the Class implementation uses the functional implementation of the BLEU score. I have moved the tests for the functional implementation to the respective directory.

Regarding the tests for the BLEUScore class, I have doubts about using the existing MetricTester because it accepts preds and targets as torch.Tensor type but the inputs for the BLEU Score metric are an iterable of the strings.

One hack I thought we could use is to provide the iterable of strings as **mertic_args and leave preds and targets as None - but this failed all the tests.

Other option I see is to implement a separate TextMetricTester for all Text based metrics. I am not too sure about this methods.

How do I proceed with the tests for Class based Text Metrics?

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.

mix of comments

torchmetrics/text/bleu.py Outdated Show resolved Hide resolved
torchmetrics/text/bleu.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/bleu.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/bleu.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/bleu.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/bleu.py Outdated Show resolved Hide resolved
torchmetrics/text/bleu.py Outdated Show resolved Hide resolved
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.

mind check the failing tests?

@@ -281,17 +281,6 @@ ssim [func]
.. autofunction:: torchmetrics.functional.ssim
:noindex:


***
NLP
Copy link
Member

Choose a reason for hiding this comment

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

shall we call it NLP or Text?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure about this. NLP also includes speech processing. So, if we are to add those metrics as well then we can call it NLP.

Copy link
Member

Choose a reason for hiding this comment

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

well, speech processing you mean conversion from audio > text and back, right?
bu then you still shall measure the quality against each independently as your prediction and target are always other audio or text, right? so we can split NLP as text and audio... 🐰
cc: @SkafteNicki @maximsch2

Copy link
Member

Choose a reason for hiding this comment

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

I prefer text, as the data modality that bleu works on is text, similar to how we have grouped other metrics based on data modality.
Also, this does not matter for the end users, as all modular metrics can just be imported with from torchmetrics import * and functional `from torchmetrics.functional import *

tests/text/test_blue.py Outdated Show resolved Hide resolved
torchmetrics/functional/nlp.py Outdated Show resolved Hide resolved
@Borda Borda added this to the v0.5 milestone Jul 15, 2021
@karthikrangasai
Copy link
Contributor Author

@Borda the tests are failing on Github only because it says list type is not hashable. They run fine on my system.

@Borda
Copy link
Member

Borda commented Jul 15, 2021

@Borda the tests are failing on Github only because it says list type is not hashable. They run fine on my system.

and what is your system? I assume list shall not be hashable anywhere as it immutable
https://stackoverflow.com/questions/42203673/in-python-why-is-a-tuple-hashable-but-not-a-list

@karthikrangasai
Copy link
Contributor Author

Yeah, it was my mistake. I was running python -m pytest -v tests but I had to run python -m pytest -v torchmetrics tests.
I have fixed the failing tests now. I had forgot to update examples in the docstring and they were failing.

@Borda
Copy link
Member

Borda commented Jul 16, 2021

I have fixed the failing tests now. I had forgot to update examples in the docstring and they were failing.

cool, mind commit your fix? 🐰

@mergify mergify bot removed the has conflicts label Jul 17, 2021
@Borda Borda enabled auto-merge (squash) July 19, 2021 09:19
@Borda Borda disabled auto-merge July 19, 2021 09:19
@Borda Borda enabled auto-merge (squash) July 19, 2021 09:19
@Borda Borda disabled auto-merge July 19, 2021 16:25
@Borda Borda merged commit 591bb2d into Lightning-AI:master Jul 19, 2021
@Borda Borda added this to the v0.5 milestone Aug 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Bleu score
3 participants