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

Redundant decoder in ESPnetASRModel when ctc_weight is 1.0 #4908

Open
pyf98 opened this issue Feb 2, 2023 · 6 comments
Open

Redundant decoder in ESPnetASRModel when ctc_weight is 1.0 #4908

pyf98 opened this issue Feb 2, 2023 · 6 comments
Labels
Bug bug should be fixed

Comments

@pyf98
Copy link
Collaborator

pyf98 commented Feb 2, 2023

Describe the bug
When ctc_weight=1.0, the ESPnetASRModel should not have a decoder in its parameters, regardless of the decoder config. However, now it will add a default RNN decoder.

Note that the typical way to use CTC only is to set ctc_weight: 1.0 and remove decoder and decoder_conf in the training config.

I believe this bug is introduced in a recent commit, because it worked well previously.

https://github.com/espnet/espnet/blob/master/espnet2/asr/espnet_model.py#L146

It should be something like this:

            if ctc_weight == 1.0:
                self.decoder = None
            else:
                self.decoder = decoder
@pyf98 pyf98 added the Bug bug should be fixed label Feb 2, 2023
@sw005320
Copy link
Contributor

sw005320 commented Feb 2, 2023

Thanks!
@ftshijt, can you fix it?
Also, is there a way to add a test to ensure this does not happen?

@ftshijt
Copy link
Collaborator

ftshijt commented Feb 2, 2023

I changed the previous behavior mainly because the self.decoder = None is not user-aware from the config (e.g., user can still set decoder with anything, but eventually, there is no decoder is initialized).

For that purpose, I add a None decoder and ask the user to set decoder=None when they do the initialization. So probably, we may just throw an error (e.g., you should set decoder to none etc.).

What's your opinion on it?

@pyf98
Copy link
Collaborator Author

pyf98 commented Feb 2, 2023

This will affect our previous CTC configs and models.

@ftshijt
Copy link
Collaborator

ftshijt commented Feb 2, 2023

I see. So I will add a warning msg and recover the previous options. thanks for bringing up the issue.

@pyf98
Copy link
Collaborator Author

pyf98 commented Feb 2, 2023

Actually, I think the decoder should be None by default. The user should specify a decoder when they really want to use one.

@ftshijt
Copy link
Collaborator

ftshijt commented Feb 2, 2023

Yeah, I thought that would be better, but the only concern I had at the time is that, it may lead to some issues to other parts (e.g., test functions etc.) Anyway, since we brought up the issue, let's just do it and fixes issues with the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug bug should be fixed
Projects
None yet
Development

No branches or pull requests

3 participants