Drop type specs in RecoHI {Configuration, HiTracking} #32386

jeongeun · 2020-12-04T09:54:46Z

PR description:

Update the safer syntax for existing parameter :

drop type specifications where the original parameter exists.
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 previous PR for RecoHI is PR#32031)

In this PR, total 29 files changed.

RecoHI/Configuration : 3 files
RecoHI/HiTracking : 26 files

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.

…RecoHI112X

cmsbuild · 2020-12-04T10:06:36Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32386/20252

This PR adds an extra 44KB to repository

cmsbuild · 2020-12-04T10:07:04Z

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

It involves the following packages:

RecoHI/Configuration
RecoHI/HiTracking

@perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks.
@MiheeJo, @jazzitup, @yenjie, @kurtejung, @mandrenguyen, @dgulhan, @yetkinyilmaz 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

perrotta · 2020-12-05T08:39:11Z

RecoHI/Configuration/python/Reconstruction_hiSignal_cff.py

@@ -11,8 +11,9 @@

 #Tracks
 hiSignalGlobalPrimTracks = hiGlobalPrimTracks.clone()
-hiSignalSelectedTracks = hiSelectedTracks.clone()
-hiSelectedTracks.src = cms.InputTag("hiSignalGlobalPrimTracks")
+hiSignalSelectedTracks = hiSelectedTracks.clone(


Here the hiSignalSelectedTracks are updated with the input collection "hiSignalGlobalPrimTracks", but then later the heavyIonTrackingTask is feeed with the (unmodified) hiSelectedTracks: I wonder how it can even work
I suppose you already tested it, e.g. with the HI workflows in the matrix tests (140.43, 140.46, 158.01): but it is possible that you wouldn't get an error in that case, because it does not seem to me that this Reconstruction_hiSignal_cff.py is ever included in any actual reconstruction sequence.

Let then involve the HI reco contact, @mandrenguyen :

I already noticed in my comment for the previous version of this PR that already the original formulation of this PR was bugged; I don't think this update is actually fixing it

I don't see this Reconstruction_hiSignal_cff.py ever used in the configs in CMSSW

Do you have any suggestion (based on a possible usage of this python snippet) for fixing it? If this file isn't really used anywhere, wouldn't it be more convenient to profit of this PR in order to completely remove it, instead of spending more efforts in trying to fix it?

perrotta · 2020-12-10T09:20:26Z

@mandrenguyen did you have time to check whether RecoHI/Configuration/python/Reconstruction_hiSignal_cff.py is needed to remain in the release?

If it is not needed, I would suggest @jeongeun to profit of this PR and remove it; if we decide not to remove it, instead, please Matt suggest some fix for it (see also previous discussions in #32386 (comment) and https://github.com/cms-sw/cmssw/pull/32171/files#r526776262)

mandrenguyen · 2020-12-10T09:23:23Z

@perrotta Sorry for the slow response. This cff is deprecated and can be removed from the release.

perrotta · 2020-12-10T09:35:57Z

@perrotta Sorry for the slow response. This cff is deprecated and can be removed from the release.

Thank you @mandrenguyen
@jeongeun please remove RecoHI/Configuration/python/Reconstruction_hiSignal_cff.py: then this PR will be ready to get approved (after the tests)

cmsbuild · 2020-12-14T03:31:59Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32386/20370

This PR adds an extra 48KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoHI/Configuration/python/Reconstruction_hiPF_cff.py modified in PR(s): Improving the boundary between PFAlgo and PFTICL (or simPF) for phase2 #32291

cmsbuild · 2020-12-14T03:32:21Z

Pull request #32386 was updated. @perrotta, @jpata, @cmsbuild, @slava77 can you please check and sign again.

perrotta · 2020-12-14T07:09:39Z

please test

perrotta · 2020-12-14T18:01:39Z

Reco comparison results: 85 differences found in the comparisons

I think I (wrongly) suggested to remove too many PSet's, and modify the original ones with dict(...) instead: sorry for that.
I'll try to list in the following what I would suggest to revert, but please check if there are more

perrotta

The differences wrt the baseline should originate from the updated configuration in
HILowPtConformalPixelTracks_cfi.py
I'd suggest reverting a few more PSet's, for safety.

perrotta · 2020-12-14T18:02:55Z

RecoHI/HiTracking/python/HIBestVertexSequences_cff.py

-        maxEta = cms.double(100.),               
-        trackQuality = cms.string("any"),
-        numTracksThreshold = cms.int32(2)
+    TkFilterParameters = dict(


This is perhaps correct, but for safety (not all parameters are re-defined here) I'd revert it to a PSet, as before

perrotta · 2020-12-14T18:03:47Z

RecoHI/HiTracking/python/HILowPtConformalPixelTracks_cfi.py

-         HitProducer = cms.string('siPixelRecHits'),
-         TTRHBuilder = cms.string('WithTrackAngle'),
+hiConformalPixelTracksPhase1SeedLayers = lowPtQuadStepSeedLayers.clone(
+    BPix = dict( 


This should get reverted to PSet

perrotta · 2020-12-14T18:03:53Z

RecoHI/HiTracking/python/HILowPtConformalPixelTracks_cfi.py

+	HitProducer = 'siPixelRecHits',
+        TTRHBuilder = 'WithTrackAngle',
+    ),
+    FPix = dict( 


This should get reverted to PSet

perrotta · 2020-12-14T18:05:41Z

RecoHI/HiTracking/python/hiPixelPairStep_cff.py

+                 'BPix1+FPix1_pos', 'BPix1+FPix1_neg',
+                 'BPix2+FPix1_pos', 'BPix2+FPix1_neg',
+                 'FPix1_pos+FPix2_pos', 'FPix1_neg+FPix2_neg'],
+    BPix = dict(


This should get reverted to PSet

perrotta · 2020-12-14T18:05:48Z

RecoHI/HiTracking/python/hiPixelPairStep_cff.py

-        TTRHBuilder = cms.string('TTRHBuilderWithoutAngle4PixelPairs'),
-        HitProducer = cms.string('siPixelRecHits'),
+    ),
+    FPix = dict(


This should get reverted to PSet

cmsbuild · 2020-12-16T05:13:34Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-32386/20434

This PR adds an extra 52KB to repository
There are other open Pull requests which might conflict with changes you have proposed:
- File RecoHI/Configuration/python/Reconstruction_hiPF_cff.py modified in PR(s): Improving the boundary between PFAlgo and PFTICL (or simPF) for phase2 #32291

cmsbuild · 2020-12-16T05:14:00Z

Pull request #32386 was updated. @perrotta, @jpata, @cmsbuild, @slava77 can you please check and sign again.

perrotta · 2020-12-16T06:35:21Z

please test
(let see as it works now)

cmsbuild · 2020-12-16T09:29:51Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-8a06e1/11739/summary.html
CMSSW: CMSSW_11_3_X_2020-12-15-2300/slc7_amd64_gcc900

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 36
DQMHistoTests: Total histograms compared: 2747284
DQMHistoTests: Total failures: 12
DQMHistoTests: Total nulls: 1
DQMHistoTests: Total successes: 2747249
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: -0.004 KiB( 35 files compared)
DQMHistoSizes: changed ( 312.0 ): -0.004 KiB MessageLogger/Warnings
Checked 153 log files, 37 edm output root files, 36 DQM output files

perrotta · 2020-12-16T09:43:15Z

+1

Dropped type specifications from cloned/modifed modules in pythons configs, as stated in the PR description
Also a deprecated (and likely bugged) configuration has been removed: RecoHI/Configuration/python/Reconstruction_hiSignal_cff.py
Jenkins tests pass and show no differences in reco outputs wrt the baseline

cmsbuild · 2020-12-16T09:45:20Z

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)

silviodonato · 2020-12-16T16:32:13Z

+1

jeongeun added 3 commits December 4, 2020 05:13

drop type specs in RecoHI{Configuration, HiTracking}

2de1ca7

drop type specs (update)

b274596

Merge branch 'master' of https://github.com/cms-sw/cmssw into syntax…

12fe74c

…RecoHI112X

cmsbuild added this to the CMSSW_11_3_X milestone Dec 4, 2020

cmsbuild added code-checks-pending orp-pending pending-signatures reconstruction-pending tests-pending labels Dec 4, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 4, 2020

cmsbuild mentioned this pull request Dec 4, 2020

Improving the boundary between PFAlgo and PFTICL (or simPF) for phase2 #32291

Merged

perrotta reviewed Dec 5, 2020

View reviewed changes

jeongeun mentioned this pull request Dec 7, 2020

drop type spec in TrackingTools RecoMuon #32396

Merged

remove Reconstruction_hiSignal_cff.py (deprecated)

8329e7c

cmsbuild added code-checks-pending and removed code-checks-approved labels Dec 14, 2020

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 14, 2020

jeongeun mentioned this pull request Dec 14, 2020

drop type spec in CommonTools #32464

Merged

cmsbuild added tests-started and removed tests-pending labels Dec 14, 2020

perrotta reviewed Dec 14, 2020

View reviewed changes

Update hiPixelPairStep_cff.py

11aeb6d

cmsbuild added code-checks-pending tests-pending and removed code-checks-approved tests-approved labels Dec 16, 2020

jeongeun added 2 commits December 16, 2020 14:04

Update HILowPtConformalPixelTracks_cfi.py

72e90d3

Update HIBestVertexSequences_cff.py

ebfd5f0

cmsbuild added code-checks-approved and removed code-checks-pending labels Dec 16, 2020

cmsbuild added tests-started and removed tests-pending labels Dec 16, 2020

cmsbuild added tests-approved and removed tests-started labels Dec 16, 2020

cmsbuild added fully-signed reconstruction-approved and removed pending-signatures reconstruction-pending labels Dec 16, 2020

cmsbuild added orp-approved and removed orp-pending labels Dec 16, 2020

cmsbuild merged commit 1aabdf6 into cms-sw:master Dec 16, 2020

jeongeun mentioned this pull request Mar 3, 2021

drop type specs and move parameters inside the modifier in RecoHI #33048

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop type specs in RecoHI {Configuration, HiTracking} #32386

Drop type specs in RecoHI {Configuration, HiTracking} #32386

jeongeun commented Dec 4, 2020

cmsbuild commented Dec 4, 2020

cmsbuild commented Dec 4, 2020

perrotta Dec 5, 2020

perrotta commented Dec 10, 2020

mandrenguyen commented Dec 10, 2020

perrotta commented Dec 10, 2020

cmsbuild commented Dec 14, 2020

cmsbuild commented Dec 14, 2020

perrotta commented Dec 14, 2020

perrotta commented Dec 14, 2020

perrotta left a comment

perrotta Dec 14, 2020

perrotta Dec 14, 2020

perrotta Dec 14, 2020

perrotta Dec 14, 2020

perrotta Dec 14, 2020

cmsbuild commented Dec 16, 2020

cmsbuild commented Dec 16, 2020

perrotta commented Dec 16, 2020

cmsbuild commented Dec 16, 2020

perrotta commented Dec 16, 2020

cmsbuild commented Dec 16, 2020

silviodonato commented Dec 16, 2020

Drop type specs in RecoHI {Configuration, HiTracking} #32386

Drop type specs in RecoHI {Configuration, HiTracking} #32386

Conversation

jeongeun commented Dec 4, 2020

PR description:

PR validation:

cmsbuild commented Dec 4, 2020

cmsbuild commented Dec 4, 2020

perrotta Dec 5, 2020

Choose a reason for hiding this comment

perrotta commented Dec 10, 2020

mandrenguyen commented Dec 10, 2020

perrotta commented Dec 10, 2020

cmsbuild commented Dec 14, 2020

cmsbuild commented Dec 14, 2020

perrotta commented Dec 14, 2020

perrotta commented Dec 14, 2020

perrotta left a comment

Choose a reason for hiding this comment

perrotta Dec 14, 2020

Choose a reason for hiding this comment

perrotta Dec 14, 2020

Choose a reason for hiding this comment

perrotta Dec 14, 2020

Choose a reason for hiding this comment

perrotta Dec 14, 2020

Choose a reason for hiding this comment

perrotta Dec 14, 2020

Choose a reason for hiding this comment

cmsbuild commented Dec 16, 2020

cmsbuild commented Dec 16, 2020

perrotta commented Dec 16, 2020

cmsbuild commented Dec 16, 2020

Comparison Summary

perrotta commented Dec 16, 2020

cmsbuild commented Dec 16, 2020

silviodonato commented Dec 16, 2020