-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Reduce non necessary sqrt computations in RecoTauTag/HLTProducers #41100
Conversation
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41100/34725 ERROR: Build errors found during clang-tidy run.
|
8bf7faf
to
396f34b
Compare
code-checks |
please test |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41100/34726
|
A new Pull Request was created by @perrotta (Andrea Perrotta) for master. It involves the following packages:
@Martin-Grunewald, @missirol can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@@ -50,7 +51,7 @@ HLTPFDiJetCorrCheckerWithDiTau::HLTPFDiJetCorrCheckerWithDiTau(const edm::Parame | |||
: tauSrc_(consumes(iConfig.getParameter<edm::InputTag>("tauSrc"))), | |||
pfJetSrc_(consumes(iConfig.getParameter<edm::InputTag>("pfJetSrc"))), | |||
extraTauPtCut_(iConfig.getParameter<double>("extraTauPtCut")), | |||
mjjMin_(iConfig.getParameter<double>("mjjMin")), | |||
m2jjMin_(iConfig.getParameter<double>("mjjMin") * iConfig.getParameter<double>("mjjMin")), |
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.
I think this needs to be improved.
A user might put mjjMin = -1
in the PSet (with the intention of switching off the cut), and the plugin will 'silently' require m2 > 1
.
One way to improve this is
m2jjMin_(std::max(0, iConfig.getParameter<double>("mjjMin")) * std::max(0, iConfig.getParameter<double>("mjjMin"))),
The same argument probably applies to other plugins touched by this PR.
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.
You are correct @missirol
The last commit should take care of it
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d65937/31411/summary.html Comparison SummarySummary:
|
hold |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41100/34751
|
Pull request has been put on hold by @perrotta |
Pull request #41100 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again. |
unhold |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-d65937/31475/summary.html Comparison SummarySummary:
|
+hlt |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
Triggered by the review in #41004 (review)
A few expensive calculations (involving square roots) were replaced by some lighter ones in RecoTauTag/HLTProducers
PR validation:
It builds
No changes expected whatsoever in output