Merge IterationType and EvaluationType, simplify Method interface #123
Conversation
vladimir-ch
force-pushed
the
remove-evaluationtype
branch
2 times, most recently
from
c044137 to
4600f07
Compare
|
I like RequestType, but maybe it's a little too passive? It's not really a request, as the Method will get back what it wants. Perhaps just "Operation"? The comments around evaluate are good, but the important part to add the information is on Method. |
|
Reading your other comment now. |
vladimir-ch
force-pushed
the
remove-evaluationtype
branch
2 times, most recently
from
20c5a76 to
f351ca9
Compare
|
I like I think this is ready for a real review. This PR is quite big, but such a fundamental change cannot be done in small steps. The core changes are in types.go, interfaces.go, local.go and linesearch.go. I will appreciate suggestions for improving the docs. @btracey @kortschak Please take a look. @btracey Updating NelderMead was easy in the end, the code is well-structured, so it was enough just to add an extra iteration type and a case to a switch. In NelderMead the benefit of this PR is probably most visible. Instead of returning unclear SubIteration and MinorIteration, we simply return FuncEvaluation and when the best value in the simplex could have possibly changed, MajorIteration is returned before proceeding to the reflection step. In both cases the effect is clear even without knowing the context in which the method is used. |
| // Local finds a local minimum of a function using a sequential algorithm. | ||
| // In order to maximize a function, multiply the output by -1. | ||
| // Local finds a local minimum of a minimization problem using a sequential | ||
| // algorithm. |
There was a problem hiding this comment.
I think it's worth saying something along the lines of "A maximization problem can be transformed into a minimization problem by multiplying the function by -1". A lot of 'commercial' solvers allow users to specify either maximize or minimize, so many users new to optimization don't realize the transformation is that easy.
| // designed such that they can be reused in future calls to Local. | ||
| // | ||
| // If method implements Statuser, method.Status() is called before every call | ||
| // to method.Iterate(). If the returned Status is not NotTerminated or the |
|
I'm reviewing from a position of ignorance in most of this, so I focussed on wording and code structure. I should know more, but I don't - it would be nice when the API has stabilised to have some wiki pages outlining use cases (examples are probably likely too long to include, but if you think you can do that, that would be good too). LGTM FWIW (after Brendan's suggestions addressed.) |
vladimir-ch
force-pushed
the
remove-evaluationtype
branch
from
f351ca9 to
eaccadf
Compare
Closes #29
vladimir-ch
force-pushed
the
remove-evaluationtype
branch
from
eaccadf to
0cb4899
Compare
|
@kortschak Thanks for taking a look! I will postpone dropping the parenthesis to another PR. It occurs also at other places in the package, so I will remove them all in one pass later. @btracey I have addressed your comments. Mostly I took your suggestions for the documentation literally, it was really helpful, they are great. You should look again at what I wrote for Method, especially the first paragraph, and for Operation. After this the main changes to be reviewed are in LinesearchMethod (major changes) and NelderMead (minor changes). The rest is just noise coming from the merge IterationType+EvaluationType => Operation and from the modified Method interface. |
| first bool // Indicator of the first iteration. | ||
| nextMajor bool // Indicates that MajorIteration must be requested at the next call to Iterate(). | ||
|
|
||
| loc Location // Storage for intermediate locations. |
There was a problem hiding this comment.
When you get here, @btracey , this is the key point in the discussion about preserving the contents of Location. If we are ok with saying that LinesearchMethod must receive the non-evaluated fields of Location unchanged, then this field can go away and we save (in some cases many) unnecessary copies of Gradient (and Hessian in case of Newton). Instead, we could build the location gradually in the Location passed to Init and Iterate.
For example conjugate gradient produces badly scaled directions and the line searches rarely finish in one iteration. Every point that does not satisfy Armijo or Wolfe means an extra copy. For expensive functions probably not significant, but still ...
There was a problem hiding this comment.
I'm okay with keeping the values in Linesearch. I would like for it to happen in the next PR as this one is already huge (I understand the reason, but we don't need to make it bigger).
| // data and their representation, and enables automation of common operations | ||
| // like checking for (various types of) convergence and maintaining statistics. | ||
| // | ||
| // A Method can command an Evaluation or a MajorIteration operation. |
|
LGTM I think with the few minor cleanups. |
|
Thanks! This is a very nice fix to the major problem in the package design. |
A proposal for resolving #103
but still work in progress.IterationTypeandEvaluationTypeintoRequestTypeOperation.Methodinterface. It can't be any simpler than this, so if this PR is accepted, further changes are very unlikely.LinesearchMethodandminimize(). But the issue at hand is solved, and the code is now much cleaner and more correct. I amquitevery happy with the changes (if nobody praises you, you must praise yourself).MethodandOperation, although while at it, I updated the docs also at other places, which, as a by-product, resolves issues Change Bfgs to BFGS in docs for Local() #29 and Statuser docs are not accurate #96.@kortschak @btracey Please take a look.