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.

Sign up for GitHub

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

Jump to bottom

Character Error Rate #575

Merged
merged 54 commits into from
Oct 25, 2021
Merged

Character Error Rate #575

merged 54 commits into from
Oct 25, 2021

Conversation

mahinlma
Copy link
Contributor

@mahinlma mahinlma commented Oct 21, 2021 •
edited
Loading

Character Error Rate logic updated

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 # (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 🙃

@mahinlma
Create cer.py
4507ece 
Character Error Rate logic updated
@codecov
Copy link

codecov bot commented Oct 21, 2021 •
edited
Loading

Codecov Report

Merging #575 (bc9132c) into master (f100130) will increase coverage by 0%.
The diff coverage is 96%.

@@          Coverage Diff          @@
##           master   #575   +/-   ##
=====================================
  Coverage      95%    95%           
=====================================
  Files         146    148    +2     
  Lines        5052   5108   +56     
=====================================
+ Hits         4807   4861   +54     
- Misses        245    247    +2

@mahinlma mahinlma changed the title Character Error Rate Character Error Rate - metric Oct 22, 2021
SkafteNicki
SkafteNicki reviewed Oct 22, 2021
View reviewed changes
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.

So the implementation is already looking great :]
Missing

  • tests
  • bindings in __init__ files
  • sections in documentation
  • entry in CHANGELOG

torchmetrics/functional/text/cer.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/cer.py Outdated Show resolved Hide resolved
torchmetrics/functional/text/cer.py Outdated Show resolved Hide resolved
torchmetrics/text/cer.py Outdated Show resolved Hide resolved
torchmetrics/text/cer.py Outdated Show resolved Hide resolved
torchmetrics/text/cer.py Outdated Show resolved Hide resolved
torchmetrics/text/cer.py Outdated Show resolved Hide resolved
@mahinlma mahinlma changed the title Character Error Rate - metric Character Error Rate Oct 22, 2021
Borda
Borda reviewed Oct 22, 2021
View reviewed changes
torchmetrics/functional/text/cer.py Outdated Show resolved Hide resolved
torchmetrics/text/cer.py Outdated Show resolved Hide resolved
mahinlma and others added 15 commits October 23, 2021 00:50
@mahinlma
Merge branch 'master' into master
00169a1
@mahinlma
minor issue fix
61a60ae
@mahinlma
Merge branch 'master' of https://github.com/mahinlma/metrics
f3993cf
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
0f7bded 
for more information, see https://pre-commit.ci
@mahinlma
Update test_cer.py
21a4b2a
@mahinlma
Merge branch 'master' of https://github.com/mahinlma/metrics
0d358b1
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
d9f0b35 
for more information, see https://pre-commit.ci
@mahinlma
Update test_cer.py
0d9e658
@mahinlma
Merge branch 'master' of https://github.com/mahinlma/metrics
41455ee
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
e5e74f8 
for more information, see https://pre-commit.ci
@mahinlma
Update test_cer.py
7de0aec
@mahinlma
Merge branch 'master' of https://github.com/mahinlma/metrics
27626c4
@pre-commit-ci
[pre-commit.ci] auto fixes from pre-commit.com hooks
51d7fbe 
for more information, see https://pre-commit.ci
@mahinlma
Update test_cer.py
cddddc9
@mahinlma
Merge branch 'master' of https://github.com/mahinlma/metrics
de33c88
@mahinlma
Copy link
Contributor Author

@Borda @SkafteNicki , there are 7 workflows awaiting approval. please check and review

@mahinlma mahinlma requested review from Borda and SkafteNicki October 23, 2021 06:04
@SkafteNicki SkafteNicki enabled auto-merge (squash) October 23, 2021 06:20
@mahinlma
Copy link
Contributor Author

@SkafteNicki , pytest with (ubuntu-20.04, 3.9, latest) and pytest (windows-2019, 3.8, minimal) cancelled

10/12 - successful, 2/12 cancelled
Error: The operation was canceled

@Borda
Copy link
Member

Borda commented Oct 23, 2021

pytest with (ubuntu-20.04, 3.9, latest) and pytest (windows-2019, 3.8, minimal) cancelled

shall be fixed now on master... the case is that the newest PT 1.10 takes much longer than others...

@mahinlma
Copy link
Contributor Author

@Borda @SkafteNicki please review and let me know, if any changes need to be updated from my end

@SkafteNicki
Copy link
Member

Hi @mahinlma, PR is overall looking really good.
I have improved testing by comparing to another framework for correctness of the computation.
Hopefully, we can get it merged before the next release this week.

@mergify mergify bot added the ready label Oct 25, 2021
@SkafteNicki SkafteNicki merged commit 3664c7b into Lightning-AI:master Oct 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SkafteNicki SkafteNicki SkafteNicki approved these changes

@Borda Borda Borda approved these changes

@ananyahjha93 ananyahjha93 Awaiting requested review from ananyahjha93

@ethanwharris ethanwharris Awaiting requested review from ethanwharris

@justusschock justusschock Awaiting requested review from justusschock justusschock is a code owner

@SeanNaren SeanNaren Awaiting requested review from SeanNaren

@tchaton tchaton Awaiting requested review from tchaton

@edenlightning edenlightning Awaiting requested review from edenlightning

Assignees

@SkafteNicki SkafteNicki

Labels
New metric ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mahinlma @Borda @SkafteNicki