-
Notifications
You must be signed in to change notification settings - Fork 85
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: Test statistic specified by string instead of using qtilde kwarg #1231
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1231 +/- ##
=======================================
Coverage 97.45% 97.45%
=======================================
Files 63 63
Lines 3688 3700 +12
Branches 524 524
=======================================
+ Hits 3594 3606 +12
Misses 55 55
Partials 39 39
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
for more information, see https://pre-commit.ci
a7bc38f
to
8010510
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kratsg I really like this PR overall a lot! Thanks for it and nice idea. :) I do have a suggestion on the API though, so let me know your thoughts.
Co-authored-by: Matthew Feickert <matthew.feickert@cern.ch>
Try to make it more explictily clear what this is returning
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should also grab the notebooks and README
in this PR as well
f$ git grep "qtilde=True"
README.rst: >>> CLs_obs, CLs_exp = pyhf.infer.hypotest(test_mu, data, model, qtilde=True, return_expected=True)
README.rst: >>> CLs_obs, CLs_exp = pyhf.infer.hypotest(test_mu, data, model, qtilde=True, return_expected=True)
README.rst: pyhf.infer.hypotest(test_poi, data, model, qtilde=True, return_expected_set=True)
README.rst: pyhf.infer.hypotest(test_poi, data, model, qtilde=True, return_expected_set=True)
docs/examples/notebooks/hello-world.ipynb: " test_mu, data, model, qtilde=True, return_expected=True\n",
docs/examples/notebooks/hello-world.ipynb: " test_mu, data, model, qtilde=True, return_tail_probs=True\n",
docs/examples/notebooks/hello-world.ipynb: " test_mu, data, model, qtilde=True, return_expected_set=True\n",
docs/examples/notebooks/toys.ipynb: " qtilde=True,\n",
src/pyhf/infer/calculators.py: ``qtilde=True`` has been deprecated in favor of ``test_stat="qtilde"``.
src/pyhf/infer/calculators.py: ``qtilde=True`` has been deprecated in favor of ``test_stat="qtilde"``.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the beautiful PR @kratsg. I've made my final changes to also bring this to the README and notebooks. Let me know your thoughts but looks good on my side.
cc @alexander-held RE: a heads up on API shifts in |
Description
This replaces
qtilde=True
andqtilde=False
withtest_stat="qtilde"
andtest_stat="q"
everywhere. At the same time, we keep aroundqtilde
for backwards-compatibility and mark it as deprecated in v0.6.0 . Likely we will remove this in v0.6.1.This is more work towards getting PR #520 in.
ReadTheDocs build: https://pyhf.readthedocs.io/en/feat-flexibleteststatistic/api.html#inference
Checklist Before Requesting Reviewer
Before Merging
For the PR Assignees: