Perplexity Speedup
CONTRIBUTOR
WRT the high values, can you add some unit tests with some [string, model] pairs and their resulting perplexity code, and @TristanThrush can run the same pairs through his version of the code?
2022-04-06T13:02:47Z
https://github.com/huggingface/datasets/pull/4108#issuecomment-1090243322
https://api.github.com/repos/huggingface/datasets/issues/4108
IC_kwDODunzps5A-8r6
https://api.github.com/repos/huggingface/datasets/issues/comments/1090243322/reactions
2022-04-06T13:02:47Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1090243322
https://avatars.githubusercontent.com/u/14205986?v=4
https://api.github.com/users/sashavor/events{/privacy}
https://api.github.com/users/sashavor/followers
https://api.github.com/users/sashavor/following{/other_user}
https://api.github.com/users/sashavor/gists{/gist_id}
https://github.com/sashavor
sashavor
MDQ6VXNlcjE0MjA1OTg2
https://api.github.com/users/sashavor/orgs
https://api.github.com/users/sashavor/received_events
https://api.github.com/users/sashavor/repos
https://api.github.com/users/sashavor/starred{/owner}{/repo}
https://api.github.com/users/sashavor/subscriptions
User
https://api.github.com/users/sashavor
public
NONE
_The documentation is not available anymore as the PR was closed or merged._
2022-04-06T13:08:14Z
https://github.com/huggingface/datasets/pull/4108#issuecomment-1090249175
https://api.github.com/repos/huggingface/datasets/issues/4108
IC_kwDODunzps5A--HX
https://api.github.com/repos/huggingface/datasets/issues/comments/1090249175/reactions
2022-04-20T13:00:54Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1090249175
https://avatars.githubusercontent.com/u/99929124?v=4
https://api.github.com/users/HuggingFaceDocBuilderDev/events{/privacy}
https://api.github.com/users/HuggingFaceDocBuilderDev/followers
https://api.github.com/users/HuggingFaceDocBuilderDev/following{/other_user}
https://api.github.com/users/HuggingFaceDocBuilderDev/gists{/gist_id}
https://github.com/HuggingFaceDocBuilderDev
HuggingFaceDocBuilderDev
U_kgDOBfTMJA
https://api.github.com/users/HuggingFaceDocBuilderDev/orgs
https://api.github.com/users/HuggingFaceDocBuilderDev/received_events
https://api.github.com/users/HuggingFaceDocBuilderDev/repos
https://api.github.com/users/HuggingFaceDocBuilderDev/starred{/owner}{/repo}
https://api.github.com/users/HuggingFaceDocBuilderDev/subscriptions
User
https://api.github.com/users/HuggingFaceDocBuilderDev
public
CONTRIBUTOR
I thought that the perplexity metric should output the average perplexity value of all the strings that it gets as input (not a perplexity value per string, as the new version does).
@lhoestq , @TristanThrush thoughts?
2022-04-06T13:28:07Z
https://github.com/huggingface/datasets/pull/4108#issuecomment-1090272593
https://api.github.com/repos/huggingface/datasets/issues/4108
IC_kwDODunzps5A_D1R
https://api.github.com/repos/huggingface/datasets/issues/comments/1090272593/reactions
2022-04-06T13:28:07Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1090272593
https://avatars.githubusercontent.com/u/14205986?v=4
https://api.github.com/users/sashavor/events{/privacy}
https://api.github.com/users/sashavor/followers
https://api.github.com/users/sashavor/following{/other_user}
https://api.github.com/users/sashavor/gists{/gist_id}
https://github.com/sashavor
sashavor
MDQ6VXNlcjE0MjA1OTg2
https://api.github.com/users/sashavor/orgs
https://api.github.com/users/sashavor/received_events
https://api.github.com/users/sashavor/repos
https://api.github.com/users/sashavor/starred{/owner}{/repo}
https://api.github.com/users/sashavor/subscriptions
User
https://api.github.com/users/sashavor
public
CONTRIBUTOR
> I thought that the perplexity metric should output the average perplexity value of all the strings that it gets as input (not a perplexity value per string, as the new version does). @lhoestq , @TristanThrush thoughts?

I support this change from Emi. If we have a perplexity function that loads GPT2 and then returns an average over all of the strings, then it is impossible to get multiple perplexities of a batch of strings efficiently. If we have this new perplexity function that is built for batching, then it is possible to get a batch of perplexities efficiently and you can still compute the average efficiently afterwards.
2022-04-07T02:52:46Z
https://github.com/huggingface/datasets/pull/4108#issuecomment-1091024254
https://api.github.com/repos/huggingface/datasets/issues/4108
IC_kwDODunzps5BB7V-
https://api.github.com/repos/huggingface/datasets/issues/comments/1091024254/reactions
2022-04-07T02:52:46Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1091024254
https://avatars.githubusercontent.com/u/20826878?v=4
https://api.github.com/users/TristanThrush/events{/privacy}
https://api.github.com/users/TristanThrush/followers
https://api.github.com/users/TristanThrush/following{/other_user}
https://api.github.com/users/TristanThrush/gists{/gist_id}
https://github.com/TristanThrush
TristanThrush
MDQ6VXNlcjIwODI2ODc4
https://api.github.com/users/TristanThrush/orgs
https://api.github.com/users/TristanThrush/received_events
https://api.github.com/users/TristanThrush/repos
https://api.github.com/users/TristanThrush/starred{/owner}{/repo}
https://api.github.com/users/TristanThrush/subscriptions
User
https://api.github.com/users/TristanThrush
public
MEMBER
Thanks a lot for working on this @emibaylor @TristanThrush :)

