[WIP] Replace method symbols with abstract types #149

Evizero · 2015-10-24T10:01:26Z

As discussed in #145 .

replace method symbols with abstract types to exploit type dispatch and simplify exposing custom keyword arguments for different methods
remove nnls
clean up tests to use types instead of symbols to specify the method
deprecate old standalone methods (e.g. Optim.l_bfgs)
add tests for deprecated methods (?)
use hashtable for method symbols
adapt readme to new API (thanks to @pkofod)
review and then resolve merge-conflicts with master

~~This works right now although the tests still need some clean-up.~~

I thought this would be a good point to gather feedback

mlubin · 2015-10-26T15:44:03Z

I like it. Should we deprecate choosing the algorithms via symbols?

gabrielgellner · 2015-10-27T00:51:25Z

Super sweet! Should getOptimizer be get_optimizer? I thought camel case was a no no in Julia.

Evizero · 2015-10-27T07:01:26Z

Should we deprecate choosing the algorithms via symbols?

I don't know. I simply assumed so for some reason. Personally, I'm neutral to it.

Should getOptimizer be get_optimizer? I thought camel case was a no no in Julia.

yes, good catch! old habits...

johnmyleswhite · 2015-10-27T17:56:23Z

src/accelerated_gradient_descent.jl

+                     callback = nothing,
+                     show_every = 1,
+                     linesearch!::Function = hz_linesearch!,
+                     nargs...)


What's this? Let's avoid passing around things that aren't strictly necessary.

what do you mean?

What's the purpose of nargs...?

johnmyleswhite · 2015-10-27T18:03:20Z

This looks really great. Two concerns:

Passing around nargs is somewhat un-Julian. We shouldn't propagate things that aren't amenable to human-level static analysis.
We should deprecate the old functions that are being removed, like Optim.l_bfgs. Some people are almost certainly calling those functions directly.

Evizero · 2015-10-27T18:08:54Z

src/optimize.jl

+                  initial_x::Array,
+                  method::Optimizer;
+                  autodiff::Bool = false,
+                  nargs...)


I pass around nargs... to not break this design choice using autodiff as a KW args. This is because now this method is a generic fall-back.

Evizero · 2015-10-27T18:15:25Z

Passing around nargs is somewhat un-Julian

