On 15 January 2017 at 05:03, Denis Laxalde <denis@laxalde.org> wrote:

Evgeni Burovski a écrit :

Small details, feel free to take or leave:

1. What is `step` in `for step in solver`? If it's a current "state of
the minimizer", then can its contents be retrieved from the solver
object itself (step vs `solver.result` above)? Or is it just a step
number and the solver holds all the information at each step via
`solver.result?` This needs to be specified and documented.

This `step` is an `OptimizeStep`. It's a new class introduced here to
wrap any useful information produced by the solver during one iteration.
In this case, it holds the current `x` and function value along with
iteration number. The idea is to return an extensible object instead of,
say, a tuple that may cause bw compat issues in the future. I think this
is documented, feel free to comment further if things are not clear enough.

Best `x` and best energy are already available as `solver.x` and `solver.population_energy[0]`, but I think this kind of object being returned from `__next__` is a good idea. Does it have to be an `OptimizeStep`, or can we return an preliminary `OptimizeResult`?

2. Since the solver class is iterable, just iterating it without the
context manager should be equivalent. Something like this (pseudocode)

solver = DifferentialEvolutionSolver(...)
while not_converged:
result = next(solver)

Also, the context manager interface is actually orthogonal to the
iteration API. The context manager abstracts out the solver
*initialization* (population initialization and energies calculation)
and *termination* (possible polishing and build of the
`OptimizeResult`).

Whilst the context manager is orthogonal to the iteration API, Evgeni's example would not be possible with the PR is it currently exists - the only way of initialising the solver (population initialization and energies calculation) would be to go through the context manager. Denis suggested that a way around this would be to have a separate initialize_population() method. It's a good idea to have this method (for reasons I outline later). However, if you create the object outside a context manager I don't think that one should have to call an additional initialize_population() method to be able to iterate the object. There are a few reasons for this:

(a) it's an extra line to type

(b) iterating the object without going through the context manager is not equivalent

(c) the solver object possesses public population and population_energies attributes, these attributes need to be present in a newly created object, not be created at a later stage

(d) if the population and population_energies attributes are present I'd argue it makes more sense for them to contain relevant values

(e) given a solver object how does one know if initialize_population has already been called (which would be a common occurrence when used in an interactive interpreter)?

(f) you can't call next(solver) if the population hasn't been initialised. You shouldn't need to know the state of the solver object to be able to iterate it, it should be iterable straightaway.

Currently the initial energy calculations are done in `__init__` and one uses the object as:

# stepwise

solver = DifferentialEvolutionSolver(...)

while not solver.converged:
next(solver)

-or-

# all at once

solver = DifferentialEvolutionSolver(...)

result = solver.solve()

The proposed PR:

with solver as DifferentialEvolutionSolver(...):

while not solver.converged():
next(solver)

result = solver.result

-or possibly-

solver = DifferentialEvolutionSolver(...)

# extra line to type,

solver.initialize_population()

while not solver.converged:
next(solver)

Perhaps the following course could be followed:

(1) make the __next__ method return an OptimizeStep object (or OptimizeResult).

(2) add a polish() method that takes the current best solution and polishes it. Having a separate polish method is useful as you can polish the object at any step, getting functionality similar to optimize.brute.

(3) Add __enter__() and __exit__() methods to allow the object to be used as a context manager. The __exit__() method can use the public polish() method. I'd like to keep the existing solve() method, it can call __exit__() at the end.

(4) population initialization and the initial energy calculation is moved to a public method, initialize_population(). This method is either called from __init__ (the status quo), or it is called on the first call to __next__(), making the first call to __next__ take twice as long. My preference is to keep the status quo because one can create the solver and immediately polish the best population member (c.f. optimize.brute), as well as the points raised above. The rationale for making it public is that once you've done one optimization you can re-initialise it with a different method ('latinhypercube' or 'random') and solve again or you could imagine re-initialising the population with a given population array (which has been requested previously) and start iterating again.