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

Implementation of theory-motivated agnostic bands #345

Merged
merged 7 commits into from
Nov 22, 2023
Merged

Conversation

emanca
Copy link
Collaborator

@emanca emanca commented Nov 21, 2023

The bands can be produced using the script makePdfUncPlot.py and then are used by default by setupCombine to build the shapes. It's possible to increase the size of the band using the new option theoryAgnosticBandSize which defaults to 1. The size of the OOA is hardcoded to 1 and the variations on sigmaUL are always 50%, even if the band is increased. I also corrected a problem in the implementation that prevented the fit to run correctly when --hdf5 option is used. Everything has been tested locally and it works and the output makes sense. We wish to merge this ASAP (at most tomorrow morning) to run the bias tests with @jeyserma together with the traditional analysis, therefore @cippy and @davidwalter2 please tell me what you think.

@emanca emanca added enhancement New feature or request high priority high priority PR labels Nov 21, 2023
scripts/combine/setupCombine.py Outdated Show resolved Hide resolved
scripts/combine/setupCombine.py Outdated Show resolved Hide resolved
@cippy
Copy link
Collaborator

cippy commented Nov 21, 2023

From a first look it would seem fine, and it shouldn't modify other things run in CI, but in any case you can surely start running all tests you need even before the PR is merged, since the main code with this PR will simply be your local code.

@emanca
Copy link
Collaborator Author

emanca commented Nov 21, 2023

From a first look it would seem fine, and it shouldn't modify other things run in CI, but in any case you can surely start running all tests you need even before the PR is merged, since the main code with this PR will simply be your local code.

Of course, but @jeyserma is currently running the bias tests on both setups

@bendavid
Copy link
Collaborator

Is this ready to merge?

@bendavid
Copy link
Collaborator

From the ci only the theory agnostic changes as expected (with the norm impacts getting significantly smaller, but probably it's hard to draw any conclusions from the limited stat run in the ci)

@bendavid
Copy link
Collaborator

ci results still look consistent

@bendavid bendavid merged commit d1c272e into WMass:main Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request high priority high priority PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants