API for fg!

[antoine-levitt]

It is often the case that computing the gradient yields the functional for the same cost. Right now, the objective functions require supplying f and g!, and there is an awful trick in http://julianlsolvers.github.io/Optim.jl/latest/user/tipsandtricks/ to avoid repeatings computations. The code itself mostly seems to use fg!. Is there a particular objection to implementing OnceDifferentiable(fg!, x_seed::AbstractArray) (that creates a wrapper for f and g!) and advising the users to use that instead?

[pkofod]

Would be nice to have an easier was of specifying that you're passing fg! In a lot of the different constructors. As you mention, almost all calls are fg calls, the exception is the case where the gradient isn't used in the method and also the linesearches. I don't think it's true that fg is always free compared to just f or just g, so I'm not in favor of totally removing the current setup, but we should make it easier to pass just fg. Nlsolve does it using helper functions.

[antoine-levitt]

Alright, how about in addition to the existing constructors I add a

function OnceDifferentiable(x0_seed::AbstractArray; f=nothing, g=nothing, fg=nothing, g! = nothing, fg! = nothing)

that adds the required methods based on the arguments it gets? (ie valid inputs are for instance passing f and g!, or passing fg).

[pkofod]

The only problem is that adding keywords with exclamation marks are kind of frowned upon. We recently removed it in the linesearch keyword because quite a few people got odd errors from writing linesearch!=LineSearches.Static(). Is it because you want people to be able to input non-mutating versions?

[antoine-levitt]

That makes sense, but in this case I guess the error will be easy to see (g and g! have different signatures). Yes, there are many ways for the user to specify an objective function (mutating/non-mutating, f and g/f and fg/only fg), and it seems like the best way for the user to do that is through keyword arguments.

[pkofod]

and it seems like the best way for the user to do that is through keyword arguments.

The problem is errors like this

julia> fun(;fg! = nothing) = fg!
fun (generic function with 1 method)

julia> fun(;fg!=234)
ERROR: UndefVarError: fg not defined
Stacktrace:
 [1] anonymous at ./<missing>:?

and even worse if it comes from a file... People will wonder why it says that fg is not defined, as they never entered fg, they entered 234!

[pkofod]

@ChrisRackauckas you seem to be using https://github.com/JuliaDiffEq/DiffEqBase.jl/blob/911ec37739c0ea3e6026b7fafa016e377cf6a4b7/src/utils.jl#L13 to detect this automatically, right? Will this fail if people have a function f with several methods that may have simply arisen from experimental work in the repl

[pkofod]

Since this is sort of an advanced feature, would it be so bad to simply implement it as
OnceDifferentiable(nothing, nothing, myfg!)?

edit:
similarly optimize(nothing, nothing, myfg!, x_seed, method, whatever, ....)
edit2:
I know I'm debating with myself, but I think I'll implement this.

[antoine-levitt]

My preference still goes to a fg! argument for simplicity and because the created bugs would not be the end of the world to debug (and we can add a line or two in the documentation to guide users), but I guess the nothing version is fine: it's slightly jarring with the rest of the constructors, but at least it doesn't require a workaround like the one currently described in the docs.

[pkofod]

My preference still goes to a fg! argument for simplicity

Yeah, well.. Practical experience shows that people do make the mistake, and the error message is not good to put it mildly.

but at least it doesn't require a workaround like the one currently described in the docs.

Hm, that workaround is mostly useless as we actually mostly use fg! as you already stated, and we now check if the x has already been evaluated.

[pkofod]

Alternatively we can have an exported module called Objectives that will have something like

optimize(Objectives.fg!(myfg), x0, method, ...)
optimize(Objectives.fg(myfg), x0, method, ...)

in there, where fg!/fg just wraps the function in a way for us to handle internally...

[ChrisRackauckas]

I think keyword arguments are fine here, just don't add the !. I would make it

OnceDifferentiable(x0_seed::AbstractArray; f=nothing, g=nothing, fg=nothing)
OnceDifferentiable{inplace}(x0_seed::AbstractArray; f=nothing, g=nothing, fg=nothing)

where inplace is a boolean for whether things are inplace or not. I don't think you need to do it function by function. Normally if the user is going to make things inplace, they can do it for all (and if they can't, e.g. for static arrays, then they can't do it for any). Then yes, there are ways to automatically determine inplace by reading the methods table.

Will this fail if people have a function f with several methods that may have simply arisen from experimental work in the repl

Well, maybe. For example, for ODEs it labels it inplace if there is any method with 3 or more arguments, ignoring the special forms for declaring Jacobians and the like. First of all, inplace is recommended and pretty much default, so that gets it right most of the time anyways. Out-of-place is more of a special case (solving equations with static arrays), so how many times will it show up that somebody experimented with adding more dispatches to f and then decided they want to do it not inplace, but then also won't set the boolean? No one has opened an issue for it yet, so I assume that case (which I know exists) hasn't caused a problem in the wild.

[pkofod]

An alternative could be

