[Wav2Vec2] Fix normalization for non-padded tensors #13512
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
src/transformers/models/wav2vec2/feature_extraction_wav2vec2.py
Outdated
Show resolved
Hide resolved
src/transformers/models/wav2vec2/feature_extraction_wav2vec2.py
Outdated
Show resolved
Hide resolved
src/transformers/models/wav2vec2/feature_extraction_wav2vec2.py
Outdated
Show resolved
Hide resolved
| @@ -79,13 +79,20 @@ def __init__( | |||
| self.do_normalize = do_normalize | |||
|
|
|||
| @staticmethod | |||
| def zero_mean_unit_var_norm(input_values: List[np.ndarray], input_lengths: List[int]) -> List[np.ndarray]: | |||
| def zero_mean_unit_var_norm( | |||
There was a problem hiding this comment.
The responsibility of retrieving the correct length from the attention mask should be in this method since input_values and attention_mask are the well-known inputs to functions in transformers
| @@ -196,19 +195,33 @@ def __call__( | |||
| return_attention_mask=return_attention_mask, | |||
| ) | |||
|
|
|||
| if "attention_mask" in padded_inputs: | |||
There was a problem hiding this comment.
This part is removed/cleaned-up
| @@ -172,14 +179,6 @@ def __call__( | |||
| and (isinstance(raw_speech[0], np.ndarray) or isinstance(raw_speech[0], (tuple, list))) | |||
| ) | |||
|
|
|||
| # make sure input is in list format | |||
There was a problem hiding this comment.
Currently all the padding is happening in pure python and not in numpy so let's move the numpification further down
| @@ -134,7 +134,22 @@ def _check_zero_mean_unit_variance(input_vector): | |||
| _check_zero_mean_unit_variance(input_values[1, :1000]) | |||
| _check_zero_mean_unit_variance(input_values[2]) | |||
|
|
|||
| def test_zero_mean_unit_variance_normalization_trunc(self): | |||
| def test_zero_mean_unit_variance_normalization(self): | |||
There was a problem hiding this comment.
Add test to make sure normalization always works as expected
4 tasks
4 tasks
patrickvonplaten
requested review from
anton-l,
LysandreJik,
sgugger and
patil-suraj
patil-suraj
approved these changes
Sep 10, 2021
src/transformers/models/wav2vec2/feature_extraction_wav2vec2.py
Outdated
Show resolved
Hide resolved
src/transformers/models/wav2vec2/feature_extraction_wav2vec2.py
Outdated
Show resolved
Hide resolved
LysandreJik
approved these changes
Sep 10, 2021
There was a problem hiding this comment.
Seems to look good but will delegate to @patil-suraj and @anton-l's w2v2 knowledge.
Let me know once this is merged so that I may release a patch.
anton-l
suggested changes
Sep 10, 2021
src/transformers/models/speech_to_text/feature_extraction_speech_to_text.py
Show resolved
Hide resolved
anton-l
approved these changes
Sep 10, 2021
patil-suraj
approved these changes
Sep 10, 2021
src/transformers/models/speech_to_text/feature_extraction_speech_to_text.py
Show resolved
Hide resolved
patrickvonplaten
added a commit
that referenced
this pull request
Sep 10, 2021
* finalize * Apply suggestions from code review * finish cleaner implementation * more tests * small fix * finish * up
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
This PR fixes a problem with normalization when the input is a list of different length that is not numpified - see: #13504
Just noticed that this bug is pretty severe actually as it affects all large-Wav2Vec2 fine-tuning :-/.
It was introduced by me in this PR: https://github.com/huggingface/transformers/pull/12804/files - I should have written more and better tests for this.
=> This means that from transformers 4.9.0 to until this PR is merged the normalization for all large Wav2Vec2 models was way off when fine-tuning the model.
@LysandreJik - do you think it might be possible to do a patched release for this?