SciPEP: optimize: class based Optimizers

[andyfaff]

This PEP proposes that the scipy scalar minimizers are rewritten in class based form. See also #8414.
@stsievert

Mailing list discussion: https://mail.python.org/pipermail/scipy-dev/2018-February/022443.html

[rgommers]

May I suggest that for all SciPEPs we use "This document has been placed in the public domain." (like for PEPs)?

[rgommers]

File naming: I'd suggest for a filename scipep-001.rst. That gives us 1000 numbers that will order correctly (which is more than we'll need I imagine) and gives a consistent naming scheme.

[rgommers]

Optimizer makes sense as a name; it would be good to replace Function with something more specific

[andyfaff]

The #8328 is using ScalarFunction and VectorFunction, but not making them public. Those classes are very similar to what is proposed for this approach, but there are some technical differences in current implementation which would have to be ironed out. For example, tracking number of function evaluations, how some memoization is carried out, etc.

[rgommers]

some more housekeeping: this doesn't render quite right in https://github.com/andyfaff/scipy/blob/8fcd7d98c5fa1fddd765b08e4426d4b493f48420/doc/source/scipeps/1-Optimizer.rst

[andyfaff]

Is there a quick way of rendering the document on my computer to see if I am improving things?

[rgommers]

There is if we'd actually add it to the html docs in this PR. Although rebuilding the docs isn't really faster than pushing to Github and viewing the reST rendering. There are also tools to locally view rendered reST, but I don't use those so don't have a good recommendation.

[lagru]

You could paste your code into an online reST editor like http://rst.ninjs.org/ if you want to quickly check for formatting & rendering errors.

[stsievert]

Thanks for that @lagru. I think I resolved the issues in 00a6446

[andyfaff]

Something went wrong with git here @stsievert.

[stsievert]

Resolved, and the formatting is still fixed.

[stsievert]

What's the next step for this?

[andyfaff]

Yes, it's lain fallow for a while. @rgommers, given that this is the first PEP for scipy I wonder if your experience with NEPs gives any insight in how we go about discussing and merging this?

[rgommers]

I have added a link to the first mailing list discussion on this proposal in the PR description.

There's a couple of things to do I think:

1. The NumPy NEP format/process/machinery has stabilized, so let's copy that for SciPy.
2. Once this proposal is converted to the same format, let's merge it with status draft.
3. Then, I think it requires a second round of discussion on the mailing list and a decision.

There's probably also a few points from the previous discussion to address. For example, the proposal asserts that maintenance costs will be much reduced, however @pv argued that they may actually be increased plus a significant rewrite for reverse-communication style may be needed.

[andyfaff]

To clarify the next two steps are:

1. I should raise another PR copying the NEP structure in numpy/doc/neps.
2. Once that's merged make sure that we're in the same format as a NEP.

Then discussion, etc.

With respect to the maintenance costs, we discuss how the maintenance costs should change. @pv mentions that maintenance costs could increase due to reverse communication style, and that coding becomes more complex.

As far as I understand 'forward communication style' means that the parameter list fully specifies the problem, and reverse communication style presumably means the opposite, i.e. some parameters are held in attributes.
My argument for that is that the existing interface would be unchanged, e.g. fmin and minimize('nelder-mead') would continue to work as-is, i.e. the forward communication style would be retained there. The object oriented Optimizer would allow the specification of the required arguments in the constructor, which would become attributes.

In my experience writing DifferentialEvolutionSolver in an object oriented form (along the lines of what the PEP proposes) was straightforward and is quite powerful in its simplicity.
In this PEP inheritance means a degree of boilerplate can be abstracted, and only the core looping functionality needs to be transferred. The process is straightforward, I was able to convert NelderMead, lbfgsb, bfgs, DifferentialEvolution in a very short time frame, with all tests passing. OO means that testing all solvers for common behaviour is much simpler (e.g. checking that callbacks work).
The last thing we'd want to happen is that the maintenance burden, or adjustment of code would become more complex - that would be the last thing I'd want to happen. If we know in more detail what the complexity concerns are we could attempt to answer them.
I would suggest that the OO spline classes that are based on PPoly are a similar kind of situation.

