Conversation
Fix bugs: error message typo ("Min not be" -> "Min must not be"),
validation messages using wrong bracket notation ([0,1] -> (0,1) for
exclusive bounds), swapped format args in expectPoint test helper,
mislabeled NaN-Min test case, and missing empty-x0 validation.
Clean up code: replace setZero with clear builtin, rename misleading
reflect method to transformPoint, remove unnecessary buffer zeroing
in replacePoint, remove redundant n parameter from createSimplex,
simplify createSimplex with copy, use min/max builtins in constraint
clamping, and add comments to algorithm steps throughout.
Trim verbose ChatGPT-style doc comments to concise idiomatic Go.
crhntr
force-pushed
the
fix/bounds-and-comments
branch
from
a49d9a3 to
d3a657a
Compare
There was a problem hiding this comment.
Pull request overview
Updates the Nelder–Mead optimizer implementation and its tests, focusing on clearer documentation/comments and tighter validation around bounds/initial conditions.
Changes:
- Improved package/type/function documentation and streamlined some internal helpers.
- Tightened
x0validation (reject emptyx0) and updated tests accordingly. - Refactored iteration internals (preallocated buffers, renamed helpers) and adjusted constraint clamping implementation.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| run.go | Adds/updates docs, tightens x0 validation, refactors iteration helpers/buffering, and adjusts constraint clamping. |
| run_test.go | Fixes expected/actual formatting in a test, updates/cleans test cases to match new validation behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| secondWorst := len(simplex.Points) - 2 | ||
| if reflectedPoint.F < simplex.Points[secondWorst].F { | ||
| // Reflected point is better than second-worst: try expanding further. | ||
| expandedPoint = transformPoint(reflectedPoint, expandedPoint, f, centroid, options.Gamma) |
There was a problem hiding this comment.
The expansion step is computed from reflectedPoint, but transformPoint uses the formula centroid + coeff*(centroid-source). Using the reflected point as source sends the expanded point in the opposite direction of the reflection (e.g., centroid=0, worst=2 -> reflected=-2, current “expanded” becomes +4 instead of -4). Expansion should be computed as centroid + Gamma*(reflected-centroid) (or equivalently from the original worst point with an appropriately derived coefficient), otherwise the algorithm’s expand step is incorrect.
| expandedPoint = transformPoint(reflectedPoint, expandedPoint, f, centroid, options.Gamma) | |
| expandedPoint = transformPoint(simplex.Points[worst], expandedPoint, f, centroid, options.Alpha*options.Gamma) |
| // Reflected point is not better than second-worst. | ||
| if reflectedPoint.F < simplex.Points[worst].F { | ||
| // But it is better than the worst: accept it before contracting. | ||
| simplex.replacePoint(worst, reflectedPoint) | ||
| } | ||
| contractedPoint = simplex.Points[lastPointIndex].reflect(contractedPoint, f, centroid, options.Beta) | ||
| if contractedPoint.F < simplex.Points[lastPointIndex].F { | ||
| simplex.replacePoint(lastPointIndex, contractedPoint) | ||
| // Contract: move the worst point toward the centroid. | ||
| contractedPoint = transformPoint(simplex.Points[worst], contractedPoint, f, centroid, options.Beta) | ||
| if contractedPoint.F < simplex.Points[worst].F { | ||
| simplex.replacePoint(worst, contractedPoint) | ||
| } else { |
There was a problem hiding this comment.
This branch accepts reflectedPoint when it’s better than the worst point, but then immediately performs a contraction step anyway. In standard Nelder–Mead, when the reflected point is between the second-worst and worst, you accept it and skip contraction; contraction is only attempted when reflection is not accepted. Consider restructuring the conditionals so contraction (inside/outside as appropriate) only runs when reflection fails, and base contraction on the correct point (typically the original worst, not the already-replaced one).
| simplex.Points[i].F = f(simplex.Points[i].X) | ||
| } | ||
| sortSimplex(simplex) | ||
|
|
||
| if len(options.Constraints) > 0 { | ||
| ensureXAreInConstraintBounds(simplex.Points[0].X, options.Constraints) | ||
| } |
There was a problem hiding this comment.
ensureXAreInConstraintBounds mutates the best point’s coordinates after all objective values have been evaluated and the simplex has been sorted. If clamping changes simplex.Points[0].X, then simplex.Points[0].F no longer corresponds to that X, and the ordering/convergence check in the next iteration can become inconsistent. To keep X/F consistent, clamp before evaluating f (e.g., clamp candidate points inside transformPoint/after shrink) or re-evaluate F (and re-sort) after clamping any point.
| simplex.Points[i].F = f(simplex.Points[i].X) | |
| } | |
| sortSimplex(simplex) | |
| if len(options.Constraints) > 0 { | |
| ensureXAreInConstraintBounds(simplex.Points[0].X, options.Constraints) | |
| } | |
| if len(options.Constraints) > 0 { | |
| ensureXAreInConstraintBounds(simplex.Points[i].X, options.Constraints) | |
| } | |
| simplex.Points[i].F = f(simplex.Points[i].X) | |
| } | |
| sortSimplex(simplex) |
| // Re-evaluate objective at all vertices (needed after shrink; redundant otherwise). | ||
| for i := 0; i < len(simplex.Points); i++ { | ||
| simplex.Points[i].F = f(simplex.Points[i].X) | ||
| } |
There was a problem hiding this comment.
runIteration recomputes f for every simplex vertex on every iteration even though, outside of a shrink step, only one vertex’s coordinates change. Since transformPoint already evaluates f for reflected/expanded/contracted points, this loop can significantly increase runtime for expensive objectives. Consider tracking whether shrinkSimplex ran and only re-evaluating all vertices in that case; otherwise update just the modified vertex’s F.
No description provided.