HGCal Eras using recommended methods #14168
Conversation
|
A new Pull Request was created by @kpedro88 (Kevin Pedro) for CMSSW_8_1_X. It involves the following packages: CalibCalorimetry/HcalPlugins @civanch, @cvuosalo, @mdhildreth, @cmsbuild, @rekovic, @franzoni, @cerminar, @slava77, @mmusich, @mulhearn, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are list here #13028 |
|
attn: @lgray |
| obj.outputCommands.append('keep *_HGCalUncalibRecHit_*_*') | ||
| _phase2_RecoLocalCaloFEVT_outputCommands = RecoLocalCaloFEVT.outputCommands | ||
| _phase2_RecoLocalCaloFEVT_outputCommands.append('keep *_HGCalRecHit_*_*') | ||
| _phase2_RecoLocalCaloFEVT_outputCommands.append('keep *_HGCalUncalibRecHit_*_*') |
There was a problem hiding this comment.
Does this create a copy of RecoLocalCaloFEVT.outputCommands (I'd say no but I could be wrong)? If not, I would do the following
eras.phase2_hgcal.toReplaceWith(RecoLocalCaloFEVT, RecoLocalCaloFEVT.clone(
outputCommands = RecoLocalCaloFEVT.outputCommands + [
'keep *_HGCalRecHit_*_*',
'keep *_HGCalUncalibRecHit_*_*'
]
))
(of course it can be done via a temporary object and using append if you prefer, I prefer to avoid them if reasonable)
There was a problem hiding this comment.
@makortel - yes, it is intended to create a copy and then append to the copy. I guess I can understand the desire to avoid temporary objects, but I think using toModify here is a little nicer than toReplaceWith... @Dr15Jones, any preference?
There was a problem hiding this comment.
@kpedro88 My main concern is that does the assignment _phase2_RecoLocalCaloFEVT_outputCommands = RecoLocalCaloFEVT.outputCommands create a copy or not (I fear not). A simplified example
import FWCore.ParameterSet.Config as cms
foo = cms.vstring("foo")
bar = foo # corresponds "_phase2_RecoLocalCaloFEVT_outputCommands = RecoLocalCaloFEVT.outputCommands"
bar.append("bar")
print foo
# prints "foo" and "bar"
Temporary object I think is more a matter of style, I mostly wanted to give an example how to avoid it.
There was a problem hiding this comment.
@makortel - Ah, that is probably a shortcoming of my Python knowledge. I'll figure out a way to fix it.
There was a problem hiding this comment.
Assignment in python is not a copy. The way I think about it is all python variables are essentially pointers. so when you do a = b, a and b both point to the same python object.
|
I'm done with all my comments |
|
@Dr15Jones thanks, I'll fix all of those. I didn't know about the dict syntax for toModify, that is a lot nicer. |
|
@Dr15Jones all comments addressed. I also defined a separate HGCal local reconstruction sequence and put it in its own cff for better organization. |
|
Looks good to me. Thanks for all your effort! |
cmsbuild
added
simulation-pending
tests-pending
comparison-pending
and removed
comparison-available
simulation-approved
tests-approved
labels
|
@vandreev11 noticed the HCAL upgrade RecHit collections were not kept in the output, so I decided it was easiest just to add them here. |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
It should really be ready for signatures now... @mmusich, @mulhearn, @slava77, @civanch, @davidlange6 |
|
+1 |
|
+1 |
|
@mmusich - there are a lot of answers to that question... For 2016, hopefully as soon as possible. We're testing things privately now. For 2017, we won't have real conditions until we know what devices/boards/etc. we're installing... so we'll rely on the hardcode for a while, at least. At some point, we may make a set of "fake" average conditions for the DB. For 2019/2023 studies, we'll continue to rely on the hardcode conditions primarily, as far as I know. They offer a lot of flexibility to cope with geometry changes, etc. automatically. (And with the configurability I've recently added, they can be used to test various electronics scenarios as well.) |
|
+1 for #14168 ef38181
|
This PR addresses comments from @Dr15Jones on #13992 and attempts to follow best practices as described in SWGuideCmsDriverEras.
I think this will need to be rebased once #14165 is merged and it's not essential for pre3 anyway (since #13992 works for immediate needs), so no need to test at the moment. Primarily posting now to see if @Dr15Jones has any more comments to address.