Misleading tensor type in code and documentation of Wav2Vec2ForPreTraining
Author: PierreOrhan
Pull request: no

The Wav2Vec2ForPreTraining expects in its forward pass a torch.BoolTensor for the sampled_negative_indices.
https://github.com/huggingface/transformers/blob/cbbeca3d1733aa7c9b443af5ff231a5affcd8a1e/src/transformers/models/wav2vec2/modeling_wav2vec2.py#L1404

But as the example and the code then indicates, this tensor should not be a boolean mask but should instead store the indices of the negative sample. Therefore its expected type should be long.

This was misleading and lead to a silent bug in my codes for quite long as I would send a boolean tensor fo the negative indices instead of a long tensor.

Is my reading of the code is correct, could we change the expected type of sampled_negative_indices ? 
Maybe @patrickvonplaten ( as it seems that he implemented the code) could confirm my reading?

It could be worth clarifying the name of mask_time_indices as well, since this one expects a mask and therefore a boolean and not a long tensor. It is also converted to a boolean tensor directly in the beginning of the forward call so this could be misleading for others as well.

Thank you for the implementation and help!

Comments:
cc @sanchit-gandhi 

Hey @PierreOrhan, 

You're entirely correct, the variable `sampled_negative_indices` is a tensor of integers of dim `(batch_size, sequence_length, num_negatives)` that specifies the positions (indices) of quantised vectors to use as negatives during pre-training, not a boolean mask. Would you like to open a PR to fix this? 🤗 You can tag me for a review!

Changing the name of the arg `mask_time_indices` would be a breaking change, but we can certainly update the docstring to clarify this!

Sure!
While we are at it, in the original paper and fairseq implementation, there is a scaling of the feature extractor gradient by 0.1 for the base architecture. I can add that gradient scaling if you wish too, it really helped my trainings. This would require either to use a constant value (0.1, but since its not used for the Large model this might not be the best idea) or to change the config class of Wav2vec2 to allow the setting of this multiplier.

This is specifically for pre-training right? We could modify the pre-training scripts accordingly!

https://github.com/huggingface/transformers/tree/main/examples/pytorch/speech-pretraining

Hey @PierreOrhan! Let me know if you're still interested in opening a PR to fix this! It would be awesome to update the Wav2Vec2 code to prevent further silent bugs like this occurring for other users in the future!

We can also look into the gradient scaling in a separate PR if you want! Happy to help with this integration too (think it could be beneficial for training)

Sure, I have this on my todo list and should be able to submit in a month or so (finishing a project right now), it will take me time since I never oppened a PR on a large repo so I need to understand the whole process.
Best,

Hey @PierreOrhan! Sounds good! Best of luck with finishing off your current project! 

Exciting that this will be your first PR on a large repo! I'll be on hand to help you with this process to make it as easy as possible! We have a pretty comprehensive guide for opening a PR on transformers: https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md Feel free to have a skim through this guide to get a feel for the steps involved, and don't hesitate to ask any questions here or on a draft PR! More than happy to help!


This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md) are likely to be ignored.