cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.12k stars 4.38k forks source link

Duplicated configuration of `EGammaSuperclusterProducer` in the Phase-2 HLT menu #47145

Closed mmusich closed 3 hours ago

mmusich commented 2 weeks ago

As reported here some duplicates (instances of the same ED module that are configured exactly in the same way down to the last parameter but have different labels) were found in the Phase 2 HLT menu. Most of them have been addressed at PR https://github.com/cms-sw/cmssw/pull/47144, but few remain to be clarified. All of the remaining ones are related to e/gamma reconstruction:

# EGammaSuperclusterProducer (hltTiclEGammaSuperClusterProducerL1Seeded)
hltTiclEGammaSuperClusterProducerL1Seeded
hltTiclEGammaSuperClusterProducerUnseeded
ticlEGammaSuperClusterProducer

it's not fully clear if these are indeed meant to be duplicates of not, as the naming is contradicting, e.g. “L1Seeded” is a duplicate of “Unseeded”. Moreover there is another issue concerning the EGammaSuperclusterProducer. One of the instances ticlEGammaSuperClusterProducer used in HLTHgcalTiclPFClusteringForEgammaL1SeededSequence_cfi , is importing things from offline:

from RecoHGCal.TICL.ticlEGammaSuperClusterProducer_cfi import ticlEGammaSuperClusterProducer

This is what we strictly avoided while building the EGamma menu for HLT Phase2 TDR as this is not a desired feature in HLT config. At the time of HLT TDR, the consensus was NOT importing from offline and having a self-sufficient, independent HLT configuration. This choice may have changed over time for whatever reason, but can be rediscussed to come to a definitive conclusion and propagate to Phase2 HLT developers. Also note that there is no concept of L1-seeding for offline. So if HLT starts to import offline config files, then eventually the boundary between unseeded and L1-seeded will be blurred, and HLT-timing will not be benefiting anymore by L1 seeded reconstruction that EGamma had implemented for HLT TDR.

FYI: @rovere @VourMa @swagata87 @RSalvatico

cmsbuild commented 2 weeks ago

cms-bot internal usage

cmsbuild commented 2 weeks ago

A new Issue was created by @mmusich.

@Dr15Jones, @antoniovilela, @makortel, @mandrenguyen, @rappoccio, @sextonkennedy, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

mmusich commented 2 weeks ago

assign hlt

mmusich commented 2 weeks ago

type egamma

cmsbuild commented 2 weeks ago

New categories assigned: hlt

@Martin-Grunewald,@mmusich you have been requested to review this Pull request/Issue and eventually sign? Thanks

makortel commented 2 weeks ago

assign upgrade

cmsbuild commented 2 weeks ago

New categories assigned: upgrade

@Moanwar,@srimanob,@subirsarkar you have been requested to review this Pull request/Issue and eventually sign? Thanks

mmusich commented 2 weeks ago

https://github.com/mmusich/cmssw/commit/daba9d0167a03a3e158d984b721bfd80480a32b3 fixes the issue and passes runTheMatrix.py --what upgrade -l 29691.203,29691.204.

mmusich commented 2 weeks ago

tagging also @saumyaphor4252 as discussed in the HLT phase2 upgrade meeting.

VourMa commented 2 weeks ago

@rovere and I think it would be better for the EGM POG to take care of this issue. We understand that the duplication arises from the fact that the L1Seeded and Unseeded modules are created based on a procModifier, and this is not what we would expect. As a result, this may not be just a problem of duplication but one of logic in how the procModifier works in this case, and the code may need some refactoring. Our understanding is that the complications came about because of #46010, so it may be useful for @RSalvatico to take a look, if possible?

mmusich commented 1 day ago

After discussion with @rovere and @waredjeb, I proposed a possible solution at https://github.com/cms-sw/cmssw/pull/47286

mmusich commented 3 hours ago

+hlt