drop type specs in RecoTracker #30947

jeongeun · 2020-07-28T19:33:21Z

PR description:

Update the safer syntax for existing parameter :
(1) drop type specifications where the original parameter exists.
(2) using "clone" instead of "deepcopy"
(3) move all parameter inside the clone

Instead of modifying parameters with full type specs, which can be interpreted as an insertion of a new parameter, it is a safer way to protect from parameter name mistakes and will also help in possible parameter migrations.
(The references were PR#30353, PR#30556, PR#30671, PR#30700, PR#30827)

In this PR, total 24 files updated.

RecoTracker/{CkfPattern} 3 files
RecoTracker/{Configuration} 3 files
RecoTracker/{SiTrackerMRHTools} 2 files
RecoTracker/{SpecialSeedGenerators} 3 files
RecoTracker/{TkSeedGenerator} 1 file
RecoTracker/{TrackProducer} 11 files
RecoTracker/{TransientTrackingRecHit} 1 file

PR validation:

Event Content comparison check was also done and there is no change with these updates.
Tested in CMSSW_11_2_X, the basic test all passed in the CMSSW PR instructions.

cmsbuild · 2020-07-28T19:33:46Z

The code-checks are being triggered in jenkins.

cmsbuild · 2020-07-28T19:38:14Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-30947/17378

This PR adds an extra 28KB to repository

cmsbuild · 2020-07-28T19:38:42Z

A new Pull Request was created by @jeongeun (JeongEun Lee) for master.

It involves the following packages:

RecoTracker/CkfPattern
RecoTracker/Configuration
RecoTracker/SiTrackerMRHTools
RecoTracker/SpecialSeedGenerators
RecoTracker/TkSeedGenerator
RecoTracker/TrackProducer
RecoTracker/TransientTrackingRecHit

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @felicepantaleo, @GiacomoSguazzoni, @JanFSchulte, @rovere, @VinInn, @mschrode, @gpetruc, @ebrondol, @mtosi, @dgulhan this is something you requested to watch as well.
@silviodonato, @dpiparo, @qliphy you are the release manager for this.

cms-bot commands are listed here

slava77 · 2020-07-28T20:16:48Z

@cmsbuild please test

cmsbuild · 2020-07-28T20:17:12Z

The tests are being triggered in jenkins.

CMSSW_11_2_X_2020-07-28-1100/slc7_amd64_gcc820: https://cmssdt.cern.ch/jenkins/job/ib-run-pr-tests/8378/console Started: 2020/07/28 22:17

cmsbuild · 2020-07-28T21:31:32Z

+1
Tested at: b9054ee
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c373d9/8378/summary.html
CMSSW: CMSSW_11_2_X_2020-07-28-1100
SCRAM_ARCH: slc7_amd64_gcc820

cmsbuild · 2020-07-28T21:31:35Z

Comparison job queued.

cmsbuild · 2020-07-28T22:54:34Z

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-c373d9/8378/summary.html

Comparison Summary:

No significant changes to the logs found
Reco comparison results: 4 differences found in the comparisons
DQMHistoTests: Total files compared: 34
DQMHistoTests: Total histograms compared: 2525444
DQMHistoTests: Total failures: 7
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 2525390
DQMHistoTests: Total skipped: 47
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 33 files compared)
Checked 144 log files, 17 edm output root files, 34 DQM output files

perrotta

"#inport copy" can also get removed from

RecoTracker/Configuration/python/RecoTrackerTopBottom_cff.py

perrotta · 2020-07-29T05:47:10Z

RecoTracker/CkfPattern/python/CkfTrackCandidatesP5_cff.py

@@ -15,9 +15,9 @@
 import copy


This can be removed, as you did in the previous files

Error occur when I remove this copy here :

File "/afs/cern.ch/work/j/jelee/ServiceWork/EventContent/CMSSW_11_2_X_2020-07-20-2300/python/RecoMuon/MuonIdentification/cosmics_id.py", line 12, in <module> cosmicsVetoTrackCandidates = copy.deepcopy(ckfTrackCandidatesP5) NameError: name 'copy' is not defined

cmssw/RecoMuon/MuonIdentification/python/cosmics_id.py

Line 11 in 5d18277

from RecoTracker.CkfPattern.CkfTrackCandidatesP5_cff import *

(I will remove this copy and update RecoMuon/MuonIdentification/cosmics_id.p)

Updating RecoMuon/MuonIdentification/python/cosmics_id.py seems to be rather straightforward: I think you should do it right now, for that specific file, so that you won't forget to clean up this one later on.

perrotta · 2020-07-29T06:11:18Z

