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

Compute WER metric iteratively #2111

Merged
merged 3 commits into from Apr 6, 2021
Merged

Conversation

albertvillanova
Copy link
Member

Compute WER metric iteratively to avoid MemoryError.

Fix #2078.

@albertvillanova albertvillanova linked an issue Mar 25, 2021 that may be closed by this pull request
Copy link
Member

@lhoestq lhoestq left a comment

Choose a reason for hiding this comment

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

LGTM thank you !

cc @patrickvonplaten any opinion on this ?

measures = compute_measures(reference, prediction)
incorrect += measures["substitutions"] + measures["deletions"] + measures["insertions"]
total += measures["substitutions"] + measures["deletions"] + measures["hits"]
return incorrect / total
Copy link

Choose a reason for hiding this comment

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

Just for safety, you may want to handle the case of total = 0 (e.g., empty set). Unit tests to verify the change would be great as well. Thanks for making this change!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @elgeish for your comments.

The reasons why I have not explicitly handled the edge case total = 0 are:

In relation to unit tests, currently we do not test scripts. Maybe @lhoestq can give more insight on this.

Copy link

Choose a reason for hiding this comment

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

Sounds good; makes sense!

@lhoestq
Copy link
Member

lhoestq commented Mar 31, 2021

I discussed with Patrick and I think we could have a nice addition: have a parameter concatenate_texts that, if True, uses the old implementation.

By default concatenate_texts would be False, so that sentences are evaluated independently, and to save resources (the WER computation has a quadratic complexity).

Some users might still want to use the old implementation.

@lhoestq lhoestq mentioned this pull request Mar 31, 2021
@albertvillanova
Copy link
Member Author

@lhoestq @patrickvonplaten are you sure of the parameter name concatenate_texts? I was thinking about something like iter...

@lhoestq
Copy link
Member

lhoestq commented Apr 1, 2021

Not sure about the name, if you can improve it feel free to do so ^^'
The old implementation computes the WER on the concatenation of all the input texts, while the new one makes WER measures computation independent for each reference/prediction pair.
That's why I thought of concatenate_texts

@albertvillanova
Copy link
Member Author

@lhoestq yes, but the end user does not necessarily know the details of the implementation of the WER computation.

From the end user perspective I think it might make more sense: how do you want to compute the metric?

  • all in once, more RAM memory needed?
  • iteratively, less RAM requirements?

Because of that I was thinking of something like iter or iterative...

@lhoestq
Copy link
Member

lhoestq commented Apr 6, 2021

Personally like concatenate_texts better since I feel like iter or iterate are quite vague

@albertvillanova
Copy link
Member Author

Therefore, you can merge... ;)

@lhoestq
Copy link
Member

lhoestq commented Apr 6, 2021

Ok ! merging :)

@lhoestq lhoestq merged commit 549cd55 into huggingface:master Apr 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MemoryError when computing WER metric
4 participants