Trigger sf #26
Trigger sf #26
Conversation
|
@ktht , did you miss this pull request by any chance or I made some major mistakes here? |
|
Please test the code. I sense that there will be segfaults. |
|
I guess there is no option to test it, as the corresponding channel is missing now, I belive |
|
But you can change the config minimally such that the relevant piece of code will be executed. |
|
I tried to check this but it is not possible to do by minimal changes as there is error not coming from trigger but before that. I don't see any such thing in cfg file https://github.com/HEP-KBFI/TallinnNtupleProducer/blob/main/EvtWeightTools/src/HadTauFakeRateInterface.cc#L86 I need one old cfg file to see how it works. |
checked on 4tau samples for 1k events, didn't encounter any error |
There was a problem hiding this comment.
I got to say that a compiling code that does not produce any runtime errors should be the baseline, but not the only sufficient condition for merging this PR. Here are my comments and questions:
- have you checked how to implement the same logic in 0l+2tau? We have a dedicated class for estimating trigger SF in that channel;
- did you compare average trigger SF before and after implementing the changes?
- have you considered the possibility that there may be more than two hadronic taus that could be matched to a trigger object? How often does that happen (if it happens at all)? If so, wouldn't it make sense to implement a logic that averages over the trigger SF probabilities that are computed from all possible pairs of hadronic taus? Or reduce the cone size that is used in trigger object matching?
- please run over more than 1k events when testing, just for spotting pathological cases or possible errors in the code.
There were other issues in the code, which I have now highlighted.
| eff_2tau_tauLeg4_mc = tau_leg_efficiency(hadTau4_pt_, hadTau4_decayMode_, "ditau", wp_str_, "eff_mc", sys); | ||
| //can we use 64, 128 filterbits only as they are related to di-tau trigger | ||
| //https://cmssdt.cern.ch/lxr/source/PhysicsTools/NanoAOD/python/triggerObjects_cff.py?v=CMSSW_12_6_X_2022-09-08-2300 #Line-0122 | ||
| if ( hadTau_recObj->filterBits() ) matched_hadTaus.push_back(hadTau_recObj); |
There was a problem hiding this comment.
You're not actually testing against the bitmask (64) here. Please remove the link to lxr and refer to this instead for self-consistency (plus we're not using NanoAOD Ntuples produced in CMSSW 12_6_x). I recommend that you import the necessary bitmask(s) from triggers_cfi.py, so that they would be implemented in just one place.
| double prob_data(1); | ||
| double prob_mc(1); |
There was a problem hiding this comment.
Why this syntax? Please keep the assignment notation since people are more familiar with it.
| if ( isDEBUG_ ) | ||
| std::cout << "prob_data: " << prob_data << "\t prob_mc: " << prob_mc << std::endl; |
There was a problem hiding this comment.
If you don't want to use curly braces, then I recommend that you put them on a single line, otherwise please use curly braces like in other places of this codebase. Also, std::endl is computationally expensive because it forces to flush the stream. Consider using just the newline character ('\n') instead.
| if ( isDEBUG_ ) | ||
| std::cout << "hadTau pt: " << hadTau->pt() << "\t eta: " << hadTau->eta() << "\t decayMode: " << hadTau->decayMode() << std::endl; |
There was a problem hiding this comment.
Same comment again about the curly braces.
This is related to the issue. I guess we can simply check the filter bits of reco object to decide whether they are matched with trigger objects and if yes, consider only those reco objects to calculate sf. If it is fine, I'll check for 1l_3tau trigger also, currently implemented only for 4tau trigger case