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

Updates to implentation of MiNNLO muR/muF polynomial variations (also to use them for unfolding and theory agnostic for OOA) #508

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dbruschi
Copy link

@dbruschi dbruschi commented Aug 1, 2024

This is still a work in progress, but it contains updates to the MiNNLO muRmuFPolVar. We noticed that the variations we had before were underestimated, we were computing them incorrectly (we were varying only the denominator instead of only the numerator in the ratio sigma_i/sigma_UL). This PR also contains a first implementation of fully correlated variations in a smooth way, at the moment (but it might change, since this is not giving us what we want) the are derived from the combination of the individual uncorrelated variations, directly in the pt-eta plane and not from the Ais directly, but given how these variations are defined it is mathematically the same. The default is still to use the binned ones, but the can be enabled with the "--muRmuFFullyCorr" option is setupCombine (to be given with "--muRmuFPolVar", which changes only the uncorrelated component).

@bendavid
Copy link
Collaborator

bendavid commented Aug 3, 2024

https://gitlab.cern.ch/cms-wmass/wremnants-data/-/merge_requests/73 will need to be rebased and wremnants-data commit updated

Copy link
Collaborator

@cippy cippy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a few comments, there are also some conflicts to fix

@@ -405,6 +406,9 @@ def setup(args, inputFile, inputBaseName, inputLumiScale, fitvar, genvar=None, x
def assertSample(name, startsWith=["W", "Z"], excludeMatch=[]):
return any(name.startswith(init) for init in startsWith) and all(excl not in name for excl in excludeMatch)

def assertSampleWithOOA(name, startsWith=["W", "Z"], excludeMatch=[]):
return any(name.startswith(init) for init in startsWith) and name.endswith("OOA") and all(excl not in name for excl in excludeMatch)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might probably be possible to use the function assertSample adding more arguments to require the presence of a string or that it appears at the end (its presence is probably sufficient in this case)

@@ -588,7 +597,7 @@ def assertSample(name, startsWith=["W", "Z"], excludeMatch=[]):
for g in cardTool.procGroups["signal_samples"] for m in cardTool.datagroups.groups[g].members},
)

if args.muRmuFPolVar and not isTheoryAgnosticPolVar:
if args.muRmuFPolVar and not isPoiAsNoi:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change correct? Right now we are always using isPoiAsNoi = True (except maybe for one case with unfolding), so this condition would never be entered

variation_hist = hist.Hist(*hist_in.axes, name = out_name, storage = hist.storage.Double())
variation_hist = variation_hist[...,0:1,:]

for i in range(len(hist_in.axes[len(hist_in.axes)-2])):
Copy link
Collaborator

@cippy cippy Aug 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once you select the axis by ID, the number of bins for that axis can be obtained with hist.axes[id].size (or .extent if you want to include possible overflow bins when they exist, .size should always return the number of bins in acceptance regardless).

In general, is it always guaranteed that the axis you want will always be the second from last, rather than the n-th from the left for example ? Perhaps, it might be safer to select the axis by name, which is more guaranteed to stay as it is (and if the name is eventually modified outside then it won't be found here and the code will just raise errors)

@davidwalter2
Copy link
Collaborator

What's the status of this PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants