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

Fix the way systematics in different groups get applied in BinnedEDManager #59

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

willp240
Copy link

There's a few changes here, building on previous PRs because I think Po-Wei and I will want to work off a branch on a shorter timescale than it's reasonable to expect a review! I'll lay out the changes then explain the big change in more detail

  • Formatting BinnedEDManager (it seems to have escaped my previous attempts somehow)
  • Include the fix in Correctly use Systematic Groups in BinnedNLLH::AddPDFs #58. Bad git practice but as long as we're aware when merging it should be ok
  • Include the changes in Don't Re-Shrink PDFs  #55. Less ok than the above. We need a true fix here, but this works in our antinu use case. This PR shouldn't be merged until we have a solution for that though
  • Fix a bug I introduced in Allow user to specify empty string for systematics group #51. We need to return out of SystematicManager::Add after using the alternative overloaded method, otherwise systematics get added to fGroups[""] twice and it rightfully complains
  • Change the logic on how systematic groups get applied in the SystematicManager. I'll explain this in detail below
  • Add a whole bunch of unit tests for different combinations of PDFs and systematics in groups and the global groups. I compare returned LLH evaluations to calculated values (in simple 2 bin setups), and compare to values I calculated by hand with pen and paper

So the big problem this fixes is for applying both at least one systematic in the global group, and at least one systematic in at least one other group. Firstly, previously, systematics were always applied to the OrignalEDs_, so when applying more than group to a PDF we'd do:

WorkingEDs_[j].SetBinContents(GetTotalResponse(groupName).operator()(OrignalEDs_.at(j).GetBinContents()));

then loop to the next groupName, and again do:

WorkingEDs_[j].SetBinContents(GetTotalResponse(groupName).operator()(OrignalEDs_.at(j).GetBinContents()));

So the last group is the only one that would be applied!

Also there was some funky logic with the global "" group. Previously if there were any systematics in the global group, these would get applied at the same time as applying any other group. So the global systematics could get applied more than once - if it weren't for the above bug meaning only last group's application would be used!

I've simplified (and hopefully corrected) the logic so we first apply the global group if there is anything in it, and then loop through groups applying them to the WorkingEDs_ so that each successive group is still used.

Some of the new unit tests fail when using OXO as it was before this PR. However, part of that is for other reasons than what they're trying to catch. If you take the current master branch, implement the return in SystematicManager::Add from this PR. Then take the BinnedNLLHTest from this PR, but remove the Consistent Probability with Buffer which is from #55 so will fail prior to that. Then if you run that BinnedNLLHTest on the current master some of the new tests will fail because the returned LLH is incorrect, whereas they now pass after this PR. That highlights what the problem was and how it is fixed I think

Sorry this is all in one PR. Having laid it out, I think I'll make separate PRs for each bit. But this gives us something to work on with it all in one place, particularly because we want what's in #55 but that won't be sorted immediately.

@dcookman
Copy link

I've now merged in PRs #58, #60, and #61, which /should/ reduce the delta between this PR and the head of the main branch. If you could merge those changes into this PR, that'll help us see the remaining differences we'll have to work with.
I think maybe a good starting point is for you to make a new PR with the additional unit tests, and then we can use those to confirm the changes we'll need to make in the coming PRs?

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

Successfully merging this pull request may close these issues.

2 participants