search

cms-sw / cmssw

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

Clean up HGCal subdetector terminology and usage #26225

Open kpedro88 opened 5 years ago

kpedro88 commented 5 years ago

From @cseez https://github.com/cms-sw/cmssw/pull/26099#issuecomment-474816786:

The correct terminology is that FH refers to fine samplings, BH refers to coarse samplings - FH/BH should not be used to refer to the distinction between Si and mixed cassettes. I don't know how deeply embedded the improper usage is in CMSSW, but we should try to eradicate it... even if it takes a few steps to do so...

Right now, RecHitTools has lastLayerEE() and lastLayerFH() functions. It should be checked that the usage of these functions is correct. In addition, functions like lastLayerFine() and lastLayerCoarse() can be added to present the indicated information in a clear way.

attn: @bsunanda @rovere

kpedro88 commented 5 years ago

assign upgrade

cmsbuild commented 5 years ago

New categories assigned: upgrade

@kpedro88 you have been requested to review this Pull request/Issue and eventually sign? Thanks

cmsbuild commented 5 years ago

A new Issue was created by @kpedro88 Kevin Pedro.

@davidlange6, @Dr15Jones, @smuzaffar, @fabiocos, @kpedro88 can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

rovere commented 5 years ago

@kpedro88 RecHitTools is a nice utility package, but it needs to be properly configured from the EventSetup. This, IMHO, in a few situations, makes its usage far less trivial, especially in plugins and helper plugins where the price you have to pay is to percolate the EventSetup deep down your code (either that or the rechHitTools instance). But I agree that we should carefully review the usage of hard-coded constants in the code. Constants declared as such in a commonly included header, with proper naming, do not follow into that category, IMHO. It lacks, of course, the flexibility and "dynamic nature" of geometry derived values from the EventSetup.

kpedro88 commented 5 years ago

Constants declared as such in a commonly included header, with proper naming, do not follow into that category, IMHO.

The V10 geometry has a different number of layers than V9. This clearly can't be handled well with a hardcoded constant. The approach really needs to be reassessed. I highly doubt V10 will be the last major change before we actually build the HGCal.

rovere commented 5 years ago

HGCal... I agree, we should review the code and find an optimal way to handle flexibility and usability.