added option for hdf5 History to read_result_from_file #663
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #663 +/- ##
============================================
+ Coverage 47.57% 90.08% +42.51%
============================================
Files 95 95
Lines 6117 6134 +17
============================================
+ Hits 2910 5526 +2616
+ Misses 3207 608 -2599
Continue to review full report at Codecov.
|
pypesto/objective/history.py
Outdated
| @@ -1115,10 +1118,10 @@ def _compute_vals_from_trace(self): | |||
| # etc | |||
| max_init_iter = 3 | |||
| for it in range(min(len(self.history), max_init_iter)): | |||
| candidate = self.history.get_fval_trace(it) | |||
| candidate = self.history.get_fval_trace()[it] | |||
There was a problem hiding this comment.
why is this necessary? passing it into the function was designed to allow for efficient access, e.g. not calculating the full trajectory, but only that specific index/indices. Not sure if csv/hdf5 use it, but could potentially.
There was a problem hiding this comment.
csv uses it, but hdf5 did not. that's why I changed it for now into this to be able to use the function in both file formats.
There was a problem hiding this comment.
oh, I see. That's a bug in the History class: It seems like when it was introduced, it was not properly propagated down to hdf5, only csv, therefore the function calls there raise a "Signature does not match base class" warning in pycharm. Could you modify the functions in the Hdf5History?
…o feature_result_from_history
…DCM/pyPESTO into feature_result_from_history
There was a problem hiding this comment.
Looks good to me, thanks! @FFroehlich as this affects some of your code, you may want to have a look, but I think we can merge already.
| for storage_type in ['.csv', | ||
| '.hdf5', | ||
| None]: | ||
| with tempfile.TemporaryDirectory(dir=".") as tmpdirname: |
There was a problem hiding this comment.
dir = "." is not necessary, but ok
test/base/test_history.py
Outdated
| # comparing nan and None difficult | ||
| if original_trace[iteration] is None or np.isnan( | ||
| original_trace[iteration]).all(): | ||
| continue |
There was a problem hiding this comment.
better check that the same then also applies to the reconst_ one?
There was a problem hiding this comment.
| continue | |
| assert(reconst_trace[iteration] is None or | |
| np.isnan(reconst_trace[iteration]).all()) |
There was a problem hiding this comment.
like this @yannikschaelte ?
There was a problem hiding this comment.
maybe separate None and nan, then it should be like the previous check
Co-authored-by: Yannik Schälte <31767307+yannikschaelte@users.noreply.github.com>
…DCM/pyPESTO into feature_result_from_history
closes #259.