Migrate module configurations in RecoLocalMuon to use automatically generated cfi default configurations #33543
Conversation
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33543/22322
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33543/22323
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33543/22324
|
|
A new Pull Request was created by @jeongeun (JeongEun Lee) for master. It involves the following packages: RecoLocalMuon/CSCRecHitD @perrotta, @jpata, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
| csc2DRecHits = cms.EDProducer("CSCRecHitDProducer", | ||
| import RecoLocalMuon.CSCRecHitD.cscRecHitDProducer_cfi as _mod | ||
|
|
||
| csc2DRecHits = _mod.cscRecHitDProducer.clone( | ||
| # | ||
| # Parameters for coordinate and uncertainty calculations |
There was a problem hiding this comment.
These comment lines must remain attached to the cscRecHitDParameters , now moved at the end of this file
| # to be deleted | ||
| CSCStripClusterSize = cms.untracked.int32(3) | ||
| CSCStripClusterSize = 3, |
There was a problem hiding this comment.
Triggered by the "to be deleted" comment above, I verified that the CSCStripClusterSize parameter is actually not needed any more in the CSCRecHitDProducer code, and this is probably a good opportunity to clean it away from the configuration. Do you agree @ptcox ?
If you do so, please also:
- remove the definition from the
fillDescriptionsmethod of CSCRecHitDProducer.cc - Add a temporary customization function for the HLT in HLTrigger/Configuration/python/customizeHLTforCMSSW.py (see for example how it is done in https://github.com/cms-sw/cmssw/pull/33526/files#diff-8fc23ee531566af47b6eff1b354bb3db1c4e8f3925413edc3632e417d168b75fR132)
|
Hi Andrea,
Concerning
# to be deleted
- CSCStripClusterSize = cms.untracked.int32(3)
+ CSCStripClusterSize = 3,
This looks like the result of people iteratively 'cleaning' up the code
until the original meaning has been lost. I see the actual definition
is now as a static const in
RecoLocalMuon/CSCRecHitD/src/CSCHitFromStripOnly.h
Indeed we use only 3 strips for a cluster. But that is not to say using
'3' is a good idea! Once we used 5, which is more realistic, and I
always wanted someone to the effects.
It should in principle remain configurable. But as you notice it seems
that option has long been removed anyway. So I guess there's nothing to
be lost further if the meaningless config is removed. Maybe the day will
never come when it all has to be put back in. Easier to start from
scratch with a whole new package :)
Tim
perrotta wrote on 4/27/21 18:30:
…
***@***.**** commented on this pull request.
------------------------------------------------------------------------
In RecoLocalMuon/CSCRecHitD/python/cscRecHitD_cfi.py
<#33543 (comment)>:
> @@ -4,73 +4,73 @@
# parameters for CSC rechit building
from RecoLocalMuon.CSCRecHitD.cscRecHitD_cff import *
-csc2DRecHits = cms.EDProducer("CSCRecHitDProducer",
+import RecoLocalMuon.CSCRecHitD.cscRecHitDProducer_cfi as _mod
+
+csc2DRecHits = _mod.cscRecHitDProducer.clone(
#
# Parameters for coordinate and uncertainty calculations
These comment lines must remain attached to the |cscRecHitDParameters|
, now moved at the end of this file
------------------------------------------------------------------------
In RecoLocalMuon/CSCRecHitD/python/cscRecHitD_cfi.py
<#33543 (comment)>:
> # to be deleted
- CSCStripClusterSize = cms.untracked.int32(3)
+ CSCStripClusterSize = 3,
Triggered by the "to be deleted" comment above, I verified that the
|CSCStripClusterSize| parameter is actually not needed any more in the
CSCRecHitDProducer code, and this is probably a good opportunity to
clean it away from the configuration. Do you agree @ptcox
<https://github.com/ptcox> ?
If you do so, please also:
* remove the definition from the |fillDescriptions| method of
CSCRecHitDProducer.cc
* Add a temporary customization function for the HLT in
HLTrigger/Configuration/python/customizeHLTforCMSSW.py (see for
example how it is done in
https://github.com/cms-sw/cmssw/pull/33526/files#diff-8fc23ee531566af47b6eff1b354bb3db1c4e8f3925413edc3632e417d168b75fR132)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#33543 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGYLHV5LRZJS7VNZKZ3OBLTK3RAZANCNFSM43VFKOJA>.
|
|
+1
|
|
+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. @silviodonato, @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
|
+1 |
PR description:
Optimization of the python configurations: Improve maintainability by cleaning up the duplicated and cloning from the default/reference configurations.
In this PR, 1 file changed. (The previous PRs were PR#33207, PR#33307, PR#33352 )
commit 1 :
Replace explicit configuration with a reference from cfipython/. (migrating EDProducer("type", .. -> typeDefaylt.clone() )
Remove the type specifications already presented in cfipython/fillDescriptions reference for improved syntax safety.
commit 2 :
Remove the duplicated parameters that are exactly the same value in cfipython reference.
PR validation:
Event Content comparison check was also done and there is no change with these updates.
Tested in CMSSW_11_3_X, the basic test all passed in the CMSSW PR instructions.