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

refactor: MLE API #690

Merged
merged 85 commits into from
Dec 24, 2019
Merged

refactor: MLE API #690

merged 85 commits into from
Dec 24, 2019

Conversation

lukasheinrich
Copy link
Contributor

@lukasheinrich lukasheinrich commented Dec 16, 2019

Description

changes the optimizer API to only be about minimization. Actual fitting (in the sense of maximum likelihood estimation) is moved to pyhf.infer.mle

#531
#687

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Simplify optimizer API to only have 'minimize()'
* Add option to return minimized function in 'minimize()'
* Add optin to return uncertainties in the parameters for 'minimize()'
* Move concrete fits that are needed for maximum likelihood estimation / inference to pyhf.infer.mle

@lukasheinrich
Copy link
Contributor Author

This works then

screenshot

@codecov
Copy link

codecov bot commented Dec 16, 2019

Codecov Report

Merging #690 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #690      +/-   ##
==========================================
+ Coverage   95.15%   95.17%   +0.01%     
==========================================
  Files          50       51       +1     
  Lines        2746     2754       +8     
  Branches      385      390       +5     
==========================================
+ Hits         2613     2621       +8     
  Misses         88       88              
  Partials       45       45
Flag Coverage Δ
#unittests 95.17% <100%> (+0.01%) ⬆️
Impacted Files Coverage Δ
src/pyhf/optimize/__init__.py 100% <ø> (ø) ⬆️
src/pyhf/optimize/opt_pytorch.py 100% <ø> (ø) ⬆️
src/pyhf/infer/mle.py 100% <100%> (ø)
src/pyhf/infer/test_statistics.py 100% <100%> (ø) ⬆️
src/pyhf/infer/utils.py 100% <100%> (ø) ⬆️
src/pyhf/optimize/autodiff.py 100% <100%> (ø) ⬆️
src/pyhf/optimize/opt_minuit.py 100% <100%> (ø) ⬆️
src/pyhf/optimize/opt_scipy.py 100% <100%> (ø) ⬆️
src/pyhf/optimize/opt_tflow.py 100% <100%> (ø) ⬆️

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 3a2a1d0...26381ae. Read the comment docs.

@matthewfeickert matthewfeickert added the refactor A code change that neither fixes a bug nor adds a feature label Dec 16, 2019
@lukasheinrich lukasheinrich requested review from kratsg and matthewfeickert and removed request for kratsg December 16, 2019 16:09
@lukasheinrich
Copy link
Contributor Author

the only reason CI is failing is due to an addition of a new file as part of the refactoring

@matthewfeickert
Copy link
Member

the only reason CI is failing is due to an addition of a new file as part of the refactoring

Later today I can work on having Codecov only fail if there is a greater than 0.2% change or something like that.

@matthewfeickert matthewfeickert added the API Changes the public API label Dec 19, 2019
Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

This is all looking good! If you can take a look at the Codefactor issue and then make a call either way on it that would be nice. I think I'm going to need a bit more time to think about the discussion around fval, but perhaps @kratsg has had time for additional thoughts here.

Copy link
Contributor

@kratsg kratsg left a comment

Choose a reason for hiding this comment

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

I still think return_fval, and fixed_values need to be changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes the public API refactor A code change that neither fixes a bug nor adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants