Fix startpoint selection and neval in scatter search #1644
Conversation
Fixes two bugs affecting `ESSOptimizer` and 'SacessOptimize`: * The ranking of candidates for local optimization startpoints was wrong in many cases. Only relevant if a local optimizer was used. * In some cases, objective evaluations were counted twice. Resulting in incorrect reporting of the total number of evaluations and not fully exhausting the objective evaluations budget.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1644 +/- ##
===========================================
+ Coverage 84.28% 84.31% +0.03%
===========================================
Files 163 163
Lines 14136 14134 -2
===========================================
+ Hits 11914 11917 +3
+ Misses 2222 2217 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
PaulJonasJost
left a comment
PaulJonasJost
left a comment
There was a problem hiding this comment.
Thanks 🙏🏼 I don't think I fully comprehended the error. Perhaps a test would be good to assess this also does not break in future versions? But not a must, as I am not sure on the exact benefit of such a test.
| and self.n_iter - self.last_local_search_niter >= self.local_n2 | ||
| ): | ||
| quality_order = np.argsort(fx_best_children) | ||
| quality_order = np.argsort(fx_best_children).argsort() |
There was a problem hiding this comment.
Why is it twice argsort? Seems counterintuitive to me
| The objective function values in the same order as the inputs. | ||
| """ | ||
| res = np.fromiter(map(self.single, xs), dtype=float) | ||
| self.n_eval += len(xs) |
There was a problem hiding this comment.
Not quite sure on the usage, I guess it means in the non MT case, we actually would count too often and don't need to track it in this manner at all? Cause now it is not tracked unless we use FunctionEvaluatorMT
There was a problem hiding this comment.
It's counted inside self.single on the line above. Thus, it was counted twice before, ẃhich is what I want to fix here.
Tests are implemented elsewhere. This is the last scatter search thing I'll update here besides adding a deprecation warning soon (#1641 (comment)). Therefore, I didn't add additional tests. |
Great, happy to merge it then :) |
3 participants
Fixes two bugs affecting
ESSOptimizerandSacessOptimizer: