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

epic: export and import fit result #159

Merged
merged 21 commits into from
Jan 4, 2021
Merged

epic: export and import fit result #159

merged 21 commits into from
Jan 4, 2021

Conversation

redeboer
Copy link
Member

@redeboer redeboer commented Nov 13, 2020

Closes #99

redeboer and others added 3 commits November 12, 2020 11:36
Adds a tf file logging decorator that can be used in Optimizers to
add log variables. The decorator wraps a model updating function.

Co-authored-by: Remco de Boer <redeboer@gmx.com>
Co-authored-by: sjaeger <sjaeger@ep1.rub.de>
@redeboer redeboer added 💡 Enhancement Epic Collection of issues labels Nov 13, 2020
@redeboer redeboer changed the title epic: export and import fit result epi: export and import fit result Nov 13, 2020
@redeboer redeboer changed the title epi: export and import fit result epic: export and import fit result Nov 13, 2020
redeboer and others added 2 commits November 16, 2020 18:17
* fix: only extract  docs version if available
* docs: describe how to start TensorBoard
* fix: remove tensorboard call
* style: apply isort
* feat: implement YAML optimize callback
* docs: rewrite fit usage page

Co-authored-by: Remco de Boer <redeboer@gmx.com>
@redeboer redeboer marked this pull request as ready for review November 18, 2020 09:40
@redeboer
Copy link
Member Author

I set this PR to ready for review, although #165 is still in the epic. We have to decide how urgent that issue is and whether it's the right way to go.

@codecov
Copy link

codecov bot commented Nov 18, 2020

Codecov Report

Merging #159 (2fc8bdb) into master (06f9d51) will increase coverage by 6.70%.
The diff coverage is 80.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #159      +/-   ##
==========================================
+ Coverage   76.17%   82.88%   +6.70%     
==========================================
  Files          12       13       +1     
  Lines         596      701     +105     
  Branches       84       96      +12     
==========================================
+ Hits          454      581     +127     
+ Misses        107       87      -20     
+ Partials       35       33       -2     
Flag Coverage Δ
unittests 82.88% <80.95%> (+6.70%) ⬆️

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

Impacted Files Coverage Δ
src/tensorwaves/optimizer/callbacks.py 76.28% <76.28%> (ø)
src/tensorwaves/optimizer/minuit.py 97.56% <96.55%> (+76.34%) ⬆️
...ensorwaves/physics/helicity_formalism/amplitude.py 73.58% <0.00%> (+0.75%) ⬆️
src/tensorwaves/data/generate.py 96.29% <0.00%> (+18.51%) ⬆️
src/tensorwaves/estimator.py 94.73% <0.00%> (+42.10%) ⬆️

src/tensorwaves/optimizer/callbacks.py Outdated Show resolved Hide resolved
src/tensorwaves/optimizer/callbacks.py Outdated Show resolved Hide resolved
src/tensorwaves/optimizer/callbacks.py Outdated Show resolved Hide resolved
src/tensorwaves/optimizer/callbacks.py Outdated Show resolved Hide resolved
@redeboer redeboer mentioned this pull request Nov 18, 2020
@redeboer
Copy link
Member Author

Notice that the build on RTD fails if the notebooks are run
https://readthedocs.org/projects/tensorwaves/builds/12372875

This could be because the fit is now run twice (albeit with fewer free parameters), although it doesn't seem like the runtime increased that significantly. Compare e.g. the CI for #159
https://github.com/ComPWA/tensorwaves/pull/159/checks?check_run_id=1423128675
with the one for this PR
https://github.com/ComPWA/tensorwaves/runs/1419960001?check_suite_focus=true

Both are around 3min

@redeboer redeboer added this to the Release 0.1.3 milestone Nov 19, 2020
* ci: increase minimal coverage to 80%
* fix: avoid deprecation warning iminuit
* fix: remove pytest color output VSCode
* test: add additional resonance to fixture
* test: generate data in fixtures
sebastianJaeger and others added 3 commits November 20, 2020 17:37
Co-authored-by: Remco de Boer <redeboer@gmx.com>
* fix: type hint Minuit2.optimize
* fix: temporary fix for #171
* refactor: change fit result dict structure
* test: extract test_estimator
* fix: remove progress bar decorator
* style: use alphabetical order Callback definitions
* refactor: rename finalize to on_function_call_end
* refactor: rename __call__ to on_iteration_end
* refactor: use logs in on_iteration_end
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.

Good job everyone! A general remark about the tests. We have to make sure we test things a bit more thoroughly, especially also edge cases. The test we have currently just create an artificially high test coverage, which I find quite dangerous.

So we have to make some improvements on that regard. That can go into different PRs though. Possibly also good issues for @Leongrim and @sebastianJaeger to get acquaint with.

tests/conftest.py Outdated Show resolved Hide resolved
@redeboer redeboer merged commit cd560fa into master Jan 4, 2021
@redeboer redeboer deleted the epic/99-export-fit branch January 4, 2021 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Epic Collection of issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add functionality to export and import fit result
3 participants