feat: Add discovery p0 test statistic

[kratsg]

Description

Supersedes #520. Particularly it's just a full reimplementation of @nikoladze 's work but on top of more recent pyhf master. I initially tried to rebase, but there's been a lot of changes in the infer API that I felt it was better to pick out all the pieces, and re-implement it accordingly.

To-Do:

• how to handle poi_test gracefully? Should q0 simply warn and set poi_test or should we fail harder? -- see feat: Cleanup internal names for hypotest #1247
  • hypotest should handle more gracefully, but the calculators should not
• how to handle asimov_mu gracefully? Should hypotest or AsymptoticCalculator check if asimov_mu is consistent with the test statistic being used? Should the test statistic functions have a func.default_asimov_mu attribute? -- see feat: Cleanup internal names for hypotest #1247
• what to name all the various "p-values" returned via hypotest? (can't reasonably call it CLs or w/e) [related discussion: feat: customizable metrics in hypotest #966] -- see feat: Cleanup internal names for hypotest #1247
• should AsymptoticCalculator return a b_only_distribution for q0 (what is this? we're very stuck on signal/background namings here) -- see feat: Cleanup internal names for hypotest #1247

Idea: change poi_test to alternative_mu (alt_mu) and asimov_mu to null_mu. This perhaps clarifies the meaning of the testing at least. Then we need to have toys to support these distributions. Then maybe change from signal_plus_b to alternative_distribution and b_only to null_distribution ?

ReadTheDocs build: https://pyhf.readthedocs.io/en/feat-discoveryteststat/_generated/pyhf.infer.test_statistics.q0.html

Closes #520

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

* Add discovery test statistic q0
* Teach the calculators to switch the asimov mu depending on the test statistic (to be refactored in later PR)
* Add tests for discovery test stat
* Add validation ROOT files and tests

Co-authored-by: Nikolai Hartmann <[email protected]>

[codecov]

Codecov Report

Merging #1232 (ea3eb21) into master (eb2449f) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1232      +/-   ##
==========================================
+ Coverage   97.47%   97.48%   +0.01%     
==========================================
  Files          63       63              
  Lines        3716     3733      +17     
  Branches      525      530       +5     
==========================================
+ Hits         3622     3639      +17     
  Misses         55       55              
  Partials       39       39              

Flag	Coverage Δ
unittests	97.48% <100.00%> (+0.01%)	⬆️

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

Impacted Files	Coverage Δ
src/pyhf/infer/__init__.py	100.00% <100.00%> (ø)
src/pyhf/infer/calculators.py	100.00% <100.00%> (ø)
src/pyhf/infer/test_statistics.py	100.00% <100.00%> (ø)
src/pyhf/infer/utils.py	100.00% <100.00%> (ø)

---

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 eb2449f...ea3eb21. Read the comment docs.

[matthewfeickert]

@kratsg overall this looks really good but let me think a bit more about the questions you raise in the PR body. Also for adding new binaries like validation/multibin_histfactory_p0/data/data.root for validation, do we want to add more binaires to the repo? Or should we have these added to scikit-hep-testdata?

[kratsg]

@kratsg overall this looks really good but let me think a bit more about the questions you raise in the PR body. Also for adding new binaries like validation/multibin_histfactory_p0/data/data.root for validation, do we want to add more binaires to the repo? Or should we have these added to scikit-hep-testdata?

I think we should add them to this repo for now. If we then decide to migrate later -- we should. But we should start considering using json2xml functionality to generate on-the-fly binaries in the future... I would be ok with removing it here, and doing an on-the-fly generation for tests.

[matthewfeickert]

@kratsg Thanks a lot for this pretty critical PR. 👍

Technically everything looks good I think. Though I agree that we should probably take this opportunity to consider the terminology and the APIs to try and make things as clear as possible. So tagging @lukasheinrich and @alexander-held here for additional comments and thoughts.

[matthewfeickert]

I think we should add them to this repo for now. If we then decide to migrate later -- we should. But we should start considering using json2xml functionality to generate on-the-fly binaries in the future... I would be ok with removing it here, and doing an on-the-fly generation for tests.

Yeah they're all told only a few kB so I think it is fine. Though at the same time I wouldn't be against doing on-the-fly generation here.

[alexander-held]

Thanks for all the work on this! I tried out the API yesterday, it was straightforward to adopt and in agreement with a ROOT reference for the examples tested.

I do not have any immediate suggestions for naming, but think that the naming inside hypotest may in the long term be difficult to read. @kratsg's idea of alternative/null sounds like it could work (maybe with extra comments calling things by their name in the respective branches in the function?).

[matthewfeickert]

@kratsg Once jax-ml/jax#5374 is resolved and @lukasheinrich reviews then I think this can go in, given that you've made dedicated followup PRs or Issues for most of my revision requests. Can you take care of the

Summarize commit messages into a comprehensive review of the PR

though?

[matthewfeickert]

The complexity of the fixtures in test_validation is getting increasingly more difficult to come back to and make sense of, and it is now over 1000 lines long. In another PR we need to either revise all of it or include some sort of walkthrough of how things work.

[matthewfeickert]

LGTM now

[kratsg]

The complexity of the fixtures in test_validation is getting increasingly more difficult to come back to and make sense of, and it is now over 1000 lines long. In another PR we need to either revise all of it or include some sort of walkthrough of how things work.

I have ideas on how to simplify it. Validation is very tricky to do, but I have a good idea of how to refactor it into something slightly more maintainble.