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

Capture unsupported kwargs in optimize method #910

Open
liuyxpp opened this issue Feb 21, 2021 · 3 comments
Open

Capture unsupported kwargs in optimize method #910

liuyxpp opened this issue Feb 21, 2021 · 3 comments

Comments

@liuyxpp
Copy link

liuyxpp commented Feb 21, 2021

At least in src/univariate/solvers/brent.jl and src/univariate/optimize/interface.jl (first optimize), the optimize function does not capture unsupported kwargs supplied. It makes other packages developed based on Optim much harder to maintain. When Optim.optimize is called in user packages, developers should filter out all unsupported kwargs. Suppose I have a method

function my_optimize(model, config)
    lower, upper = get_lower_upper(config)
    Optim.opimize( x -> solve!(reset(model, x), config), lower, upper, config.algo; config.kwargs...)
end

Now, if config.kwargs contains keyword arguments which are not supported by Optim.optimize, it will not compile. Typical error message is

optimize(::Any, ::T, ::T, ::Optim.Brent; rel_tol, abs_tol, iterations, store_trace, show_trace, callback, show_every, extended_trace) where T<:AbstractFloat at ~/.julia/packages/Optim/onG5j/src/univariate/solvers/brent.jl:23 got unsupported keyword arguments "tol", "interval"

One possible solution to this is to add an extra kwargs... to the optimize method in brent.jl:23, such as

function optimize(
        f, x_lower::T, x_upper::T,
        mo::Brent;
        rel_tol::T = sqrt(eps(T)),
        abs_tol::T = eps(T),
        iterations::Integer = 1_000,
        store_trace::Bool = false,
        show_trace::Bool = false,
        callback = nothing,
        show_every = 1,
        extended_trace::Bool = false,
        kwargs...) where T <: AbstractFloat  # this line is new

Correct me if I am wrong or missing some points here.

@pkofod
Copy link
Member

pkofod commented Feb 21, 2021

I think the downside is that it can be harder to debug sometimes and users might also write reltol=1e-5 thinking they're setting a loose tolerance without getting an error that reltol is not a supported keyword.

@liuyxpp
Copy link
Author

liuyxpp commented Feb 21, 2021

However, in src/univariate/solvers/golden_section.jl, there is a ...nargs in the optimize method as follows

function optimize(f, x_lower::T, x_upper::T,
     mo::GoldenSection;
     rel_tol::T = sqrt(eps(T)),
     abs_tol::T = eps(T),
     iterations::Integer = 1_000,
     store_trace::Bool = false,
     show_trace::Bool = false,
     callback = nothing,
     show_every = 1,
     extended_trace::Bool = false,
     nargs...) where T <: AbstractFloat

Why is the choice inconsistent?

@pkofod
Copy link
Member

pkofod commented Apr 28, 2021

As you can see, the univariate optimization uses keywords where the multivariate optimization uses an options-type. At some point they probably both had kwargs... and I forgot to remove it in GoldenSection. This discrepancy goes back many years, and wouldn't be there if I rewrote it now. I could change it to the options type in v2.0.

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

No branches or pull requests

2 participants