-
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
Refactor ESSOptimizer #1182
Refactor ESSOptimizer #1182
Conversation
Fixes an log message. The rest is just shuffling some code around for readability / reducing duplications.
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #1182 +/- ##
===========================================
+ Coverage 84.24% 84.33% +0.09%
===========================================
Files 148 148
Lines 11714 11724 +10
===========================================
+ Hits 9868 9887 +19
+ Misses 1846 1837 -9 ☔ View full report in Codecov by Sentry. |
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 to me.
return FunctionEvaluatorMT( | ||
problem=problem, | ||
startpoint_method=startpoint_method, | ||
n_threads=n_threads or 1, | ||
) |
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.
So the default is multithread with one thread. Just out of curiosity, why not Multi process with 1?
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.
I'd expect it to be more or less the same. The mp part would still create a pool of one, the mt part doesn't do any extra.
refset: | ||
The initial RefSet or ``None`` to auto-generate. | ||
""" | ||
): |
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.
A short docstring would still be nice, even if it is an internal _foo
function.
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.
or rather a hint that doc is equivalent to minimize
?
@@ -328,6 +328,7 @@ def submit_solution( | |||
sender_idx: Index of the worker submitting the results. | |||
elapsed_time_s: Elapsed time since the beginning of the sacess run. | |||
""" | |||
abs_change = fx - self._best_known_fx.value |
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.
Naming a bit misleading as you use abs
command below. But no idea for better naming, expect perhaps change
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.
I think gets relatively clear from the context. change
would as ambiguous - (absolute) relative or (absolute) absolute?.
Fixes two log messages. The rest is just some cleanup and shuffling some code around for readability / reducing duplications.