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 start/decay as Kwargs in Schedulers (apart from the UTF-8 ones) #41

Closed
lfenzo opened this issue Nov 9, 2022 · 5 comments · Fixed by #60
Closed

Add start/decay as Kwargs in Schedulers (apart from the UTF-8 ones) #41

lfenzo opened this issue Nov 9, 2022 · 5 comments · Fixed by #60
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@lfenzo
Copy link
Contributor

lfenzo commented Nov 9, 2022

Describe the potential feature

When writing code from terminal-based editors e.g. Vim, NeoVim, Nano (and AFAIK Helix), it becomes a bit of a cumbersome to repeatedly copy-paste mathematical characters such as λ and γ which appear in most schedulers. Although I see the merit of having UTF-8 characters as kwargs for mathematical aesthetics, I believe we should also have the option to specify such parameters by spelling out their meanings i.e. with start, decay, etc.

As an example, currently we have the following:

   _       _ _(_)_     |  Documentation: https://docs.julialang.org
  (_)     | (_) (_)    |
   _ _   _| |_  __ _   |  Type "?" for help, "]?" for Pkg help.
  | | | | | | |/ _` |  |
  | | |_| | | | (_| |  |  Version 1.7.3 (2022-05-06)
 _/ |\__'_|_|_|\__'_|  |  Fedora 36 build
|__/                   |

julia> using ParameterSchedulers

julia> s = Exp(; start = 0.01, decay = 0.95)
ERROR: UndefKeywordError: keyword argument λ not assigned
Stacktrace:
 [1] top-level scope
   @ REPL[2]:1
 [2] top-level scope
   @ ~/.julia/packages/CUDA/DfvRa/src/initialization.jl:52

Motivation

No response

Possible Implementation

I believe that the implementation should be straight forward, just requiring another definition of the schedulers using the start and decay kwargs (which are already in the "Arguments" section of some of the schedulers doc pages). Using the Step scheduler as an example, we'd have:

Step(;λ, γ, step_sizes) = Step(λ, γ, step_sizes) # current implementation
Step(;start, decay, step_sizes) = Step(start, decay, step_sizes)
@darsnack darsnack added enhancement New feature or request good first issue Good for newcomers labels Nov 9, 2022
@darsnack
Copy link
Member

darsnack commented Nov 9, 2022

Feel free to submit a PR implementing this; otherwise, I'll add it soon!

@lfenzo
Copy link
Contributor Author

lfenzo commented Nov 9, 2022

Thanks @darsnack, I will soon submit a PR addressing this issue!

@lfenzo
Copy link
Contributor Author

lfenzo commented Nov 11, 2022

It turns out that the implementation is not as straight forward as I expected. The reason is that since the methods being defined as in the example I provided are identical (the only difference being the kwarg names), Julia will overwrite the definitions and keep only the last evaluated method:

function foo(; some, args)
    return [some, args]
end

function foo(; other, args)
    return [other, args]
end

println(methods(foo))
# 1 method for generic function "foo":
[1] foo(; other, args) in Main at /var/home/enzo/Documents/projetos/ParameterSchedulers.jl/test.jl:5

Swapping the order of the definition we have:

function foo(; other, args)
    return [other, args]
end

function foo(; some, args)
    return [some, args]
end

println(methods(foo))
# 1 method for generic function "foo":
[1] foo(; some, args) in Main at /var/home/enzo/Documents/projetos/ParameterSchedulers.jl/test.jl:5

So now I don't know exactly how to proceed in this situation. As I skimmed through the code in src/cyclic.jl I noticed that some of the schedulers will have their interface changed in upcoming versions, e.g. Triangle:

function Triangle(range::T, offset::T, period::S) where {T, S}
    @warn """Triangle(range0, range1, period) is now Triangle(range, offset, period).
             To specify by endpoints, use the keyword argument form.
             This message will be removed in the next version.""" _id=(:tri) maxlog=1

    Triangle{T, S}(range, offset, period)
end
Triangle(;λ0, λ1, period) = Triangle(abs(λ0 - λ1), min(λ0, λ1), period)

so a couple of ideas emerged:

  • as this @warn has been around for at least one year in the codebase, it could be time to apply the announced upcoming changes. In this scenario, all schedulers in src/cyclic.jl would have their interface changed to non-mathematical characters; the ones in src/decay.jl could follow the same trend replacing λ by startand so on. Although we're on v0.* this is highly breaking and would required rewriting of almost all docs and tests. Still, if you think this is reasonable, I can work on this in a future PR.
  • find some hacky way to allow Step(;start, decay, step_sizes) and Step(;λ, γ, step_sizes) to coexist (if such thing is actually possible).

@darsnack
Copy link
Member

darsnack commented Feb 3, 2024

Following up on this, I am fine with dropping support for all unicode keywords and switching the plaintext names. Still interested in making a PR?

@lfenzo
Copy link
Contributor Author

lfenzo commented Feb 8, 2024

Still interested in making a PR?

Sure! I'll drop one as soon as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants