-
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
improve implementation of HLTJetTag<T>
plugin template
#41351
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41351/35190
|
A new Pull Request was created by @missirol (Marino Missiroli) for master. It involves the following packages:
@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
Please do not start the tests yet, unless you want to see them fail.
|
please test
The two items above are done (it only took me one month..). Let's see if this PR works now. |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8a0bda/32678/summary.html Comparison SummaryThere are some workflows for which there are errors in the baseline: Summary:
|
18a1de5
to
c7dfe93
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41351/35583
|
Pull request #41351 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again. |
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8a0bda/32689/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) |
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.
When we combine the header and source file for plugins we normally keep the source and remove the header. I couldn't find an official requirement for it in http://cms-sw.github.io/cms_coding_rules.html, though.
Therefore, I am not going to enforce it here. However, it looks to me quite reasonable that plugins shouldn't stay on header files, because they shouldn't get included by other files.
Therefore @missirol I wonder whether would you agree to move the current HLTJetTag.h
file into a HLTJetTag.cc
one, or if you see any reason not to do so, instead: in the second case, this PR can be merged as such.
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 I agree with everything. What is non-standard here is that this is a plugin template, so in a sense I followed rule 4.3
Header files must not contain any implementation except for class templates and code to be inlined.
, and that's why it stayed as a header file. The header file is called by HLTrigger/btau/plugins/modules.cc
, which is where the actual plugins (template instantiations) are created.
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.
Ok, it makes sense, thank you Marino.
Let things as such, then
+1 |
PR description:
This PR aims to improve the plugin template
HLTJetTag<T>
as follows.JetTagCollection
whose values as the jet b-tagging scores).edm::AssociationVector::operator[]
. This requires that the input jet collection is the same as the "key" collection of theJetTagCollection
. If this is not the case, the plugin will throw an exception at runtime.The limits of the current implementation of
HLTJetTag<T>
were discussed in #41045.PR validation:
Private tests done as part of #41045.
If this PR is a backport, please specify the original PR and why you need to backport that PR. If this PR will be backported, please specify to which release cycle the backport is meant for:
CMSSW_13_0_X
CMSSW_13_1_X