-
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
SacessOptimizer: More efficient saving of intermediate results #1529
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## develop #1529 +/- ##
===========================================
- Coverage 84.35% 84.33% -0.02%
===========================================
Files 163 163
Lines 14013 14034 +21
===========================================
+ Hits 11820 11836 +16
- Misses 2193 2198 +5 ☔ View full report in Codecov by Sentry. |
0220e10
to
08dc47f
Compare
08dc47f
to
43388f3
Compare
Hi, could somebody please review? |
@@ -683,20 +692,14 @@ def run( | |||
# perform one ESS iteration | |||
ess._do_iteration() | |||
|
|||
if self._tmp_result_file: |
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.
Another hyper parameter like trace_save_iter
for ESS?
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.
Not really. This class is not intended to be used directly, only through SacessOptimizer
. In that case, this will always evaluate to True. It's rather for unit testing purposes where one doesn't want/need any intermediary result files.
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 was that a request to make saving intermediate result optional in SacessOptimizer
? (Can be done, but I'd do that separately then).
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.
Yes @dweindl sorry, shaky mouse. Don't see it be super urgent to add this, but agreed, add separately.
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.
Remaining looks good. Thanks!
Don't write old local optima again and again in every iteration to save time and avoid huge fragmented files. Some other smaller modifications/fixes: * make sacess_history plotting more robust. * Set SacessOptimizer.exit_flag
43388f3
to
41e91a8
Compare
Don't write old local optima again and again in every iteration to save time and to avoid huge fragmented files.
Some other smaller modifications/fixes: