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

Add ParameterSchedulers.jl to docs #1511

Merged
merged 2 commits into from
Feb 19, 2021

Conversation

darsnack
Copy link
Member

@darsnack darsnack commented Feb 17, 2021

As per the discussion in #1506, this adds a section to the docs that briefly describes scheduling with ParameterSchedulers.jl. Only concerns before merging are the naming conventions in ParameterSchedulers.jl. If I could get some feedback on those, then I can submit a minor release before officially merging this into the Flux docs.

PR Checklist

  • Tests are added
  • Entry in NEWS.md
  • Documentation, if applicable
  • Final review from @dhairyagandhi96 (for API changes).

@CarloLucibello
Copy link
Member

Do we need to document and mention direct usage of ParameterSchedulers.jl in Flux? I thought that one of the points of #1506 was to hide ParameterSchedulers.jl usage behind a Flux.Schedule module (which is something I like)

@darsnack
Copy link
Member Author

I also want to hide behind Schedule, but I can't do that without #1506. At the same time, it seemed like #1506 wasn't getting merged because of some doubts about ParameterSchedulers.jl usage with Flux code? The option presented was to document it instead of depend on it which I felt was reasonable. It would direct people towards the package, and we could get feedback without committing to it in Flux.

@darsnack
Copy link
Member Author

I'm okay just mentioning ParameterSchedulers.jl in the "ecosystem" section as just a bullet and removing the rest too. My impression is that if it is not explicitly mentioned with an example, then people will not know to use it. That would defeat the purpose of "vetting" the package.

Comment on lines +155 to +156
opt.eta = next!(schedule)
# your training code here
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
opt.eta = next!(schedule)
# your training code here
# your training code here
opt.eta = next!(schedule)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The original is actually the expected behavior. The first call to next!(schedule) will return the very first parameter value. I think that makes sense given that the opt.eta value when you construct opt can be out of sync with the schedule policy. This style ensures that the schedule policy sets is the authoritative eta on every iteration.

Comment on lines +163 to +164
opt.eta = eta
# your training code here
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
opt.eta = eta
# your training code here
# your training code here
opt.eta = eta

@CarloLucibello
Copy link
Member

I took a quick look at ParametersSchedulers.jl and it looks, add just a minor naming concern for which I filed an issue.

I also want to hide behind Schedule, but I can't do that without #1506. At the same time, it seemed like #1506 wasn't getting merged because of some doubts about ParameterSchedulers.jl usage with Flux code?

I would prefer to merge #1506 but don't want to spend months discussing it, so let's merge this and #1506 will come later.

@darsnack
Copy link
Member Author

@CarloLucibello if you are OK with my response to the review, then we can merge

@darsnack
Copy link
Member Author

Never mind missed the approve

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 19, 2021

Build succeeded:

@bors bors bot merged commit ea41ea6 into FluxML:master Feb 19, 2021
@darsnack darsnack deleted the darsnack/scheduling-docs branch February 19, 2021 17:44
@darsnack darsnack mentioned this pull request Feb 22, 2021
92 tasks
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