optimize(fg,  x0, method, options; only_fg = true, in_place = false)
optimize(fg!, x0, method, options; only_fg = true)  # in_place = true
optimize(f,   x0, method, options, in_place = false) # only_fg = false
optimize(f!,  x0, method, options) # only_fg = false, in_place = true

where defaults are then given by the comments in the last line. We could reintroduce the autodiff keyword here as well, which is probably not a bad idea. They would be mirrored in the NDifferentiable constructors, such that these keywords for optimize should simply be understood as a shortcut to creating an NDifferentiable yourself.

I think this could be a relatively good way of avoiding the regular user even knowing about NDifferentiable as it's not really that interesting for the user who just passes a function, or a set of functions, and want to minimize something once. A maximize (and maybe scale) keyword could be added like this as well, as this is a functionality that's been requested a few times.

For the performance demaning users who really want to keep the caches (that I want to experiment moving out anyway) because they're solving some problem over an over, can then create an NDifferentiable and an NDifferentiableCache instance themselves.

(because you're not a participant yet: @anriseth how does this suggestions sound to you?)

[anriseth]

optimize(f!, x0, method, options) # only_fg = true, in_place = true

Did you mean to have only_fg=false here?

I don't understand how in_place is meant to work for the objective?

Some notes, just in case they haven't been considered.

• There are many ways to scale things (different scaling in each input element, output space), so scale may be a bit too complicated to have as a keyword.
• I'm a fan of having access to *Differentiable, so a second round to improve that with NDifferentiable and NDifferentiableCache can be nice.
• Fminbox requires *Differentiable.
• Let's make sure the new objects also work well with the constrained optimization Constrained optimization episode 2: revenge of the slack variables #303 (if it ever gets rebased and merged 😮 )

[pkofod]

Did you mean to have only_fg=false here?

yes, edited

I don't understand how in_place is meant to work for the objective?

Yeah, actually it probably only makes sense for g unless we tell people specifically that a non-in-place fg should return (f, g). But you're probably asking for the f case, and that was a bit stupid, but I actually had nlsolve in mind there. They're going to be sharing types going forward.

There are many ways to scale things (different scaling in each input element, output space), so scale may be a bit too complicated to have as a keyword.

Yes, it's not really my priority anyways, but...

[pkofod]

Anyway, I'll have PRs for NLSolversBase, Optim, and NLsolve as soon as I have fixed the lasts tests in NLsolve (basically just need to use the new syntax). Then we can discuss the specifics.

[pkofod]

So refactoring something like NDifferentiables makes you question certain past design choices. I think this design choice of making it easier to pass f and g separately may have made sense from a "I guess I should provide the objective and gradient evaluators" but as @antoine-levitt noted above, we basically always use fg!. If we don't, then it's because we're either in a situation where there is no gradient (NelderMead,...) or in some line search cases.

Seeing as we now have inter-procedural constant propagation, it would basically be free to have users provide

function fg!(G, x, calc_grad)
    if calc_grad
        # do grad stuff in-place of course
    end
    # do f stuff
end

This is now free because in value!(df, x) we would then do

value!(df, x) = value_gradient!(df, x, false)

such that the branch would be completely compiled away for value! purposes - unless I've misunderstood the constant propagation thing (anyway, I doubt that branch would have devastating consequences for any time of optimization you could cook up, even extremely(!) contrived examples.).

The only problem is that you objective function would need three positional arguments where two of them are not used in the case of NelderMead, SimulatedAnnealing, and ParticleSwarm...

Additionally, I think that for many cases in NLSolve, it makes sense to evaluate the F(x) vector and Jacobian separately.

[ChrisRackauckas]

That last choice is very similar to NLopt, except it just either gives an empty array for the grad output or not.

[pkofod]

Ah yes of course. We could just either do that or pass nothing. So that is the case also for their neldermead?

[ChrisRackauckas]

Yup, their derivative free methods just never pass a pre-allocated output for the gradient, so that branch is effectively ignored. I like the nothing thing though.

[cortner]

Two comments:

• it would be very nice for users to offer a constructor that takes f, g instead of f, g!, as far as I can see the new PR for NLSolversBase doesn't allow this?
• From a readability perspective: dispatch should make it invisible to the Optim code whether the objective is better evaluated via fg! or via f, g!. What I mean is Optim should maybe stop using fg! altogether and only ever call f and g (probably not even g!) and the NDifferentiable can take care of everything else?

[pkofod]

as far as I can see the new PR for NLSolversBase doesn't allow this?

no but it certainly could, the original motivation for this PR was actually simply to be able to use OnceDifferentiable in both Optim and NLsolve :) It'll remove this abomination from NLsolve

df = DifferentiableGivenSparseMultivariateFunction(f!, g!, J)
nlsolve(df, initial_x)

[cortner]

and given this is ready, you should not wait until there is agreement on different ways to construct OnceDifferentiable, I just thought to mention it that it might be useful to make such changes now or very soon .

[pkofod]

I believe everything here is covered already