-
Notifications
You must be signed in to change notification settings - Fork 255
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
asr model evaluator addition + doc #378
asr model evaluator addition + doc #378
Conversation
The documentation is not available anymore as the PR was closed or merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @bayartsogt-ya, thank you for this incredibly clean PR! Just one small nit and I'll fix the output format of WER/CER.
""" | ||
Examples: | ||
```python | ||
>>> from evaluate import evaluator | ||
>>> from datasets import load_dataset | ||
>>> task_evaluator = evaluator("automatic-speech-recognition") | ||
>>> data = load_dataset("mozilla-foundation/common_voice_11_0", "en", split="validation[:40]") | ||
>>> results = task_evaluator.compute( | ||
>>> model_or_pipeline="https://huggingface.co/openai/whisper-tiny.en", | ||
>>> data=data, | ||
>>> input_column="path", | ||
>>> label_column="sentence", | ||
>>> metric="wer", | ||
>>> ) | ||
``` | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the other Evaluators we added the example also to a string at the beginning that we then added with @add_end_docstrings
. Could we do the same here for uniformity?
@@ -267,6 +267,12 @@ def compute( | |||
random_state=random_state, | |||
) | |||
|
|||
# TODO: To clarify why `wer` and `cer` return float |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's an oversight in the standardization of the metrics. Let me fix that in a separate PR so we can remove this here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually that will probably break a few things so let's keep your workaround for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds great and thanks for making this change! Let me merge with your branch and do changes accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's actually keep your workaround for now.
Hi @bayartsogt-ya, see my comment above: can you keep your workaround and just fix the docstring. also no need to merge the upstream branch into yours then. Thanks! |
4d5eba7
to
3bd2788
Compare
Awesome, thanks for this addition! |
@lewtun
As discussed #324, I am adding
automatic-speech-recognition
evaluator here.Since
automatic-speech-recognition
pipeline already exists and it supports bothWav2Vec2
andWhisper
, I think it is safe addition.I did have some concern where
wer
andcer
metrics both returningfloat
while contract says it should bedict
.So I kind of hacked the way around by checking the type, but definitely want to hear your opinion on this.
Thanks!