Remove default optimizer, add None optimizer option

[ethanwharris]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs?
• Did you write any new necessary tests?
• If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

• no longer automatically choose an optimizer
• will run without an optimizer and warn the user if configure_optimizers is not overriden or returns None
• refactored default optimizer to the init_optimizers method
• refactored init_optimizers and configure_schedulers to their own mixin in seperate file (optimizers.py)
• changed init_optimizers to just take model so that configure_optimizers is only called in one place

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[williamFalcon]

what if we just make the whole function optional? instead of just the args? or is that optional syntax doing that?

[williamFalcon]

or i guess if we make the method still required but optional return, then it maintains readability? on second thought i kind of like this better. that way it’s explicit in code that you used no optimizer

[ethanwharris]

Yeah, this does that - or at least, PyCharm isn't prompting me to implement the method :) - the optional thing just means you wont get a type warning if you return None

[ethanwharris]

^^ we can do that - currently method isn't required but happy to change that :) agree that this behaviour should be explicit

[codecov]

Codecov Report

Merging #1279 into master will increase coverage by <1%.
The diff coverage is 89%.

@@           Coverage Diff           @@
##           master   #1279    +/-   ##
=======================================
+ Coverage      92%     92%   +<1%     
=======================================
  Files          62      62            
  Lines        3245    3173    -72     
=======================================
- Hits         2970    2905    -65     
+ Misses        275     268     -7

[Borda]

can you pls sync work with #1269

[mergify]

This pull request is now in conflict... :(

[pep8speaks]

Hello @ethanwharris! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-04-02 12:38:16 UTC