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

Adding NLP Metrics SQuADv2 and WER #368

Closed
wants to merge 38 commits into from

Conversation

gagan3012
Copy link
Contributor

@gagan3012 gagan3012 commented Jul 13, 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 #52
Fixes #366 (issue).

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
Copy link

pep8speaks commented Jul 13, 2021

Hello @gagan3012! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-07-16 07:59:42 UTC

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. Just wanted to make sure: Are these metrics meant to be accumulated over multiple batches or are they only intended to be evaluated for single batch/sample? Because if it is the latter then it is maybe better to implement them as functionals.

torchmetrics/text/wer/wer.py Outdated Show resolved Hide resolved
torchmetrics/text/wer/wer.py Show resolved Hide resolved
torchmetrics/text/wer/wer.py Outdated Show resolved Hide resolved
torchmetrics/text/wer/wer.py Show resolved Hide resolved
torchmetrics/text/squadv2/squadv2.py Outdated Show resolved Hide resolved
torchmetrics/text/squadv2/squadv2.py Outdated Show resolved Hide resolved
torchmetrics/text/squadv2/evaluate_squad.py Outdated Show resolved Hide resolved
@Borda Borda added this to the v0.5 milestone Jul 15, 2021
torchmetrics/text/squadv2/evaluate_squad.py Outdated Show resolved Hide resolved
torchmetrics/text/squadv2/squadv2.py Outdated Show resolved Hide resolved
torchmetrics/text/squadv2/squadv2.py Outdated Show resolved Hide resolved
torchmetrics/text/wer/wer.py Show resolved Hide resolved
torchmetrics/text/wer/wer.py Outdated Show resolved Hide resolved
Co-authored-by: Nicki Skafte <skaftenicki@gmail.com>
@gagan3012
Copy link
Contributor Author

Hello,
I will implement all changes by tonight (EST)

@gagan3012
Copy link
Contributor Author

Hello,
I have made all the changes requested. Please take a look Thanks

@Borda
Copy link
Member

Borda commented Jul 16, 2021

@gagan3012 seems it is your first PR so GH requires to allow running CI by a maintainer, so pls when you see it is hanging, pls ping me in slack so we can speed it up :]

@@ -0,0 +1 @@
jiwer
Copy link
Member

Choose a reason for hiding this comment

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

any minimal version?

tests/text/test_wer.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/__init__.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/squadv2.py Outdated Show resolved Hide resolved
@@ -0,0 +1,316 @@
import argparse
Copy link
Member

Choose a reason for hiding this comment

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

can you confirm that this is your own code? 🐰

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

I am not very convinced about having any entry point to TM, that shall be done consistently to all metrics then...
also, WEB testing is very sparse and missing any tests for squadv2

@gagan3012 we really appreciate your effort, I would just recommend splitting adding the two metrics to two PR where each will be adding only one of them which would simplify review and also your work by focussing only on one at the time 🐰

cc: @SkafteNicki

@gagan3012
Copy link
Contributor Author

Hello,
I think I will close this and Create separate PRs for both of the metrics

@gagan3012 gagan3012 closed this Jul 16, 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.

Squad V2 Question Answering Task metric [Metrics] WER
4 participants