RecoTracker/Configuration/python/RecoTrackerP5_cff.py


 from RecoTracker.CkfPattern.CkfTrackCandidatesP5_cff import *
-ckfTrackCandidatesP5.src = cms.InputTag('combinedP5SeedsForCTF')
+ckfTrackCandidatesP5.src = 'combinedP5SeedsForCTF'
 #backward compatibility 2.2/3.1
 #ckfTrackCandidatesP5.SeedProducer = 'combinedP5SeedsForCTF'


This also do not exists any more, and it could be probably removed from this config

perrotta · 2020-07-29T06:13:25Z

RecoTracker/Configuration/python/RecoTrackerP5_cff.py

@@ -44,8 +43,8 @@
 # TRACK INFO
 #include "AnalysisAlgos/TrackInfoProducer/data/TrackInfoProducerP5.cff"


It does not exists: you can remove

The same for RSFinalTrackSelectorP5_cff here above

perrotta · 2020-07-29T06:37:41Z

RecoTracker/Configuration/python/RecoTrackerTopBottom_cff.py

+    TrajectoryBuilderPSet = dict(refToPSet_ = 'GroupedCkfTrajectoryBuilderP5Bottom'),
+    NavigationSchool      = 'CosmicNavigationSchool',
+    src                   = 'combinedP5SeedsForCTFBottom', #ok for 32X
+    #SeedProducer          = 'combinedP5SeedsForCTFBottom', #ok for 22X


perrotta · 2020-07-29T06:38:20Z

RecoTracker/Configuration/python/RecoTrackerTopBottom_cff.py

+    TrajectoryBuilderPSet = dict(refToPSet_ = 'GroupedCkfTrajectoryBuilderP5Top'),
+    NavigationSchool      = 'CosmicNavigationSchool',
+    src                   = 'combinedP5SeedsForCTFTop', #ok for 32X
+    #SeedProducer          = 'combinedP5SeedsForCTFTop', #ok for 22X


perrotta · 2020-07-29T06:50:28Z

RecoTracker/SpecialSeedGenerators/python/CombinatorialSeedGeneratorForCosmicsP5_cff.py

@@ -84,16 +84,17 @@
 #recHitMatcher
 #include "RecoLocalTracker/SiStripRecHitConverter/data/SiStripRecHitMatcher.cfi"


This does not exists any more since a while, already: it can be removed

perrotta · 2020-07-29T06:58:02Z

RecoTracker/TrackProducer/python/TrackRefitterP5_cfi.py

-    src = cms.InputTag("ctfWithMaterialTracksP5"),
-    Fitter = cms.string('FittingSmootherRKP5'),
+    src                 = "ctfWithMaterialTracksP5",
+    Fitter              = 'FittingSmootherRKP5',
    #TTRHBuilder = cms.string('WithAngleAndTemplate'),


you can remove the type specification even if you let it commented out

perrotta · 2020-08-03T17:57:11Z

We are noticing that the combinatorialcosmicseedfinderP5Top/Bottom modules actually end up in some CalibTracker sequence, see #30947 (comment)

Maybe Tracker DPG (@mmusich ...) is more indicated that AlCa to understand whether those modules and downhill sequences are still needed or obsolete, and if they are still used in some "private" CalibTracker script, it would be good to know how to test them for correctness after the updates.

Again, I will sign this PR if there are no objections: but it would be good to clean up things, if it can be done

mmusich · 2020-08-03T18:19:56Z

@perrotta..
I suspect those sequences are used in the Strip cross-talk measurement procedure but I am not 100% sure. @pieterdavid @robervalwalsh can you maybe cross-check with Eric?
Given it's August and the expert might be away can you clarify how urgent is this update? Judging from the content of the PR does not seem super urgent.

perrotta · 2020-08-04T06:45:51Z

@perrotta..
I suspect those sequences are used in the Strip cross-talk measurement procedure but I am not 100% sure. @pieterdavid @robervalwalsh can you maybe cross-check with Eric?
Given it's August and the expert might be away can you clarify how urgent is this update? Judging from the content of the PR does not seem super urgent.

Thank you @mmusich !
Indeed, this PR sits in an ongoing campaign, and none of these PRs are really urgent, as similar ones acting on different packages can live on their own.
The only "issue" we have here is that we are not able to test a few moduels touched by this PR, and therefore certify beyond any doubt that the replacements applied do not actually modify the past behaviour of those modules. On the other hand, after a visual inspection it really looks like that the replacements are done correctly, and therefore we do not expect issues. What I actually really wanted to know was whether we are maintaining any dead code (in which case better to clean it up) or some still used one.
We can let this PR in standby for some time, and wait that the experts get back, if in vacation. If it will take longer than expected, to avoid possible needs of rebase or similar ones I think it will be anyhow safe to merge this PR at some point, and maybe continuing the discussion with those experts afterwards.

