Skip to content
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

feat: add intermediate result YAML saving #154

Merged
merged 6 commits into from
Nov 18, 2020

Conversation

sebastianJaeger
Copy link
Contributor

@sebastianJaeger sebastianJaeger commented Nov 11, 2020

I added a functionality that regularly saves intermediate fit results, so not all of the progress is lost upon abortion of the fit or a death of the process.

Closes #158

@redeboer redeboer changed the title "Add intermediate result saving" feat: add intermediate result saving Nov 11, 2020
@codecov
Copy link

codecov bot commented Nov 11, 2020

Codecov Report

❗ No coverage uploaded for pull request base (epic/99-export-fit@f4267a4). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                  Coverage Diff                  @@
##             epic/99-export-fit     #154   +/-   ##
=====================================================
  Coverage                      ?   71.54%           
=====================================================
  Files                         ?       12           
  Lines                         ?      608           
  Branches                      ?       87           
=====================================================
  Hits                          ?      435           
  Misses                        ?      142           
  Partials                      ?       31           
Flag Coverage Δ
unittests 71.54% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Member

@spflueger spflueger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sebastianJaeger Thanks for the feature! I added a few comments.

One general thing is, that it is rather hard to test this functionality nicely. But that's more of a design issue, and I would propose to implement this functionality in a decorator (the decorator can be tested with a mock quite nicely).

If you do now have fun doing this, I can understand. But this would be a perfect time to test our new branching model in action. The idea is to push this code into a new "feature" branch. In this branch the "code quality" are not as strict as in the master branch, but allow quick additions of new features. Then those branches can be refactored and merge with the master later on. @redeboer can tell a bit more about the details

src/tensorwaves/optimizer/minuit.py Outdated Show resolved Hide resolved
src/tensorwaves/optimizer/minuit.py Outdated Show resolved Hide resolved
Comment on lines 87 to 91
output = {
"Parameters": {
name: float(value) for name, value in parameters.items()
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably just dump the dictionary as it is, without wrapping it in the "Parameters" (That's a bit of a preference though). I probably would put parameters in the automated temporary filename. Maybe @redeboer has an opinion about this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still wrapped the parameters in a subsection, because there's also a "Time" section and "EstimatorValue".

src/tensorwaves/optimizer/minuit.py Outdated Show resolved Hide resolved
@redeboer
Copy link
Member

redeboer commented Nov 12, 2020

Here are a few thoughts:

This would be a perfect time to test our new branching model in action. The idea is to push this code into a new "feature" branch. In this branch the "code quality" are not as strict as in the master branch, but allow quick additions of new features. Then those branches can be refactored and merge with the master later on. @redeboer can tell a bit more about the details

Since this concerns quite a few issues, I created a branch 99-export-fit that is to address #99. This has become the target branch of this PR.

@redeboer redeboer changed the base branch from master to epic/99-export-fit November 12, 2020 10:42
@redeboer redeboer force-pushed the Add-intermediate-result-saving branch from 67d149c to 5bd0d7c Compare November 12, 2020 10:47
@redeboer redeboer changed the title feat: add intermediate result saving feat: add intermediate result YAML saving Nov 13, 2020
@redeboer redeboer marked this pull request as draft November 16, 2020 14:14
@redeboer redeboer self-assigned this Nov 16, 2020
@redeboer redeboer force-pushed the Add-intermediate-result-saving branch from 5bd0d7c to 59bbd90 Compare November 17, 2020 10:20
@redeboer redeboer marked this pull request as ready for review November 17, 2020 10:21
@redeboer redeboer force-pushed the Add-intermediate-result-saving branch from 59bbd90 to 536f6a0 Compare November 17, 2020 10:24
@redeboer
Copy link
Member

I updated this PR a bit with the new callback syntax (#164). Now it should also close #158, as illustrated in the notebook.

@sebastianJaeger, have a look here for the new implementation
2d621a5?file-filters%5B%5D=.py#diff-9f533f5231a0e5fcd84806765d6bbf0282451172465fee983ac5df640128cceeR34-R77

@redeboer redeboer requested a review from spflueger November 17, 2020 10:28
@redeboer redeboer added this to the Release 0.1.3 milestone Nov 17, 2020
Copy link
Member

@spflueger spflueger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all looks good. Just some small remarks that are not urgent (since its only going into the epic branch).

src/tensorwaves/optimizer/callbacks.py Show resolved Hide resolved
src/tensorwaves/optimizer/callbacks.py Outdated Show resolved Hide resolved
src/tensorwaves/optimizer/callbacks.py Show resolved Hide resolved
@spflueger
Copy link
Member

I added some more ideas. But depending on how fast this functionality is needed it can be merged to the epic branch and improved there later on. Hence i approved the PR

@redeboer redeboer merged commit 9969a2d into epic/99-export-fit Nov 18, 2020
@redeboer redeboer deleted the Add-intermediate-result-saving branch November 18, 2020 09:39
redeboer added a commit that referenced this pull request Jan 4, 2021
* ci: add GitHub workflow for epic branches (#144)
* ci: increase minimal coverage to 80%
* feat: add CSVSummary callback (#173)
* feat: add variable logging functionality using TensorFlow (#155)
* feat: implement YAML optimize callback (#154)
* feat: implement Loadable callback (#177)
* feat: log execute time in optimize call (#156 and #164)
* fix: copy initial parameters in optimize call (#174)
* fix: implement temporary solution for #171
* fix: remove pytest color output VSCode
* test: add additional resonance to fixture
* refactor: change fit result dict structure
* docs: use only 3 free parameters
  Speeds up CI and prevents memory problems on Read the Docs

Co-authored-by: sjaeger <sjaeger@ep1.rub.de>
Co-authored-by: spflueger <spfluege@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants