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
Remove manually generated cfi file for ElectronSeedProducer #28213
Remove manually generated cfi file for ElectronSeedProducer #28213
Conversation
The code-checks are being triggered in jenkins. |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-28213/12329
|
A new Pull Request was created by @guitargeek (Jonas Rembser) for master. It involves the following packages: DPGAnalysis/Skims @perrotta, @pgunnell, @chayanit, @zhenhu, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
@cmsbuild please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
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.
nice work grouping the parameters in the ElectronSeedProducer.
psd0.add<bool>("dynamicPhiRoad", true); | ||
|
||
// specify where to get the hits from | ||
psd0.add<edm::InputTag>("measurementTrackerEvent", {"MeasurementTrackerEvent"}); |
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.
is this parameter still used?
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.
Yes, this one is still used. Not in the current configuration thought, but at least the plugin could theoretically be configured such that it is actually used in a meaningful way.
// H/E rechits | ||
psd0.add<edm::InputTag>("hcalRecHits", {"hbhereco"}); // OBSOLETE | ||
psd0.add<double>("hOverEHBMinE", 0.7); // OBSOLETE | ||
psd0.add<double>("hOverEHFMinE", 0.8); // OBSOLETE |
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.
better remove these, if they are obsolete
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, that should definitely go away. I will push an update after local tests.
@cmsbuild please test |
The tests are being triggered in jenkins. |
Comparison job queued. |
Comparison is ready Comparison Summary:
|
+1 |
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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
The
ecalDrivenElectronSeeds_cfi.py
file redundantly redefines all the configuration values that are already set in thefillDescriptions()
method of theElectronSeedProducer
. It would be better to avoid this to make theElectronSeedProducer
more maintainable. The entries infillDescriptions
have been reshuffled and commented to match the previous easy-to-read structure in the cfi file.On this occasion, the
fromTrackerSeeds
parameter was finally removed completely, as it could not be disabled anymore anyway (the code would have segfaulted has reported in #27541) and the standard tracker seeds should always be used now. That finishes what #28039 and #28133 started.Another update is that the HoE values are not stored anymore in some unused vectors. I think this must be a remnant from removing the HoE values in the ElectronSeed class [1] two years ago.
Unrelated to this, there is a separate commit that moves the forward declaration and implementation of
GsfElectronAlgoHeavyObjectCache
into theGsfElectronAlgo
class. It was always a bit strange to me why many other helper classes for GsfElectronAlgo (that are also much longer) don't have their own files while the HeavyObjectCache (which really doesn't make sense to use in another context) does not.[1] https://github.com/cms-sw/cmssw/blob/master/DataFormats/EgammaReco/interface/ElectronSeed.h#L16
PR validation:
CMSSW compiles and local matrix tests pass.
if this PR is a backport please specify the original PR:
No backport intended.