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

Config handling, minor refactorings #192

Closed
wants to merge 3 commits into from
Closed

Config handling, minor refactorings #192

wants to merge 3 commits into from

Conversation

twerkmeister
Copy link
Contributor

  • handing down config to model instead of early unpacking, solves problems with different interfaces of models Taco1 and 2, makes it easier to introduce new config variables for the models, less verbose. For example num_mels wasn't handed down properly to taco2 model before...
  • few other small refactorings, removed a bit of duplicated code, fewer global variables in train.py

@twerkmeister
Copy link
Contributor Author

not ready to merge yet, I didn't properly test with the upstream changes

@twerkmeister
Copy link
Contributor Author

should be good now

@twerkmeister twerkmeister mentioned this pull request May 29, 2019
@erogol
Copy link
Contributor

erogol commented Jun 3, 2019

@twerkmeister this is outdated. I also believe keeping the variables as it is is more descriptive. If you feel otherwise, let's discuss.

@erogol erogol closed this Jun 3, 2019
@twerkmeister
Copy link
Contributor Author

twerkmeister commented Jun 5, 2019

I am still in favor of using the config object more. With the setup right now, imagine adding a new configurable variable to the decoder. You then need to add the parameter to the initialization of 1) the model, 2) the decoder, and 3+4) instantiation of these classes. That's 4 changes just to add a new config parameter. If you pass the config, you don't need to change anything when introducing a new parameter, because you are simply passing down the config as before and then you use it at the respective location you wanna use it. Having the parameters in multiple class initiations also makes using default parameters much more difficult (they have to be in sync). On top of that it would be nice to have a default config object that is better structured and explained than the current comments in json solution. And we can still add comments to the nn.module subclasses to give a more in depth intuition about the parameters.

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.

2 participants