perrotta · 2020-08-10T14:45:41Z

@mmusich @pieterdavid @robervalwalsh: did you have time to look at the combinatorialcosmicseedfinderP5Top/Bottom modules and the Strip cross-talk measurement related sequences which include it? Any suggestion about how to test their correctness?
As I said, I'm just waiting for your green light before signing this PR for reco

robervalwalsh · 2020-08-10T15:09:20Z

@perrotta, I started looking into that but did not get a conclusion yet.

mmusich · 2020-08-10T15:13:16Z

@perrotta I personally haven't checked, though can you clarify something about:

The ctftracksP5 sequence/task is only used by CalibTracker/Configuration/python/Reconstruction_cff.py to build the recotrackP5 sequence, which enters the reconstructionP5_step in that calibration file.

as far as I can see that's not used only there, but also elsewhere e.g. in the Pixel / Strip online clients: link.
Secondly I am missing what is the link between ctftracksP5Top/Bottom sequences and ctftracksP5.
You write:

ctftracksP5Top/Bottom sequences, which enter the ctftracksP5 sequence (defined in /RecoTracker/Configuration/python/RecoTrackerP5_cff.py)

but looking in RecoTracker/Configuration/python/RecoTrackerP5_cff.py, I don't see where these sequences enter ctftracksP5

robervalwalsh · 2020-08-10T15:24:10Z

@perrotta @mmusich the sequence was removed a long time ago "until it is ported to the new cluster removal" in the commit
8d229f7

I am trying to trace the history of the new cluster removal to understand why it was never brought back and if it should be back.

perrotta · 2020-08-10T15:30:39Z

@perrotta @mmusich the sequence was removed a long time ago "until it is ported to the new cluster removal" in the commit
8d229f7

I am trying to trace the history of the new cluster removal to understand why it was never brought back and if it should be back.

Thank you @robervalwalsh

perrotta · 2020-08-10T15:33:29Z

@perrotta I personally haven't checked, though can you clarify something about:

The ctftracksP5 sequence/task is only used by CalibTracker/Configuration/python/Reconstruction_cff.py to build the recotrackP5 sequence, which enters the reconstructionP5_step in that calibration file.

as far as I can see that's not used only there, but also elsewhere e.g. in the Pixel / Strip online clients: link.
Secondly I am missing what is the link between ctftracksP5Top/Bottom sequences and ctftracksP5.
You write:

ctftracksP5Top/Bottom sequences, which enter the ctftracksP5 sequence (defined in /RecoTracker/Configuration/python/RecoTrackerP5_cff.py)

but looking in RecoTracker/Configuration/python/RecoTrackerP5_cff.py, I don't see where these sequences enter ctftracksP5

You are right @mmusich , i messed up the lines while trying to reconstruct the path: the sequence they enter is trackerCosmics_TopBot , in the same file (see https://github.com/cms-sw/cmssw/blob/master/RecoTracker/Configuration/python/RecoTrackerP5_cff.py#L77), which looks like a dead end there.

perrotta · 2020-08-17T07:02:44Z

@robervalwalsh @mmusich
This PR is purely technical, and I do not expect it can affect any output anyhow.
The still open issue relates to the possibility to test those modules which are not included in any official standard sequence, and verify at the same time whether they are still needed at all. Since this issue can be easily decoupled from the code changes implemented in this PR, if you have nothing against it I'd plan to sign it for reco before the ORP meeting tomorrow (Aug 18): then all other checks can be still performed independently.
Please let me know if you have any objection.

mmusich · 2020-08-17T14:33:21Z

@perrotta please go ahead, no objections on our side.

perrotta · 2020-08-17T14:37:09Z

@perrotta please go ahead, no objections on our side.

Thank you @mmusich

perrotta · 2020-08-17T14:38:35Z

+1

Technical cleaning of the syntax for parameters passed to cloned/modified modules, as specified in the PR description
Jenkins tests pass

cmsbuild · 2020-08-17T14:38:59Z

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)

qliphy · 2020-08-17T14:59:15Z

+1

jeongeun added 2 commits July 28, 2020 21:08

drop type specs where original parameter exist

df0c7a8

drop type specification where original parameter exist

b9054ee

cmsbuild added this to the CMSSW_11_2_X milestone Jul 28, 2020

cmsbuild added code-checks-pending comparison-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Jul 28, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Jul 28, 2020

cmsbuild added tests-started and removed tests-pending labels Jul 28, 2020

cmsbuild added tests-approved and removed tests-started labels Jul 28, 2020

