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.
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)
should be equivalent to your example above (modulo callback).
It's already possible as: while not solver.converged(): step = next(solver) (only the `converged()` is new from this PR.) 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`). In the PR, there have been discussions to expose these steps as public methods to make the use of the context manager optional; I'm fine with this if it makes things clearer (though the use case is not clear to me).
3. My mental model would be that the solver being iterable and the context manager are both a syntax sugar for a pair of methods, roughly `Solver.make_a_single_step_then_pause()` and `Solver.do_full_minimization_until_convergence()` which repeatedly calls the first one. (This might be influenced by a DIY MC code I had a while ago, https://github.com/ev-br/mcba/blob/master/mcba/abstract_walker.py#L80, not sure how well it maps onto this case, so feel free to disregard)
This `make_a_single_step_then_pause` method is just called `__next__` (this is not new). I personally find `step = next(solver)` explicit enough not to require a more specific method, but that's arguable. The `Solver.do_full_minimization_until_convergence()` is basically the (convenience) function `differential_evolution`. There used to be a `solve()` method on the solver class but I dropped it as I did not see any use case where one would want this over just using the convenience function (also arguable).
4. Having the solver class iterable, with or without the context manager, forces the iteration to happen at the python level. This is likely not a problem with the current code, where the whole class is in python anyway. However if some future refactoring moves parts of the implementation to a compiled code, the overhead can get significant. This seems to argue for a black-box `do_a_full_simulation` method. (Again, this might be my MCMC background talking, where a single step is cheap and the python generator overhead is non-trivial.)
Can't really comment on this point. The "problem" already exists in the current implementation AFAICT. Maybe this means we'll ultimately have to move the whole class to a compiled code implementation? Thank you for your feedback.