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

refactor optimize and scheduling selection #55

Closed
cwmeijer opened this issue Dec 15, 2020 · 2 comments · Fixed by #74
Closed

refactor optimize and scheduling selection #55

cwmeijer opened this issue Dec 15, 2020 · 2 comments · Fixed by #74
Assignees

Comments

@cwmeijer
Copy link
Contributor

Maybe we should take this as an opportunity to use a uniform scheduler creation procedure. I see that different experiment functions use different code (e.g. asr.experiment() has very different rules than basic.experiment()). This should include:

  • a uniform set of accepted optimizers (adadelta, adam...)
  • a uniform set of scheduling options (none, cyclic, noam; where I would use none instead of constant as adaptive optimizers don't use a constant learning rate even without scheduler)
  • uniform parameter names (constant_lr -> lr)
  • a single function in scheduler.py to handle this, called from all experiment() functions

Originally posted by @bhigy in #51 (comment)

@cwmeijer cwmeijer self-assigned this Dec 15, 2020
@cwmeijer
Copy link
Contributor Author

cwmeijer commented Jan 7, 2021

I see multiple options here, but I'm sure there are more.

  1. I create both optimizer and scheduler in basic.experiment() and pass the args dict from every individual experiment script to experiment(). This will keep the experiment scripts clean, but will make parsing arguments a responsibility of experiment, which doesn't fit its level of abstraction nicely as it is also dealing with clear-cut (run_)config dict as a parameter.
  2. I create optimizer and scheduler objects in the experiment scripts and pass these onto basic.experiment() as parameters. This would mean some code duplication in the experiment scripts, as they would need to create these objects. It would also clutter the basic.experiment()'s signature by adding 2 additional parameters. Also, the run_config would no longer contain information about optimizer and scheduler parameters, making logging of parameters to fully replicate the experiment cumbersome.
  3. I create optimizer and scheduler objects in the experiment scripts and pass these onto basic.experiment() as part of the run_config dict. This would, as in 2, mean some code duplication in the experiment scripts, as they would need to create these objects. The config would then contain these 2 objects, making it hard to write the config to a log.
  4. I create both optimizer and scheduler in basic.experiment() and pass the relevant parameters (e.g. max_lr) from every individual experiment script to experiment() through run_config. This will leave a lot of logic up to the experiment scripts, and needs to be duplicated for every experiment script, leaving lots of room for bugs and out-of-date code. This can be solved by letting the scripts call a shared piece of code. For instance: utils.add_optimizer_and_scheduler_parameters(run_config, args); M.experiment(run_config) This would solve quite some of the above issues, but still feels off because of the 2 calls.

@cwmeijer cwmeijer added the help wanted Extra attention is needed label Jan 7, 2021
@cwmeijer cwmeijer removed their assignment Jan 7, 2021
@cwmeijer
Copy link
Contributor Author

cwmeijer commented Feb 4, 2021

Use create_optimizer and create_scheduler in all files that define an experiment. Merge options to these 2 functions.

@cwmeijer cwmeijer removed the help wanted Extra attention is needed label Feb 4, 2021
@cwmeijer cwmeijer self-assigned this Feb 12, 2021
cwmeijer added a commit that referenced this issue Feb 12, 2021
note that I removed an if statement on learning rate config as it didn't seem to
have any result. This is because cyclic scheduler was always used and lr got
ignored always because of that.
@bhigy bhigy closed this as completed in #74 Feb 22, 2021
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 a pull request may close this issue.

1 participant