-
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
fixes for recent updates to CkfTrackCandidateMaker
#36658
fixes for recent updates to CkfTrackCandidateMaker
#36658
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-36658/27690
|
A new Pull Request was created by @missirol (Marino Missiroli) for master. It involves the following packages:
@jpata, @cmsbuild, @clacaputo, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
type bugfix |
test parameters:
|
please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-583a82/21582/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
@@ -50,9 +50,6 @@ | |||
updator = 'KFUpdator' | |||
) | |||
|
|||
from Configuration.ProcessModifiers.seedingDeepCore_cff import seedingDeepCore | |||
seedingDeepCore.toModify(TrajectoryBuilderForElectrons, maxPtForLooperReconstruction = cms.double(0.0) ) |
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 removed because the default value for CkfTrajectoryBuilder.maxPtForLooperReconstruction is already 0.0? Is it foreseen that default tracking and deepcore may end up using different thresholds, so it might be better to keep this?
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 it must be removed b/c TrajectoryBuilderForElectrons
is a CkfTrajectoryBuilder
, and that does not use a parameter maxPtForLooperReconstruction
(only GroupedCkfTrajectoryBuilder
does). This also implies that these two lines were not having any effect up to now (the parameter wasn't actually being 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.
To be clearer (maybe I was not): this one update is the one that fixes the unit test (b/c TrajectoryBuilderForElectrons.maxPtForLooperReconstruction
is invalid); it is not related to the "drop type-specs" updates.
@@ -222,8 +222,7 @@ | |||
conv2CkfTrajectoryBuilder = RecoTracker.CkfPattern.GroupedCkfTrajectoryBuilder_cfi.GroupedCkfTrajectoryBuilder.clone( | |||
trajectoryFilter = dict(refToPSet_ = 'conv2CkfTrajectoryFilter'), | |||
minNrOfHitsForRebuild = 3, | |||
clustersToSkip = cms.InputTag('conv2Clusters'), |
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.
Here, too, the parameter is removed b/c GroupedCkfTrajectoryBuilder
does not have/use a parameter named clustersToSkip
.
+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. @perrotta, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
This PR is a follow-up of #36459:
11723.17
;fillDescriptions
toCkfTrackCandidateMaker
(and its dependencies) #36459 (comment).Merely technical. No changes expected.
PR validation:
runTheMatrix.py
(for-l limited
and-l 11723.17
), andaddOnTests.py
passed.If this PR is a backport, please specify the original PR and why you need to backport that PR:
N/A