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.readthedocs.io/en/trigger-ci-beojan-autoscan/api.html#inference

Checklist Before Requesting Reviewer

• [x] Tests are passing
• [x] "WIP" removed from the title of the pull request
• [x] 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

Jan 21 '21 10:01 beojan

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

Jan 21 '21 13:01 alexander-held

Codecov Report

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

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

@@            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              

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

Jan 21 '21 14:01 codecov[bot]

I think this should be ready for formal review now.

Jan 29 '21 14:01 beojan

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.

Jan 30 '21 18:01 matthewfeickert

@beojan believe it or not, we haven't forgotten about this PR :grimacing:. We're just trying to make sure that we have some particular pieces in for a patch release before we start merging in new features for a minor release. But this will get taken care of in due course.

Mar 16 '21 06:03 matthewfeickert

Hi, sorry to bother, I was wondering whether there has been any further advance on this issue. The linear scan is showing to be either slow or inaccurate for our ensemble of signals and we'd be curious to try out this alternative. We could of course go back to cabinetry and use what is there, but personally I believe that having this in pyhf would be beneficial for the package. Cheers, Ben

Nov 25 '21 14:11 gollumben

I was wondering whether there has been any further advance on this issue.

This will be going into v0.7.0, but because we wanted this to go into a minor release and not a patch release we let it linger for a long time. I need to take some time to basically rebuild this on another branch and then merge it back here or get @beojan's approval to close this PR for another more updated one with him as co-author. The fact that this PR is so old is a good example of us needing to find a new way of doing releases (e.g. using release branches and not releasing of master/main).

(Actually I can probably just clean this one up as there is only 1 merge conflict.)

The linear scan is showing to be either slow or inaccurate for our ensemble of signals and we'd be curious to try out this alternative.

@gollumben Do you have examples you can share in a GitHub Discussion?

Nov 30 '21 16:11 matthewfeickert

I could try and rebase this PR later this week.

On Tue, Nov 30, 2021, 08:48 Matthew Feickert @.***> wrote:

I was wondering whether there has been any further advance on this issue.

This will be going into v0.7.0, but because we wanted this to go into a minor release and not a patch release we let it linger for a long time. I need to take some time to basically rebuild this on another branch and then merge it back here or get @beojan https://github.com/beojan's approval to close this PR for another more updated one with him as co-author. The fact that this PR is so old is a good example of us needing to find a new way of doing releases (e.g. using release branches and not releasing of master/main).

The linear scan is showing to be either slow or inaccurate for our ensemble of signals and we'd be curious to try out this alternative.

@gollumben https://github.com/gollumben Do you have examples you can share in a GitHub Discussion?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/scikit-hep/pyhf/pull/1274#issuecomment-982819956, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4OENNKQZPB6EJKZOXVNWLUOT54HANCNFSM4WMTWDVA . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

Nov 30 '21 16:11 beojan

I could try and rebase this PR later this week.

You're welcome to do that (and I usually like rebasing PRs of my own to keep things clear on my local dev branch for myself) but my guess is that it will be way easier to resolve the merge conflict if we just attempt to merge in master to this PR's branch and then fix it there once.

Nov 30 '21 16:11 matthewfeickert

Hi @matthewfeickert, thank you so much! I will share my example during the next week. Cheers, Ben

Nov 30 '21 17:11 gollumben

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

Sep 19 '22 23:09 review-notebook-app[bot]

intervals.upper_limits.upper_limit and intervals.upperlimit is very confusing. Is there a way to clean this API up slightly? Otherwise, the rest looks good to me. I'd typically prefer if we don't have upper_limit.

intervals.upperlimit is being deprecated so it is being left in place until we remove it in v0.9.0 so that we don't break a bunch of people hard. Can you suggest the API signature that you'd like to use?

Sep 20 '22 15:09 matthewfeickert

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

Sep 20 '22 16:09 matthewfeickert

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.

Sep 20 '22 21:09 matthewfeickert

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.

Sep 21 '22 05:09 matthewfeickert

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.)

Sep 21 '22 07:09 matthewfeickert

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.

Sep 21 '22 07:09 kratsg