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 autoscan for upper limit using TOMS Algorithm 748 #1274

Merged
merged 94 commits into from
Sep 21, 2022

Conversation

beojan
Copy link
Contributor

@beojan beojan commented Jan 21, 2021

Description

Resolves #1265

This PR uses SciPy's TOMS748 implementation to autoscan for the upper limit. @alexander-held's suggestions about memoizing the objective function and determining the best brackets for each band have been implemented.

ReadTheDocs build: https://pyhf--1274.org.readthedocs.build/en/1274/api.html#confidence-intervals

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 pyhf.infer.intervals and pyhf.infer.intervals.upper_limits modules.
* Migrate pyhf.infer.intervals.upperlimit to pyhf.infer.intervals.upper_limits.upper_limit.
* Add pyhf.infer.intervals.upper_limits.toms748_scan to API which implements the SciPy TOMS748
  root finding method to scan for the upper limit.
* Add pyhf.infer.intervals.upper_limits.linear_grid_scan to preserve the linear scan of the
  old pyhf.infer.intervals.upperlimit API in a specific API.
* Use pyhf.infer.intervals.upper_limits.toms748_scan as the default method of scanning for
  and upper limit in pyhf.infer.intervals.upper_limits.upper_limit.
* Update lower bound of the supported scipy versions to v1.2.0 to use scipy.optimize.toms748.
* Add tests for the pyhf.infer.intervals.upper_limits.toms748_scan API and the behavior 
  changes in pyhf.infer.intervals.upper_limits.upper_limit.
* Add 'Confidence Intervals' section to the API docs and move the pyhf.infer.intervals.upper_limits
  APIs there.
* Add a deprecation warning to pyhf.infer.intervals.upperlimit and note it is scheduled
  for removal in pyhf v0.9.0.
* Add Beojan Stanislaus to contributors list.

Co-authored-by: Matthew Feickert <matthew.feickert@cern.ch>

src/pyhf/infer/intervals.py Outdated Show resolved Hide resolved
tests/test_infer.py Outdated Show resolved Hide resolved
@alexander-held
Copy link
Member

A few more ideas that might be out of scope for this implementation:

  • optional return of the mu values and obs/exp CLs would be convenient for visualizations, e.g. with pyhf.contrib.viz.brazil
  • it might be useful to allow calculating the limit for obs/exp only, or obs + exp (without bands), which is much faster and sufficient for analysis optimizations
  • some kind of status update (e.g. via logging) could be useful for complex workspaces, where the whole scan could take a very long time and the user may wonder whether the code is stuck
  • it would be useful to protect against exceptions (e.g. when signs of left/right do not differ), since otherwise all results may be lost

Co-authored-by: Giordon Stark <kratsg@gmail.com>
@matthewfeickert matthewfeickert marked this pull request as draft January 21, 2021 14:34
@matthewfeickert matthewfeickert added the feat/enhancement New feature or request label Jan 21, 2021
@matthewfeickert matthewfeickert changed the title WIP: Autoscan for upper limit using TOMS748 feat: Add autoscan for upper limit using TOMS Algorithm 748 Jan 21, 2021
@matthewfeickert matthewfeickert marked this pull request as ready for review January 21, 2021 14:45
@codecov
Copy link

codecov bot commented Jan 21, 2021

Codecov Report

Base: 98.28% // Head: 98.29% // Increases project coverage by +0.01% 🎉

Coverage data is based on head (36a9911) compared to base (749b9f7).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1274      +/-   ##
==========================================
+ Coverage   98.28%   98.29%   +0.01%     
==========================================
  Files          68       69       +1     
  Lines        4482     4529      +47     
  Branches      730      738       +8     
==========================================
+ Hits         4405     4452      +47     
  Misses         45       45              
  Partials       32       32              
Flag Coverage Δ
contrib 27.51% <20.89%> (-0.14%) ⬇️
doctest 61.13% <68.65%> (+<0.01%) ⬆️
unittests-3.10 96.31% <100.00%> (+0.08%) ⬆️
unittests-3.7 96.28% <100.00%> (+0.08%) ⬆️
unittests-3.8 96.33% <100.00%> (+0.08%) ⬆️
unittests-3.9 96.35% <100.00%> (+0.08%) ⬆️

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

Impacted Files Coverage Δ
src/pyhf/infer/intervals/__init__.py 100.00% <100.00%> (ø)
src/pyhf/infer/intervals/upper_limits.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@matthewfeickert matthewfeickert added docs Documentation related tests pytest labels Jan 21, 2021
@beojan
Copy link
Contributor Author

beojan commented Jan 29, 2021

I think this should be ready for formal review now.

@matthewfeickert
Copy link
Member

I think this should be ready for formal review now.

Thanks very much @beojan. We have a few other PRs that we're trying to prioritize for the v0.6.0 release, but we'll try to get to this one early next week as well.

@matthewfeickert
Copy link
Member

matthewfeickert commented Sep 20, 2022

For reasons unclear to me we're currently being blocked by Issue #2015, which is only happening with this PR/these changes. :?

@matthewfeickert
Copy link
Member

I'm unsure as to what is causing Issue #2015, as I'm not able to reproduce it at all locally, but I can reproduce it in CI, but only on branches that contain the commits in this PR. :? @kratsg @lukasheinrich any thoughts here are welcome.

@matthewfeickert
Copy link
Member

I'm not sure on this, but I think that because simply calling pyhf.set_backend("jax") here fixes the problem seen in Issue #2015 in CI makes me think that there is a bug somewhere in how the backends are getting reset in the tensor manager.

PR #2017 will fix this, so after it is merged in and the master branch is merged back into this PR the tests will pass. I've verified this on a test branch already.

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.

Perhaps dropping the explicit set_backend calls?

src/pyhf/infer/intervals/upper_limits.py Show resolved Hide resolved
src/pyhf/infer/intervals/upper_limits.py Show resolved Hide resolved
src/pyhf/infer/intervals/upper_limits.py Show resolved Hide resolved
v0.7.0 automation moved this from Reviewer approved to Review in progress Sep 21, 2022
@matthewfeickert
Copy link
Member

matthewfeickert commented Sep 21, 2022

Perhaps dropping the explicit set_backend calls?

@kratsg We can, though is the idea here that this avoids people focusing too much on NumPy? The main reason I've been putting those into examples is that I wanted the output tensor format to be made explicitly clear in advance. Though would you recommend not doing that now that we have the default backend machinery? (I'm going to go offline now, so I'll defer to you here.)

@kratsg
Copy link
Contributor

kratsg commented Sep 21, 2022

Though would you recommend not doing that now that we have the default backend machinery? (I'm going to go offline now, so I'll defer to you here.)

It's cleaner since we might change default backend later. Or if we drop numpy later (doubt it) but I'm fine with it as it is. I don't think we set backend explicitly in most other tests.

v0.7.0 automation moved this from Review in progress to Reviewer approved Sep 21, 2022
@kratsg kratsg merged commit 0ae9b0e into scikit-hep:master Sep 21, 2022
v0.7.0 automation moved this from Reviewer approved to Done Sep 21, 2022
matthewfeickert added a commit that referenced this pull request Sep 23, 2022
* Update scipy lower bound to v1.2.0 to match lower bound in setup.cfg.
   - Amends PR #1274
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Changes the public API dependencies Pull requests that update a dependency file docs Documentation related feat/enhancement New feature or request tests pytest
Projects
Development

Successfully merging this pull request may close these issues.

Auto scan for hypothesis test inversion (Implemented)
6 participants