-
Notifications
You must be signed in to change notification settings - Fork 125
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
Fixing FTAG MCMC SFs #1686
Fixing FTAG MCMC SFs #1686
Conversation
Dear @tofitsch , Could you take a quick look at this? I've tested that it behaves as I expect, but it's good to have a reviewer separate from the author. Thanks, |
Looks generally good to me. I trust you with all the numbers and will not check them :D only worry I have is regarding backwards compatibility. E.g. before any file sample name containing |
Good idea, I'll check for h7s. There are some such as The amc definitely shouldn't fall under this case as it has it's own MC-MC SFs, and I don't think MG should either as the MC-MC SFs (https://ftag.docs.cern.ch/algorithms/activities/mcmc/#run-2) specify they are for Powheg+Herwig7. I believe the one with PowhegHerwig in the name should fall under Herwig7.1. I'm less clear on the dijet samples. It could be worth adding a warning in general that it's good to double-check what values the parsing uses, as it's very easy to miss edge cases, but I have mixed feelings on this. @tofitsch Does this plan (the PR as is) sound reasonable? I think this is the safest solution- if there's patterns that we should be counting but aren't, they'll give errors and users can either report them (in which case we'll adjust to add them), or manually set which to use. I believe the alternative would count patterns that shouldn't be counted, which I would rather not do as it makes it possible we will process some incorrectly. |
else if(tmp_name.Contains("SH_2210")) return Sherpa2210; | ||
else if(tmp_name.Contains("SH_2211")) return Sherpa2210; | ||
else if(tmp_name.Contains("SH_2212")) return Sherpa2210; | ||
else if(tmp_name.Contains("SH_2214")) return Sherpa2210; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I realise I'm a little late now, but would it be possible to declare SH_2214 also Sherpa2212 so that the BJetEffCorr doesn't crash on the Run3 V+jets samples?
I can start a new pull request for this line if needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @dbuchin ,
You should be able to manually set the calibration by setting m_EfficiencyCalibration to the desired value in your configuration ("700660" to use the Sherpa 2.2.12 SFs). Would this work for you? If possible, I would prefer not to automatically declare any unless they are officially supported (see https://ftag.docs.cern.ch/algorithms/activities/mcmc/#release-22-supported-generators for the official list).
Updating MC-MC SFs to match https://ftag.docs.cern.ch/algorithms/activities/mcmc/#release-22-supported-generators and the CDI files (with the CDI files given precedence). Note this requires a new option in BJetEfficiencyCorrector, isRun3, which is set to False by default.