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

ComposedSchedule example for CosAnneal in the docs is incorrect #38

Closed
a-cakir opened this issue Oct 5, 2022 · 2 comments · Fixed by #40
Closed

ComposedSchedule example for CosAnneal in the docs is incorrect #38

a-cakir opened this issue Oct 5, 2022 · 2 comments · Fixed by #40
Labels
bug Something isn't working

Comments

@a-cakir
Copy link

a-cakir commented Oct 5, 2022

The Cosine annealing variants example shown on the cheatsheet page leads to the following error below.

julia> s = ComposedSchedule(CosAnneal(range, offset, period),
                            (Step(range, m_mul, period), offset, period))
ERROR: MethodError: no method matching CosAnneal(::typeof(range), ::Float64, ::Int64)
Closest candidates are:
  CosAnneal(::T, ::T, ::S, ::Bool) where {T, S<:Integer} at ~/.julia/packages/ParameterSchedulers/CTkAS/src/cyclic.jl:212
Stacktrace:
 [1] top-level scope
   @ REPL[47]:1
 [2] top-level scope
   @ ~/.julia/packages/CUDA/DfvRa/src/initialization.jl:52

You can create a ComposedSchedule by adding the Bool flag when defining the CosAnneal, however, that leads to error when you try to get a learning rate down the line

(this example uses Exp(r, γ) as the schedule)

julia> s = ComposedSchedule(CosAnneal(r, offset, period, true), (Exp(r, γ), offset, period))
ComposedSchedule(CosAnneal{Float64, Int64}, (Exp{Float64}(1.0, 0.95), Constant{Float64}(0.0), Constant{Int64}(5)))

julia> s(1)
ERROR: MethodError: no method matching CosAnneal{Float64, Int64}(::Float64, ::Float64, ::Int64)
Closest candidates are:
  CosAnneal{T, S}(::Any, ::Any, ::Any, ::Any) where {T, S<:Integer} at ~/.julia/packages/ParameterSchedulers/CTkAS/src/cyclic.jl:212
Stacktrace:
 [1] (::ParameterSchedulers.var"#30#31"{CosAnneal{Float64, Int64}})(s::CosAnneal{Float64, Int64}, ps::Tuple{Float64, Float64, Int64})
   @ ParameterSchedulers ~/ParameterSchedulers.jl/src/complex.jl:212
 [2] (::ComposedSchedule{CosAnneal{Float64, Int64}, Tuple{Exp{Float64}, ParameterSchedulers.Constant{Float64}, ParameterSchedulers.Constant{Int64}}, ParameterSchedulers.var"#30#31"{CosAnneal{Float64, Int64}}})(t::Int64)
   @ ParameterSchedulers ~/.julia/packages/ParameterSchedulers/CTkAS/src/complex.jl:227
 [3] top-level scope
   @ REPL[50]:1
 [4] top-level scope
   @ ~/.julia/packages/CUDA/DfvRa/src/initialization.jl:52

This can be remedied by passing the bool flag to the parameter list at the end:

julia> s = ComposedSchedule(CosAnneal(r, offset, period, true), (Exp(r, γ), offset, period, **true**))
ComposedSchedule(CosAnneal{Float64, Int64}, (Exp{Float64}(1.0, 0.95), Constant{Float64}(0.0), Constant{Int64}(5), Constant{Bool}(true)))

julia> s(1)
1.0

Would be good to correct the documentation and/or the code. Not sure what the intended behavior is/should be

@ericphanson
Copy link

btw, @a-cakir pointed out offline that ComposedSchedule is not tested either, that seems like a (maybe separate) issue as well

@darsnack
Copy link
Member

darsnack commented Oct 5, 2022

Looks like the positional constructor is missing a default value for restarts. I'll fix this tonight.

ComposedSchedule probably deserves more thorough testing, but it is used for many of the cyclic schedules. So it is implicitly tested right now. See #39.

@darsnack darsnack added the bug Something isn't working label Oct 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants