Fix autosave function for single core #770

yannikschaelte · 2021-12-06T15:53:21Z

Assert {id} template in CSV histories.
Fix behavior of filename=None for single-core by rewriting pre- and post-processing (filename was ignored / overwritten by None, and thus the default output file always generated)

Actual fix of #743 not done yet.

yannikschaelte · 2021-12-06T17:27:33Z

(applied black to history.py. should stop doing that 🐈‍⬛ )

pypesto/objective/history.py

test/conftest.py

test/base/test_store.py

codecov-commenter · 2021-12-06T17:45:47Z

Codecov Report

Merging #770 (0ec162e) into develop (a3fb58b) will increase coverage by 41.78%.
The diff coverage is 95.74%.

@@             Coverage Diff              @@
##           develop     #770       +/-   ##
============================================
+ Coverage    48.05%   89.84%   +41.78%     
============================================
  Files           98       98               
  Lines         6829     6847       +18     
============================================
+ Hits          3282     6152     +2870     
+ Misses        3547      695     -2852

Impacted Files	Coverage Δ
pypesto/optimize/task.py	`100.00% <ø> (ø)`
pypesto/sample/sample.py	`96.55% <ø> (+58.62%)`	⬆️
pypesto/optimize/util.py	`95.12% <92.59%> (+38.87%)`	⬆️
pypesto/objective/history.py	`95.31% <96.42%> (+48.15%)`	⬆️
pypesto/optimize/optimize.py	`97.95% <100.00%> (+7.95%)`	⬆️
pypesto/optimize/optimizer.py	`90.56% <100.00%> (+4.94%)`	⬆️
pypesto/objective/amici_util.py	`75.43% <0.00%> (-8.78%)`	⬇️
pypesto/objective/amici_calculator.py	`91.66% <0.00%> (-1.67%)`	⬇️
pypesto/store/hdf5.py	`83.33% <0.00%> (+4.16%)`	⬆️
... and 66 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a3fb58b...0ec162e. Read the comment docs.

PaulJonasJost

Not entirely sure what you did in the testing files but looks like a more elegant way of creating temporary hdf5 files 👍

yannikschaelte · 2021-12-06T18:02:42Z

Not entirely sure what you did in the testing files but looks like a more elegant way of creating temporary hdf5 files +1

you mean that hdf5_file argument? That was just as a cleaner solution to those with ... statements, and to fight claustrophobia.

pypesto/optimize/optimize.py

pypesto/objective/history.py

FFroehlich · 2021-12-06T22:30:51Z

pypesto/optimize/util.py

+        return False
+
+    # extract storage type
+    path = Path(storage_file)


FFroehlich · 2021-12-07T14:12:19Z

test/base/test_history.py

@@ -79,13 +86,13 @@ def check_history(self):
                if storage_type is not None:
                    read_results_from_file(
                        self.problem, self.history_options,
-                        len(result.optimize_result.list) + 2
+                        n_starts=n_starts,


As indicated by the docstring here, having n_starts+2 was very much intentional

ah missed the docstring 🙈 problem is that now the csv files each carry an id, therefore files with id=1,2 do not exist. Would it be desirable to allow passing a list of ids (/filenames) to read_results_from_file instead of the single n_starts? then this scenario could be covered here by [0, 0, 0]

I guess silently reading the same file multiple times bc no id is present, is not desirable, unless explicitly requested, thus the {id} check.

but the {id} was there before. This is supposed to check whether read_results_from_file can handle missing files (optimization stopped before file was written, for whatever reason). If we do not wan't this behavior we should add a check whether read_results_from_file fails when the file is not present.

how did the {id} come in before? It's not in the original file name in 6d3226d#diff-80b64c062a6e89e507e7518b03d2cf45dc9518a824f39bae232b565e8e5016afL52, does it come in in processing somewhere?

I thought it just read in the same file filename.csv 3 times in the previous test implementation, therefore all files existed. when adding {id}, then there was a trace is Null error or something.

ah, I supposed the test didn't implement what I intended to then ;)

will fix this in a separate PR

thanks :) it confused me too then. so in conclusion I guess yes we want to be able to merge histories also when some ids are not present (maybe warn then).

minor fixes

e772107

yannikschaelte self-assigned this Dec 6, 2021

Merge branch 'develop' into fix_store

da043ea

yannikschaelte marked this pull request as draft December 6, 2021 15:53

yannikschaelte added 4 commits December 6, 2021 17:11

fix no storage

a6c244f

docs

368d05a

fixup

91e7fa2

fixes

4d5a742

yannikschaelte changed the title ~~Fix hdf5 file distribution~~ Fix autosave function for single core Dec 6, 2021

Merge branch 'develop' into fix_store

2e82fd7

yannikschaelte marked this pull request as ready for review December 6, 2021 17:27

yannikschaelte commented Dec 6, 2021

View reviewed changes

pypesto/objective/history.py Show resolved Hide resolved

yannikschaelte commented Dec 6, 2021

View reviewed changes

pypesto/objective/history.py Show resolved Hide resolved

yannikschaelte requested review from PaulJonasJost, FFroehlich and dweindl December 6, 2021 17:30

yannikschaelte added 2 commits December 6, 2021 18:33

meaningful variable name

c12b48b

fix operator priority

ef083f6

dweindl reviewed Dec 6, 2021

View reviewed changes

pypesto/objective/history.py Show resolved Hide resolved

test/conftest.py Outdated Show resolved Hide resolved

test/base/test_store.py Show resolved Hide resolved

PaulJonasJost approved these changes Dec 6, 2021

View reviewed changes

simplify hdf5 file fixture

23b27a3

FFroehlich approved these changes Dec 6, 2021

View reviewed changes

yannikschaelte and others added 5 commits December 7, 2021 11:24

Merge branch 'develop' into fix_store

bfd5353

fix data +2 error

cd51567

Merge branch 'fix_store' of github.com:icb-dcm/pypesto into fix_store

2816476

apply code review suggestions

209e45d

Merge branch 'develop' into fix_store

0ec162e

yannikschaelte merged commit cd826fa into develop Dec 7, 2021

yannikschaelte deleted the fix_store branch December 7, 2021 12:54

FFroehlich reviewed Dec 7, 2021

View reviewed changes

yannikschaelte mentioned this pull request Jan 7, 2022

Release 0.2.10 #773

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix autosave function for single core #770

Fix autosave function for single core #770

yannikschaelte commented Dec 6, 2021 •

edited

Loading

yannikschaelte commented Dec 6, 2021

codecov-commenter commented Dec 6, 2021 •

edited

Loading

PaulJonasJost left a comment

yannikschaelte commented Dec 6, 2021 •

edited

Loading

FFroehlich Dec 6, 2021

FFroehlich Dec 7, 2021

yannikschaelte Dec 7, 2021

yannikschaelte Dec 7, 2021

FFroehlich Dec 7, 2021

yannikschaelte Dec 7, 2021

yannikschaelte Dec 7, 2021

FFroehlich Dec 7, 2021

FFroehlich Dec 7, 2021

yannikschaelte Dec 7, 2021

Fix autosave function for single core #770

Fix autosave function for single core #770

Conversation

yannikschaelte commented Dec 6, 2021 • edited Loading

yannikschaelte commented Dec 6, 2021

codecov-commenter commented Dec 6, 2021 • edited Loading

Codecov Report

PaulJonasJost left a comment

Choose a reason for hiding this comment

yannikschaelte commented Dec 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yannikschaelte commented Dec 6, 2021 •

edited

Loading

codecov-commenter commented Dec 6, 2021 •

edited

Loading

yannikschaelte commented Dec 6, 2021 •

edited

Loading