cmsbuild added comparison-available and removed comparison-pending labels Jul 28, 2020

perrotta reviewed Jul 29, 2020

View reviewed changes

drop not used lines, use safer syntax in config

e4792e5

cmsbuild removed code-checks-approved comparison-available tests-approved labels Jul 30, 2020

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Aug 17, 2020

cmsbuild added orp-approved and removed orp-pending labels Aug 17, 2020

cmsbuild merged commit 56dcfc5 into cms-sw:master Aug 17, 2020

jeongeun mentioned this pull request Aug 18, 2020

drop type specs in RecoJets #31162

Merged

jeongeun mentioned this pull request Aug 25, 2020

drop type specs in RecoEgamma #31243

Merged

This was referenced Sep 2, 2020

drop type specs in RecoEcal #31332

Merged

drop type specs in RecoParticleFlow #31389

Merged

silviodonato mentioned this pull request May 13, 2021

added the possibility to lock a record #33659

Merged

This was referenced Jul 5, 2021

fix definition override in RecoTrackerP5_cff.py #34332

Merged

fix definition override in RecoTrackerP5_cff.py [11.2.X] #34346

Merged

fix definition override in RecoTrackerP5_cff.py [11_3_X] #34347

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drop type specs in RecoTracker #30947

drop type specs in RecoTracker #30947

jeongeun commented Jul 28, 2020

cmsbuild commented Jul 28, 2020

cmsbuild commented Jul 28, 2020

cmsbuild commented Jul 28, 2020

slava77 commented Jul 28, 2020

cmsbuild commented Jul 28, 2020 •

edited

Loading

cmsbuild commented Jul 28, 2020

cmsbuild commented Jul 28, 2020

cmsbuild commented Jul 28, 2020

perrotta left a comment

perrotta Jul 29, 2020

jeongeun Jul 29, 2020 •

edited

Loading

perrotta Jul 29, 2020

perrotta Jul 29, 2020 •

edited

Loading

perrotta Jul 29, 2020

perrotta Jul 29, 2020

perrotta Jul 29, 2020

perrotta Jul 29, 2020

perrotta Jul 29, 2020

perrotta Jul 29, 2020

perrotta commented Aug 3, 2020

mmusich commented Aug 3, 2020

perrotta commented Aug 4, 2020

perrotta commented Aug 10, 2020

robervalwalsh commented Aug 10, 2020

mmusich commented Aug 10, 2020 •

edited

Loading

robervalwalsh commented Aug 10, 2020

perrotta commented Aug 10, 2020

perrotta commented Aug 10, 2020

perrotta commented Aug 17, 2020

mmusich commented Aug 17, 2020

perrotta commented Aug 17, 2020

perrotta commented Aug 17, 2020

cmsbuild commented Aug 17, 2020

qliphy commented Aug 17, 2020

		@@ -44,8 +43,8 @@
		# TRACK INFO
		#include "AnalysisAlgos/TrackInfoProducer/data/TrackInfoProducerP5.cff"

		@@ -84,16 +84,17 @@
		#recHitMatcher
		#include "RecoLocalTracker/SiStripRecHitConverter/data/SiStripRecHitMatcher.cfi"

drop type specs in RecoTracker #30947

drop type specs in RecoTracker #30947

Conversation

jeongeun commented Jul 28, 2020

PR description:

PR validation:

cmsbuild commented Jul 28, 2020

cmsbuild commented Jul 28, 2020

cmsbuild commented Jul 28, 2020

slava77 commented Jul 28, 2020

cmsbuild commented Jul 28, 2020 • edited Loading

cmsbuild commented Jul 28, 2020

cmsbuild commented Jul 28, 2020

cmsbuild commented Jul 28, 2020

perrotta left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeongeun Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perrotta Jul 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

perrotta commented Aug 3, 2020

mmusich commented Aug 3, 2020

perrotta commented Aug 4, 2020

perrotta commented Aug 10, 2020

robervalwalsh commented Aug 10, 2020

mmusich commented Aug 10, 2020 • edited Loading

robervalwalsh commented Aug 10, 2020

perrotta commented Aug 10, 2020

perrotta commented Aug 10, 2020

perrotta commented Aug 17, 2020

mmusich commented Aug 17, 2020

perrotta commented Aug 17, 2020

perrotta commented Aug 17, 2020

cmsbuild commented Aug 17, 2020

qliphy commented Aug 17, 2020

cmsbuild commented Jul 28, 2020 •

edited

Loading

jeongeun Jul 29, 2020 •

edited

Loading

perrotta Jul 29, 2020 •

edited

Loading

mmusich commented Aug 10, 2020 •

edited

Loading