CMS-HGCAL / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
3 stars 2 forks source link

Further unification of classes towards final compact dense indexing #93

Closed pfs closed 7 months ago

pfs commented 7 months ago

Hi @kerstinlovisa !

This commit proposes to fully unify Si and SiPM-on-tile indices under the same dense indexing class.

The differentiated records which were being used for the SiCell and SiPMCells become a single one.

The producer (HGCalMappingCellESProducer) reads both cell map files and adds the new type codes to a unique cell indexer which tracks all the different typecodes.

The SoA for the cell info has a new field called isSiPM to differentiate if needed later in the chain (e.g. when assigning the DetId)

In addition, the PR introduces a change to the module indexer to implement the tracking of readout sequences and offsets. The new module indexer is still under debugging so it hasn't yet been re-enabled in the sequence although it compiles fine.

kerstinlovisa commented 7 months ago

Hi @pfs thanks!

I made some small changes to the SiPM mapping just before your PR, for example I removed the thickness variable from the Cell SoA because we won't need it for the cells (it could depend on the final mapping version later). Sorry, so I think there's some small changes that will need to be fixed but I can merge and correct it myself.

I also think it would be possible for us to account for the max number of cells for each time if we wanted to for the offset. Or do think it's unnecessary?

pfs commented 7 months ago

Hi @kerstinlovisa . For the changes in the siPM mapping that's fine we just need to adapt the file parsing. One thing that I was going to propose is if we can order the columns the same way in both the Si and SiPM cell files. E.g. for the indexer we don't need to parse them all so the first columns would be typecode, roc, half.

I'm not sure i understand the second question. Is it that you would prefer a "strict counting" of channels as they come listed in the file? The current scheme is the eRx are counted and then they are converted to 39 cells / eRx. That corresponds to 36 channels + 1 calib channel + 2 common mode. Here there are some overheads:

kerstinlovisa commented 7 months ago

Thanks @pfs.

Yes, indeed I can updated the ordering of the columns. Can I merge this PR now?

My question was more about the SiPM module types which have lower number of rings than standard 8 number of rings, say for example K5, which will be assigned 2*39 number of channels but actually only have 39 + 8 channels on the tileboard. I don't know if this overhead would make a large difference?

pfs commented 7 months ago

Hi @kerstinlovisa. It can be merged. It's compiling and running although some parts are still commented.

The test/testMappingModuleIndexer_cfg.py is pointing to a simplified module locator, to understand if all the indices are being properly assigned.

For the question: 39 + 8 is 60% larger than needed but it will be peanuts as there are only a "few" of these. I would not optimize it further for now. In system validation it may be that 39+39 are readout for commissioning and that way the offline is also prepared.

kerstinlovisa commented 7 months ago

@pfs Ok thanks!