Update Fminbox to better API #584
Conversation
I'll close the PR. It is probably easier to keep it all within one PR, as there are several WIP PRs to deal with already. |
src/types.jl
Outdated
| successive_f_tol::Int = 0, | ||
| iterations::Integer = 1_000, | ||
| outer_iterations = 1000, |
There was a problem hiding this comment.
outer_iterations::Int ?
(And should we change iterations::Integer to ::Int as well?)
There was a problem hiding this comment.
If it is a single number, I am not sure it is worth supporting all sizes of integers, unless there is an application with more iterations than 9,223,372,036,854,775,807.
There was a problem hiding this comment.
It really doesn't matter... "worth" is without meaning when it's free, but I can change it to keep consistence.
| @@ -179,10 +198,14 @@ function optimize( | |||
| warn("Initial position cannot be on the boundary of the box. Moving elements to the interior.\nElement indices affected: $boundaryidx") | |||
| end | |||
| df.df(gfunc, x) | |||
There was a problem hiding this comment.
Let's update this to gradient!(df, x) ; gfunc .= gradient(df)? (Or however is best to use the NLSolversBase interface here)
| Fminbox() = Fminbox{ConjugateGradient}() # default optimizer | ||
| struct Fminbox{T} <: AbstractOptimizer | ||
| inner_optimizer::T | ||
| mu0 |
| # Increment the number of steps we've had to perform | ||
| iteration += 1 | ||
|
|
||
| copy!(xold, x) | ||
| # Optimize with current setting of mu | ||
| funcc = (g, x) -> barrier_combined(gfunc, gbarrier, g, x, fb, mu) | ||
| funcc = (g, x) -> barrier_combined(gfunc, gbarrier, g, x, fb, mu[1]) |
There was a problem hiding this comment.
May also trigger the closure bug.
| @@ -179,10 +198,14 @@ function optimize( | |||
| warn("Initial position cannot be on the boundary of the box. Moving elements to the interior.\nElement indices affected: $boundaryidx") | |||
| end | |||
| df.df(gfunc, x) | |||
| mu = isnan(mu0) ? initial_mu(gfunc, gbarrier; mu0factor=mufactor) : mu0 | |||
| mu = isnan(mu0) ? [initial_mu(gfunc, gbarrier; mu0factor=mufactor)] : [mu0] | |||
There was a problem hiding this comment.
Why is mu an array? I only see one place where it is modified inside of optimize, so passing it around as a number will probably be better.
There was a problem hiding this comment.
If you look at the changes you'll realize it is to avoid redefining pcp and thus _optimizer on every iteration. I think it's significantly simpler to do it like this compared to defining a type and call overloading it. However, if the closure bug (which seriously is the biggest gripe in Julia to me) has bite here, then it might be worth it to do the complicated thing.
edit: but will it really trigger the closure bug in funcc? I'm closing over mu[1] there... Anyway, the call overloading may still be worth it.
There was a problem hiding this comment.
Hmm, I see. A Base.RefValue would probably work better then which is created using Ref(mu0) for example. I am not 100% sure the closure bug will bite, but I have become somewhat closure-phobic over the last few months.
There was a problem hiding this comment.
A red could work, and I think I’ve used that elsewhere, so maybe for consistency I should do that. I really think it’s annoying that the closure bug is still around. We got the first perf regression issue here at optim just after v0.5.0 hit , and I really hoped it would be fixed in v0.5.1, but...
There was a problem hiding this comment.
Ya, it's annoying but since it's a bug fix, I think it may be done in the 1.0 cycle.
Codecov Report
@@ Coverage Diff @@
## master #584 +/- ##
==========================================
+ Coverage 88.51% 88.75% +0.24%
==========================================
Files 36 36
Lines 1942 1957 +15
==========================================
+ Hits 1719 1737 +18
+ Misses 223 220 -3
Continue to review full report at Codecov.
|
I guess ref #583 (I can take the tests here if you just close that PR, or you can PR your tests against this branch)
also ref #542
The idea here is to have
Fminboxaccept the method instanceor
(I know this current version has a keyword, but that'll go)
I also moved the weird way of specifying the options into
Options, so the API is now like the rest of the package.I also plan to have
where method might be
BFGS()or similar dispatch tothat is if bounds are given, it will default to Fminbox (the only possibility we have right now, but other methods could be implemented).
edit: don't mind the weird first commit...