-
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
Migrate rest of RecoLocalTracker to esConsumes #33123
Migrate rest of RecoLocalTracker to esConsumes #33123
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33123/21474
|
A new Pull Request was created by @mmusich (Marco Musich) for master. It involves the following packages: RecoLocalTracker/Phase2TrackerRecHits @perrotta, @kpedro88, @cmsbuild, @srimanob, @slava77, @jpata 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-817052/13397/summary.html Comparison SummarySummary:
|
@@ -111,7 +111,9 @@ void SeedClusterRemover::readPSet( | |||
} | |||
|
|||
SeedClusterRemover::SeedClusterRemover(const ParameterSet &iConfig) | |||
: doStrip_(iConfig.existsAs<bool>("doStrip") ? iConfig.getParameter<bool>("doStrip") : true), | |||
: tTrackerGeom_( | |||
esConsumes<TrackerGeometry, TrackerDigiGeometryRecord>(edm::ESInputTag("", ""))), //is it correct to use "" ? |
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 original comment was made 6 years ago.
65ded94#diff-34ddaf02250754d45c345cbe666b5e9ef429c81f61cc386f5a2761a1fc7b9829R385
is it reasonable to expect that the comment is not necessary anymore? (there is no default argument value to escape passing something)
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 suppose it can. If I remove the comment, I think I can remove entirely the ESInputTag
as I think that ("","")
it's the default.
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 then, even simpler/shorter
@@ -61,7 +61,9 @@ using namespace std; | |||
using namespace edm; | |||
|
|||
SeedClusterRemoverPhase2::SeedClusterRemoverPhase2(const ParameterSet &iConfig) | |||
: doOuterTracker_(iConfig.existsAs<bool>("doOuterTracker") ? iConfig.getParameter<bool>("doOuterTracker") : true), | |||
: tTrackerGeom_( | |||
esConsumes<TrackerGeometry, TrackerDigiGeometryRecord>(edm::ESInputTag("", ""))), //is it correct to use "" ? |
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 comment looks like a copy of that from the non-Phase2 variant.
is it reasonable to expect that the comment is not necessary anymore?
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.
see above.
733ea9b
to
0a8736c
Compare
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33123/21492
|
@cmsbuild please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-817052/13414/summary.html Comparison SummarySummary:
|
+Upgrade Migration PR, no change expected in Physics result. |
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:
Title says it all, follows the migrations done for Strips at PR #31826 and for Pixels at PR #32093.
PR validation:
It compiles.
if this PR is a backport please specify the original PR and why you need to backport that PR:
Not a backport, no backport is needed.