See my last comment. I'm not sure how to solve this without breaking the autodiff KW, but I'll look into it. (If you have an idea I'm all ears)

We should deprecate the old functions that are being removed

ok will do

Evizero · 2015-10-28T11:34:28Z

A question about nargs...: Do you want to get rid of those in general? even in optimize.jl where they greatly reduce redundant code? If so I will. But I got to say that this would make the whole code a lot uglier and almost all default values meaningless as they would always be overwritten by the initial method. Seems like bad coding practice to me

johnmyleswhite · 2015-10-29T02:56:47Z

Yes, we can't merge code that uses nargs. It makes local reasoning excessively difficult because it pushes around an unknown number of arguments of unknown types.

When I have some time free, I will figure out how to avoid the use of nargs. Ideally we'll move towards a shim-layer API that translates keyword arguments into a type that has fields for all options. You can see what NLopt does for an example: https://github.com/JuliaOpt/NLopt.jl/blob/523987afbb32105ed78dad0a127298284113147d/src/NLoptSolverInterface.jl#L4

Evizero · 2015-10-29T11:58:45Z

When I have some time free, I will figure out how to avoid the use of nargs. Ideally we'll move towards a shim-layer API that translates keyword arguments into a type that has fields for all options

This makes sense to me. I'll give it a shot.

Evizero · 2015-10-31T21:35:17Z

Let me know what you think of this. Basically I put the general optimization options in OptimizationOptions and the special arguments (such as the history size of LBFGS) into the Optimizer type itself

mlubin · 2015-10-31T23:52:59Z

👍
Embedding the options in the algorithm types themselves is essentially what we also do in MathProgBase.

mlubin · 2015-12-26T21:59:22Z

@Evizero, any plans to finish this up? It would be a very welcome contribution.

Evizero · 2015-12-27T00:29:59Z

I got sidetracked because the approach of optimizing Julia-functions directly turned out to be somewhat inconvenient for what I was working on at the time.

Be that as it may, I do think the main bulk of work is done already. So if you're still interested in this, then I'll see to it that I finish this up within the next couple weeks. I'm currently drowning in work for an overdue project, but I do try to get a little Julia coding done each day.

mlubin · 2015-12-28T02:40:06Z

Definitely interested, I promise a quick review on my part

Evizero · 2016-01-28T22:19:59Z

Some progress. Still need to add the old methods back and deprecate them (e.g. Optim.l_bfgs(...))

Evizero · 2016-01-28T22:38:06Z

BTW I wasn't able to rebase because of conflicting patches. So now nnls is reintroduced. If I remember it correctly I originally deleted nnls because I read somewhere that this was on the agenda anyway. EDIT: I undid the premature merge.

Should nnls stay or go?

johnmyleswhite · 2016-01-28T22:46:44Z

Either is good by me if it helps you.

pkofod · 2016-02-15T12:07:42Z

Looks good to me.

Evizero · 2016-02-15T21:38:25Z

All right then! Tomorrow morning I will go over the code again to see if I missed something (like a test here or there) and then I'll resolve the merge conflicts with master so we can merge this!

pkofod · 2016-02-15T21:53:20Z

Just a quick question, do we need to export Optimizer? I'm not saying it's wrong, I'm just trying to understand what the benefit is.

Evizero · 2016-02-15T22:05:50Z

no, good point. I guess there is no benefit for the end user in having Optimizer in the namespace. I will take a close look at the exports tomorrow as well.

Conflicts: src/cg.jl src/nelder_mead.jl src/nnls.jl test/brent.jl test/callbacks.jl test/nelder_mead.jl test/newton.jl test/simulated_annealing.jl

Evizero · 2016-02-16T09:31:49Z

So, I think this should be ok now as far as the API is concerned. I undid some commits here and there, since the method keyword is back into play.

~~The travis error on 0.5 looks unrelated to this patch.~~

KristofferC · 2016-02-16T09:59:33Z

test/callbacks.jl

@@ -1,6 +1,14 @@

-for method in (:nelder_mead,
-               :simulated_annealing)
+function cb(tr::OptimizationTrace)


This function and the one below should be removed. They got accidentally added in your rebase.

After removing those, tests pass on 0.5.

Oh, I see. Well If I remove those then there is no test anymore that checks if the callback got called

Maybe an anonymous function will do the trick

I am only talking about the two most upper ones. They seem completely unused since they get overwritten immediately.

You are right, I saw them too and removed them.

pkofod · 2016-02-17T21:21:54Z

Essentially, lgtm. Looking down over the README.mdagain. Now, we still allow method to be specified, but I'm not sure there's a reason to introduce the usage of optimize as if method is required. On the other hand, the documentation might need a larger overhaul soon, so this can be merged as is (imho) as the syntax still works.

pkofod · 2016-02-22T12:25:50Z

@johnmyleswhite sorry to ping, but do you have any comments ? Besides a pre-merge squash maybe.

johnmyleswhite · 2016-02-22T16:58:07Z

I will review this tonight and make sure this gets merged unless there turns out to be an issue I don't expect -- which I think is extremely unlikely.

johnmyleswhite · 2016-02-23T03:01:51Z

README.md

@@ -52,36 +30,55 @@ optimize(f, [0.0, 0.0])
 Optim will default to using the Nelder-Mead method in this case. We can specify the Nelder-Mead method explicitly using the `method` keyword:

 ```jl
-optimize(f, [0.0, 0.0], method = :nelder_mead)
+optimize(f, [0.0, 0.0], method = NelderMead())


Should we encourage people to use keywords in the docs? I would think one of the main gains of the switch to types is the chance for completely static dispatch, which I would assume want to promote, although that's obviously a complex issue.

I agree that it should go away most places, and it is my fault it is still in there. As mentioned above, in my latest comment, I figured it could get changed when some of the other API changes are made.

Works for me.

johnmyleswhite · 2016-02-23T03:11:25Z

Made some comments that could be up for discussion, but I'm happy with this and think it's ready to merge unless anyone wants to make changes.

It's an impressively thorough body of high-quality work. Well done, @Evizero.

Removed method keywords, clarified the reasons for use of user provided gradients, and clarified a dispatch comment.

pkofod · 2016-02-23T06:27:23Z

Okay, so I created https://github.com/Evizero/Optim.jl/pull/5 . It is up to you @Evizero if you want to merge it. If you'd rather not, I'll just commit it after this merges.

I agree this is solid work, and I really think it moves the package forwards.

Fix README.md once more.

Evizero · 2016-02-23T08:13:11Z

Thanks for the review and kind comments. I am glad that I could contribute something useful and will hopefully find myself in this position again.

[WIP] Replace method symbols with abstract types

KristofferC · 2016-02-23T14:30:36Z

🎉 🎈

mlubin · 2016-02-23T14:31:16Z

Thanks @Evizero, this is the biggest change to Optim in quite a while.
Would someone like to summarize the changes in a post to the julia-opt list?

pkofod · 2016-02-23T14:41:25Z

By the way, regarding the lowercase letters in Fminbox. What is one of the nice things in this PR is that if we want it to be FMinBox at some point, we can now deprecate Fminbox which was not possible with :fminbox. Very handy for potential breaking changes going forward.

Edit: We can certain notify julia-opt, but we should probably note that this is the api for optimization, and that the api for accessing the results will also change soon.

Evizero added 3 commits October 24, 2015 10:16

add abstract baseclass for Optimizer

2e97456

remove nnls

d9c133c

refactor to use types instead of symbols to denote methods

9207108

johnmyleswhite reviewed Oct 27, 2015
View reviewed changes

Evizero reviewed Oct 27, 2015
View reviewed changes

rename getOptimizer to get_optimizer and use Dict as lookuptable

6c33711

introduce OptimizationOptions

9354d05

mlubin mentioned this pull request Dec 12, 2015

extra cg arguments in fminbox #89

Closed

mlubin mentioned this pull request Dec 28, 2015

allow optimize() to override default neighbor!() for simulated_annealing #155

Closed

mlubin mentioned this pull request Jan 25, 2016

Non convergence of norm #161

Closed

timholy mentioned this pull request Jan 26, 2016

optim-julep: define AbstractProblem and make use of it in optimizers #163

Closed

mlubin mentioned this pull request Feb 1, 2016

Getting the API train running #165

Merged

Evizero added 6 commits February 16, 2016 09:52

allow new api call without specifying options

03c9ac2

add tests to make sure deprectated api still works

14add34

move printing the trace header to OptimizationOptions

ff2586f

allow low-level interface to be called using functions directly

6b88947

readme: mention typestable low-level interface to optimize

e23fa51

Merge branch 'master' into chris

cd297ad

Conflicts: src/cg.jl src/nelder_mead.jl src/nnls.jl test/brent.jl test/callbacks.jl test/nelder_mead.jl test/newton.jl test/simulated_annealing.jl

KristofferC reviewed Feb 16, 2016
View reviewed changes

fix callbacks test for 0.5 by using anonymous functions

c67feac

johnmyleswhite reviewed Feb 23, 2016
View reviewed changes

Fix README.md once more.

abd3fab

Removed method keywords, clarified the reasons for use of user provided gradients, and clarified a dispatch comment.

Merge pull request #5 from pkofod/patch-5

673c784

Fix README.md once more.

mlubin added a commit that referenced this pull request Feb 23, 2016

Merge pull request #149 from Evizero/chris

c42b60f

[WIP] Replace method symbols with abstract types

mlubin merged commit c42b60f into JuliaNLSolvers:master Feb 23, 2016

[WIP] Replace method symbols with abstract types #149

[WIP] Replace method symbols with abstract types #149

Conversation

Evizero commented Oct 24, 2015

mlubin commented Oct 26, 2015

gabrielgellner commented Oct 27, 2015

Evizero commented Oct 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnmyleswhite commented Oct 27, 2015

Choose a reason for hiding this comment

Evizero commented Oct 27, 2015

Evizero commented Oct 28, 2015

johnmyleswhite commented Oct 29, 2015

Evizero commented Oct 29, 2015

Evizero commented Oct 31, 2015

mlubin commented Oct 31, 2015

mlubin commented Dec 26, 2015

Evizero commented Dec 27, 2015

mlubin commented Dec 28, 2015

Evizero commented Jan 28, 2016

Evizero commented Jan 28, 2016

johnmyleswhite commented Jan 28, 2016

pkofod commented Feb 15, 2016

Evizero commented Feb 15, 2016

pkofod commented Feb 15, 2016

Evizero commented Feb 15, 2016

Evizero commented Feb 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkofod commented Feb 17, 2016

pkofod commented Feb 22, 2016

johnmyleswhite commented Feb 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnmyleswhite commented Feb 23, 2016

pkofod commented Feb 23, 2016

Evizero commented Feb 23, 2016

KristofferC commented Feb 23, 2016

mlubin commented Feb 23, 2016

pkofod commented Feb 23, 2016