-
-
Notifications
You must be signed in to change notification settings - Fork 608
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
Conversation
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) |
I also want to hide behind |
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. |
opt.eta = next!(schedule) | ||
# your training code here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opt.eta = next!(schedule) | |
# your training code here | |
# your training code here | |
opt.eta = next!(schedule) |
There was a problem hiding this comment.
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.
opt.eta = eta | ||
# your training code here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
opt.eta = eta | |
# your training code here | |
# your training code here | |
opt.eta = eta |
I took a quick look at ParametersSchedulers.jl and it looks, add just a minor naming concern for which I filed an issue.
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. |
@CarloLucibello if you are OK with my response to the review, then we can merge |
Never mind missed the approve bors r+ |
Build succeeded: |
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 addedEntry in NEWS.mdFinal review from@dhairyagandhi96
(for API changes).