-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ESSOptimizer: Fix bug in go-beyond #1480
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #1480 +/- ##
===========================================
- Coverage 82.91% 82.90% -0.02%
===========================================
Files 163 163
Lines 13786 13786
===========================================
- Hits 11431 11429 -2
- Misses 2355 2357 +2 ☔ View full report in Codecov by Sentry. |
Fixes some issues related to copies vs views, that stopped the go-beyond search prematurely.
fcb82b3
to
dec6b6c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks. I added some questions for my own clarification 🙏🏼
@@ -583,9 +583,9 @@ def _go_beyond(self, x_best_children, fx_best_children): | |||
continue | |||
|
|||
# offspring is better than parent | |||
x_parent = self.refset.x[i] | |||
x_parent = self.refset.x[i].copy() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do i see correctly that the problem was that x_parent/child
are edited further below and thus edit the reset itself?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem was not so much editing the refset per se, but that in the second inner iteration, x_parent
would become identical to x_child
.
@@ -560,7 +560,7 @@ def _do_local_search( | |||
def _maybe_update_global_best(self, x, fx): | |||
"""Update the global best value if the provided value is better.""" | |||
if fx < self.fx_best: | |||
self.x_best = x[:] | |||
self.x_best[:] = x |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how is this related to the go-beyond algorithm?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not. But it's another potential (non-)copying issue.
Fixes some issues related to copies vs views that stopped the go-beyond search prematurely after the first successful step.