add fillDescriptions to CkfTrackCandidateMaker (and its dependencies)
#36459
Conversation
|
A new Pull Request was created by @missirol (Marino Missiroli) for master. It involves the following packages:
@Martin-Grunewald, @clacaputo, @cmsbuild, @missirol, @slava77, @jpata can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-aef66a/21198/summary.html Comparison SummarySummary:
|
missirol
force-pushed
the
devel_ckfTrackCandidateMakerFillDesc_123X
branch
from
d420c86 to
21d4e79
Compare
cmsbuild
added
code-checks-pending
tests-pending
and removed
tests-approved
code-checks-approved
labels
|
Pull request #36459 was updated. @Martin-Grunewald, @clacaputo, @cmsbuild, @missirol, @slava77, @jpata can you please check and sign again. |
|
please test |
| iDesc.add<double>("subclusterCutSN", 12.); | ||
|
|
||
| edm::ParameterSetDescription psdLM; | ||
| psdLM.setAllowAnything(); |
There was a problem hiding this comment.
psdLM.setAllowAnything();
Just to note that, here, there is still a shortcut. The treatment of this PSet is not entirely trivial, so further improvements are left to experts.
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-aef66a/21546/summary.html Comparison SummarySummary:
|
|
+reconstruction
|
|
+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, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
cmsbuild
added
fully-signed
hlt-approved
and removed
hlt-pending
pending-signatures
labels
| @@ -161,8 +160,7 @@ | |||
| numHitsForSeedCleaner = cms.int32(50), | |||
There was a problem hiding this comment.
Why not removing the explicit type also here and below (and above for maxDPhiForLooperReconstruction, maxPtForLooperReconstruction)?
There was a problem hiding this comment.
@perrotta I agree, this should be fixed. I'm wondering what the best timeline is: we could test this PR as is in the IB over the weekend, to see if any issues show up in other wfs, before the next pre-release, or I could apply the changes now and that will likely delay things a bit (b/c of re-testing and re-getting signatures). If you think this could be merged already now, I would go for the first option (and will make a separate PR on Monday to fix the explicit-type changes I missed); if you prefer otherwise, I can apply the changes already to this PR.
| @@ -210,8 +210,7 @@ | |||
| # TRACK BUILDING | |||
| import RecoTracker.CkfPattern.GroupedCkfTrajectoryBuilder_cfi | |||
| lowPtTripletStepTrajectoryBuilder = RecoTracker.CkfPattern.GroupedCkfTrajectoryBuilder_cfi.GroupedCkfTrajectoryBuilder.clone( | |||
| MeasurementTrackerName = '', | |||
| trajectoryFilter = cms.PSet(refToPSet_ = cms.string('lowPtTripletStepTrajectoryFilter')), | |||
| trajectoryFilter = dict(refToPSet_ = 'lowPtTripletStepTrajectoryFilter'), | |||
| maxCand = 4, | |||
| estimator = 'lowPtTripletStepChi2Est', | |||
| maxDPhiForLooperReconstruction = cms.double(2.0), | |||
| @@ -275,8 +275,7 @@ | |||
| # TRACK BUILDING | |||
| import RecoTracker.CkfPattern.GroupedCkfTrajectoryBuilder_cfi | |||
| pixelPairStepTrajectoryBuilder = RecoTracker.CkfPattern.GroupedCkfTrajectoryBuilder_cfi.GroupedCkfTrajectoryBuilder.clone( | |||
| MeasurementTrackerName = '', | |||
| trajectoryFilter = cms.PSet(refToPSet_ = cms.string('pixelPairStepTrajectoryFilter')), | |||
| trajectoryFilter = dict(refToPSet_ = 'pixelPairStepTrajectoryFilter'), | |||
| maxCand = 3, | |||
| estimator = 'pixelPairStepChi2Est', | |||
| maxDPhiForLooperReconstruction = cms.double(2.0), | |||
|
@missirol I cannot find the place where the parameters for HI were wrongly set as untracked, as you mentioned in #36459 (comment) |
| phase2skipClusters_(false) { | ||
| clustersToSkipTag_(conf.getParameter<edm::InputTag>("clustersToSkip")), | ||
| skipClusters_(!clustersToSkipTag_.label().empty()), | ||
| phase2ClustersToSkipTag_(conf.getParameter<edm::InputTag>("phase2clustersToSkip")), |
There was a problem hiding this comment.
Since clustersToSkip and phase2ClustersToSkip are supposed to be used alternatively, I have the impression that these settings could be simplified... But I agree this is not the PR where to do it, you had already quite a lot of adjustments to apply here only to implement fillDescriptions (thank you for it, by the way!)
Hi @perrotta, in the |
|
Ah, ok, thank you @missirol |
|
+1
|
PR description:
This PR is an attempt to integrate the changes developed in #35385 (credits: @mmusich), in order to address #35344.
This PR starts from the last bit of development mentioned in #35385 (comment), rebased on a recent IB; more information on how these changes came together can be found in #35385. Since the latter slowed down, I'm opening another PR to at least test those latest changes.
Adding a
fillDescriptionsmethod toCkfTrackCandidateMakerrequired more work than expected, because of the many dependencies of this plugin. Going down the rabbit hole of addingfillDescriptionsfor all the relevant classes led to touching a large number of files, both in thec++andpythoninterfaces. The introduction offillDescriptionsmethods in these classes exposed a number of unused parameters in PSets of various modules. In order to clean these up, customisations for the HLT configs were also necessary.Merely technical.
Must not change any of the outputs.No changes expected.PR validation:
addOnTests, unit tests, and a limited set of matrix workflows passed. No detailed physics validation, only verified that the HLT decisions for a few wfs (max. 10 events each) are unchanged.If this PR is a backport, please specify the original PR and why you need to backport that PR:
N/A