[rgommers]

To clarify the next two steps are:

1. I should raise another PR copying the NEP structure in numpy/doc/neps.
2. Once that's merged make sure that we're in the same format as a NEP.

Yes, that's what I was suggesting.

[lucascolley]

I believe the most recent comment about this is (from @andyfaff in gh-18742):

it would be nice to have an iterable solver interface, which would be best served by a rebuilt OO interface (i.e. making the class public). I would still like to have a rebuilt OO interface, but it's difficult to drag everyone there. I think the best way to do that would be to have a separate OO project, which could get merged if people like it.

[jonas-eschle]

Hi @andyfaff , I've seen that this is around since a long time, and I would be willing to help to push this forward (or even create a standalone pkg, following the "specialized package" way).

Overall, what the SciPEP wants seems very close to what we independently implemented in zfit, a general purpose fitting library built to be powerful enough to give the user control and separater things (plus to be fast using jitted backends). Making this design, as it is about here, available to more users would be great!

I see that there has been a lack of progress recently, but if you're up to, I am gladly in to push and polish this further, so just to understand what's your take on the PR and time in the near future, not to have an asynchronous back-and-forth over a long period of time.

Content wise, I think there is one thing that was not so much discussen in the whole proposal, the stopping criterion. While some minimizers have these built in, I think there should also be a certain object that handles that. Currently, minimizers are (extremely) hard to compare in their performance, as they use vastly different criteria. But something I am gladly up to discuss, such as the rest of the PR

[andyfaff]

I think the way to go on this is would be to make a separate package. After a period of refinement the aim would be to reincorporate it into scipy. However, there's no guarantees on that, only if it was shown to have demonstrable benefit.

A period of refinement would allow the class design to be polished. I don't know how much improvement could be made over the method backends for speed, that would involve quite a bit of rewriting. If that were to happen we'd be wanting that work to be already folded into the existing minimize methods. If you have some thoughts on how to improve performance we'd be willing to hear.
All I envisaged was lifting the core out and putting the main iteration loop into a __next__ method.

I wasn't thinking of having a method that would do all the iteration, only exposing the next method so that the user can control how the minimisation proceeds. There could then be a converged property that would reflect the original stopping criterion.

[jonas-eschle]

I think the way to go on this is would be to make a separate package. After a period of refinement the aim would be to reincorporate it into scipy. However, there's no guarantees on that, only if it was shown to have demonstrable benefit.

Sounds reasonable, and also there is maybe not even a need to incorporate this back (scipy is great, but maybe rather a bit too large in terms of scope).

Are you interested/have time to tackle this and see if we find common ground and a way forward?

All I envisaged was lifting the core out and putting the main iteration loop into a __next__ method.
I wasn't thinking of having a method that would do all the iteration, only exposing the next method so that the user can control how the minimisation proceeds. There could then be a converged property that would reflect the original stopping criterion.

Okay, I see! So my view on this:
The next approach seems great to me if the minimizer allows for it (technically, it should as long as the previous state is given), but that maybe needs significant code changes in the implementations (and also for probably other black boxes, I had troubles in other libraries to even get a state from the result, such as the hessian/etc). So it seems to me a very nice thing to have, but also not per se necessary:
Most cases I am aware of (this may the limited) is about finding some (usually global, deep learning may be an exception here) minima, and we want to have an algorithm, that does it for us. While the iteration is a nice feature to help finding this minimum, a black-box call to minimize that will return the actual minimum seems the more crucial point for the vast majority of uses-cases (how often do people iterate a minimizer manually and check the minimum? Less than one percent in my felt experience, but that may differs).

So iteration seems nice, but just a consistent API (one API, the class constructor, to configure the algorithm, different for each one; one API to minimize the function, the same for each one) to minimize a function seems to me the most pressing issue.

Needless to say that any next minimizer can always be expressed with at minimize (or whatever name); not the other way around