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
Merge .h and .cc files of PhysicsTools/PatAlgos plugins #34307
Conversation
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34307/23639
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34307/23640
|
A new Pull Request was created by @guitargeek (Jonas Rembser) for master. It involves the following packages: PhysicsTools/PatAlgos @perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c98ed1/16399/summary.html CMS StaticAnalyzer warnings: There are 10 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c98ed1/16399/llvm-analysis/esrget-sa.txt for details. Comparison SummarySummary:
|
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'm puzzled by PhysicsTools/PatAlgos/plugins/PATPrimaryVertexCleaner.cc, which is said to be an "empty file" in the github diff, but it actually does exist with its content (as it should) even after the PR...
@@ -1,11 +1,61 @@ | |||
#include "ModifiedObjectProducer.h" |
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.
The file can be renamed with the class name (i.e. without the final "s", as it was the case for the .h before)
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.
Okay!
// -*- C++ -*- | ||
// | ||
// Package: PatShapeAna | ||
// Class: PatShapeAna | ||
// | ||
/**\class PatShapeAna PatShapeAna.cc PhysicsTools/PatShapeAna/src/PatShapeAna.cc | ||
/**\class PatShapeAna |
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.
Since you already modified the name, why not to fix it then?
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.
Sure!
#include "PhysicsTools/PatAlgos/plugins/PATTriggerMatchSelector.h" | ||
#include "CommonTools/UtilAlgos/interface/PhysObjectMatcher.h" | ||
/** | ||
\class pat::PATTriggerMatchSelector PATTriggerMatchSelector.h "PhysicsTools/PatAlgos/plugins/PATTriggerMatchSelector.h" |
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.
Since the file name has changed, maybe also those comments should
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.
Okay
@@ -70,6 +70,14 @@ | |||
patJetCorrFactors | |||
) | |||
|
|||
from Configuration.ProcessModifiers.run2_miniAOD_pp_on_AA_103X_cff import run2_miniAOD_pp_on_AA_103X |
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.
This is from #33928: a rebase could get rid of these spurious additions in the diff
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.
Thanks for spotting, I made some mistakes when moving around directories.
@@ -152,7 +152,7 @@ def toolCode( self, process ): | |||
trigEvtProdMod = getattr( process, triggerEventProducer ) | |||
trigEvtProdMod.processName = hltProcess | |||
trigEvtProdMod.patTriggerProducer = cms.InputTag( triggerProducer ) | |||
if not path is '': |
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.
This is from #34253: a rebase could get rid of these spurious additions in the diff
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.
Thanks for spotting, I made some mistakes when moving around directories.
Hi Andrea, thanks for the review! I'll update the PR in a second. About the PATPrimaryVertexCleaner.cc: I only fixed the permissions from 755 to 644, as it was the only file in the 'plugins' directory that wrongly was marked executable. You can even see it in the GitHub diff, and the |
-1 Failed Tests: RelVals CMS StaticAnalyzer warnings: There are 10 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c98ed1/16440/llvm-analysis/esrget-sa.txt for details. RelValsThe relvals timed out after 4 hours. |
please test |
-1 Failed Tests: UnitTests RelVals RelVals-INPUT AddOn Unit TestsI found errors in the following unit tests: ---> test runtestPhysicsToolsPatAlgos had ERRORS RelVals
Expand to see more relval errors ...RelVals-INPUT
Expand to see more relval errors ...AddOn Tests
Expand to see more addon errors ... |
@guitargeek , also given the errors in the tests: what about reverting the last commit and descope the ESProducer migration to a follow-up PR? I was basically ready to sign the "merge" PR before that commit, and not requiring other signatures it could have been merged rather quickly... |
@perrotta, okay I didn't expect you to want to merge this with the static analyzer report with the |
-1 Failed Tests: RelVals-INPUT CMS StaticAnalyzer warnings: There are 10 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c98ed1/16442/llvm-analysis/esrget-sa.txt for details. RelVals-INPUTThe relvals timed out after 4 hours.
Expand to see more relval errors ...
Comparison SummarySummary:
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c98ed1/16449/summary.html CMS StaticAnalyzer warnings: There are 10 EventSetupRecord::get warnings. See https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c98ed1/16449/llvm-analysis/esrget-sa.txt for details. Comparison SummarySummary:
|
+reconstruction
|
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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
Part of the series of PRs that merges
.h
files with the.cc
files of the plugins in the PhysicsTools subsystem.This has already been done for RecoEgamma, RecoEcal, and RecoParticleFlow subsystems (see e.g. #34069, etc.).
It makes maintaining the plugins much easier and also excludes the possibility to wrongly include plugin header files.
There were the steps taken to merge the files:
PR validation:
CMSSW compiles.
if this PR is a backport please specify the original PR and why you need to backport that PR:
No backport intended.