For consistency with the other metrics, I think it's nice if we return the mean perplexity. Though I agree that having the separate perplexities per sample can also be useful. What do you think about returning both ?
```python
return {"perplexities": ppls, "mean_perplexity": np.mean(ppls)}
```
we're also doing this for the COMET metric.
2022-04-07T15:01:48Z
https://github.com/huggingface/datasets/pull/4108#issuecomment-1091850448
https://api.github.com/repos/huggingface/datasets/issues/4108
IC_kwDODunzps5BFFDQ
https://api.github.com/repos/huggingface/datasets/issues/comments/1091850448/reactions
2022-04-07T15:01:48Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1091850448
https://avatars.githubusercontent.com/u/42851186?v=4
https://api.github.com/users/lhoestq/events{/privacy}
https://api.github.com/users/lhoestq/followers
https://api.github.com/users/lhoestq/following{/other_user}
https://api.github.com/users/lhoestq/gists{/gist_id}
https://github.com/lhoestq
lhoestq
MDQ6VXNlcjQyODUxMTg2
https://api.github.com/users/lhoestq/orgs
https://api.github.com/users/lhoestq/received_events
https://api.github.com/users/lhoestq/repos
https://api.github.com/users/lhoestq/starred{/owner}{/repo}
https://api.github.com/users/lhoestq/subscriptions
User
https://api.github.com/users/lhoestq
public
CONTRIBUTOR
> Thanks a lot for working on this @emibaylor @TristanThrush :)
> 
> For consistency with the other metrics, I think it's nice if we return the mean perplexity. Though I agree that having the separate perplexities per sample can also be useful. What do you think about returning both ?
> 
> ```python
> return {"perplexities": ppls, "mean_perplexity": np.mean(ppls)}
> ```
> 
> we're also doing this for the COMET metric.

Thanks! Sounds great to me.
2022-04-07T19:02:14Z
https://github.com/huggingface/datasets/pull/4108#issuecomment-1092098543
https://api.github.com/repos/huggingface/datasets/issues/4108
IC_kwDODunzps5BGBnv
https://api.github.com/repos/huggingface/datasets/issues/comments/1092098543/reactions
2022-04-07T19:02:14Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1092098543
https://avatars.githubusercontent.com/u/20826878?v=4
https://api.github.com/users/TristanThrush/events{/privacy}
https://api.github.com/users/TristanThrush/followers
https://api.github.com/users/TristanThrush/following{/other_user}
https://api.github.com/users/TristanThrush/gists{/gist_id}
https://github.com/TristanThrush
TristanThrush
MDQ6VXNlcjIwODI2ODc4
https://api.github.com/users/TristanThrush/orgs
https://api.github.com/users/TristanThrush/received_events
https://api.github.com/users/TristanThrush/repos
https://api.github.com/users/TristanThrush/starred{/owner}{/repo}
https://api.github.com/users/TristanThrush/subscriptions
User
https://api.github.com/users/TristanThrush
public
MEMBER
The CI fail is unrelated to your PR and has been fixed on master, feel free to merge the master branch into your PR to fix the CI ;)
2022-04-13T15:40:45Z
https://github.com/huggingface/datasets/pull/4108#issuecomment-1098204809
https://api.github.com/repos/huggingface/datasets/issues/4108
IC_kwDODunzps5BdUaJ
https://api.github.com/repos/huggingface/datasets/issues/comments/1098204809/reactions
2022-04-13T15:40:45Z
https://api.github.com/repos/huggingface/datasets/issues/comments/1098204809
https://avatars.githubusercontent.com/u/42851186?v=4
https://api.github.com/users/lhoestq/events{/privacy}
https://api.github.com/users/lhoestq/followers
https://api.github.com/users/lhoestq/following{/other_user}
https://api.github.com/users/lhoestq/gists{/gist_id}
https://github.com/lhoestq
lhoestq
MDQ6VXNlcjQyODUxMTg2
https://api.github.com/users/lhoestq/orgs
https://api.github.com/users/lhoestq/received_events
https://api.github.com/users/lhoestq/repos
https://api.github.com/users/lhoestq/starred{/owner}{/repo}
https://api.github.com/users/lhoestq/subscriptions
User
https://api.github.com/users/lhoestq
public
This PR makes necessary changes to perplexity such that:
- it runs much faster (via batching)
- it throws an error when input is empty, or when input is one word without <BOS> token
- it adds the option to add a <BOS> token

Issues:
- The values returned are extremely high, and I'm worried they aren't correct. Even if they are correct, they are sometimes returned as `inf`, which is not very useful (see [comment below](https://github.com/huggingface/datasets/pull/4108#discussion_r843931094) for some of the output values). 
  - If the values are not correct, can you help me find the error?
  - If the values are correct, it might be worth it to measure something like perplexity per word, which would allow us to get actual values for the larger perplexities, instead of just `inf`

Future:
- `stride` is not currently implemented here. I have some thoughts on how to make it happen with batching, but I think it would be better to get another set of eyes to look at any possible errors causing such large values now rather than later.