Geometry module, link and board mapping updates - 2

[snwebb]

PR description:

Replacement/continuation of #459, after implementation of module rotation class in cms-sw#32684. Note that the changes from PR32684 have been copied here, as they do not currently exist in the PFCal-dev repository.
This adds new functionality to obtain the mapping between modules, lpgbts, and stage 1 and 2 FPGAs.
These are in the form of a `.json' file, which is then read-in and manipulated (json file only an example skeleton at present)

PR validation:

Tested that the code compiles and each function works in isolation.

[snwebb]

• Thanks for the comments - I've addressed the ones above (other than one which I had a query about).
• I still need to update the json file
• I get a crash when I run after the initialisation of this geometry file (see below) - would this be expected?

Begin processing the 1st record. Run 1, Event 409728, LumiSection 46 on stream 0 at 19-Feb-2021 15:23:48.927 CET
----- Begin Fatal Exception 19-Feb-2021 15:24:00 CET-----------------------
An exception of category 'Invalid DetId' occurred while
   [0] Processing  Event run: 1 lumi: 46 event: 409728 stream: 0
   [1] Running path 'ntuple_step'
   [2] Calling method for module HGCalTriggerNtupleManager/'hgcalTriggerNtuplizer'
Exception Message:
Cannot initialize HGCScintillatorDetId from b4a40091

[snwebb]

Hi @jbsauvan - I have now added a realistic input json file (other than the Stage 1 to Stage 2 mapping which is preliminary).

[jbsauvan]

Thanks @snwebb
Concerning the HGCScintillatorDetId exception, it is due to the usage of HGCScintillatorDetId for scintillator modules in the ntuplizers.

I quickly updated the geometry tester class for V9Imp3 and found some inconsistencies in the TC <-> module navigation (checking and comparing TCs from module and module from TC):

 Trigger cell 3003562773( EE:HSil= 1:0 type= 2 z= 1 layer= 1 wafer(u,v:x,y)= (-11,-5:17,-10) triggerCell(u,v:x,y)= (5,1:-14,9))
 has not been found in                                                                                                                            
 module HGCalModuleDetId::HFNose:EE:HSil:HScin=0:1:0:0 type= 2 z= -1 layer=1 sector=1 module(u,v)= (6,11)
 Available trigger cells are:                                                                         
      EE:HSil= 0:1 type= 1 z= -1 layer= 1 wafer(u,v:x,y)= (-6,0:12,0) triggerCell(u,v:x,y)= (4,5:9,-1)  
      EE:HSil= 0:1 type= 1 z= -1 layer= 1 wafer(u,v:x,y)= (-6,0:12,0) triggerCell(u,v:x,y)= (4,4:3,0)   
      EE:HSil= 0:1 type= 1 z= -1 layer= 1 wafer(u,v:x,y)= (-6,0:12,0) triggerCell(u,v:x,y)= (5,6:15,0)  
      EE:HSil= 0:1 type= 1 z= -1 layer= 1 wafer(u,v:x,y)= (-6,0:12,0) triggerCell(u,v:x,y)= (5,5:9,2)  
      EE:HSil= 0:1 type= 1 z= -1 layer= 1 wafer(u,v:x,y)= (-6,0:12,0) triggerCell(u,v:x,y)= (5,4:3,4)                  
      EE:HSil= 0:1 type= 1 z= -1 layer= 1 wafer(u,v:x,y)= (-6,0:12,0) triggerCell(u,v:x,y)= (6,7:21,2)    
      EE:HSil= 0:1 type= 1 z= -1 layer= 1 wafer(u,v:x,y)= (-6,0:12,0) triggerCell(u,v:x,y)= (6,6:15,4)  
      EE:HSil= 0:1 type= 1 z= -1 layer= 1 wafer(u,v:x,y)= (-6,0:12,0) triggerCell(u,v:x,y)= (6,5:9,6)                                                                                  
      EE:HSil= 0:1 type= 1 z= -1 layer= 1 wafer(u,v:x,y)= (-6,0:12,0) triggerCell(u,v:x,y)= (6,4:3,8)
.....

You can find the tester class in commit jbsauvan@c5a9dd2

Not sure where the issue is, but it seems there is some problem with the zside and wafer u,v

[snwebb]

Thanks @jbsauvan . I found a few issues that contributed to the problem:

Firstly there was a one character error where there was a U rather than a V
Secondly the definition of the subdetector was not fully consistent throughout (I made it more like the HGCalDetId).
Thirdly the definition of the z-side (i.e. does 0 or 1 mean negative or positive) was opposite in HGCalModuleDetId and HGCalTriggerDetId. The reason for this is because the definitions in HGCalDetId (where I copied the function from) and HGCalTriggerDetId were already inverted.

[snwebb]

I fixed a couple of issues:

• I've set the hSc_num_panels_per_sector_ to 8 (since there are 288 cells in 360º)
• There was a bug in the way I got the TC eta from the module - I've now put it back to what is was like for the V2Impl

The tests are passed in the tester program, although it crashes at the end with the following error:

----- Begin Fatal Exception 02-Mar-2021 19:28:05 CET-----------------------
An exception of category 'StdException' occurred while
   [0] Processing  stream begin Run run: 1 stream: 0
   [1] Calling method for module HGCalTriggerGeomTesterV9Imp3/'hgcaltriggergeomtester'
Exception Message:
A std::exception was thrown.
_Map_base::at

[jbsauvan]

This is due to an issue in fetching the number of links in modules at

cmssw/L1Trigger/L1THGCal/plugins/geometries/HGCalTriggerGeometryV9Imp3.cc

Lines 537 to 538 in 47fb05c

	links = links_per_module_.at(
	packLayerWaferId(module_det_id_si.layer(), module_det_id_si.moduleU(), module_det_id_si.moduleV()));

[snwebb]

Thank you - I have now added "disconnected modules" (i.e. modules that currently have no lpGBT links) to the json config file. The tester class now runs without errors.

[jbsauvan]

Thanks @snwebb
I will update the tester class to be able to check also lpgbt and boards mappings.
Just one thing that can be changed in the meantime: the number of links per module should be the number of elinks and not lpgbts. The lpgbts are not directly connected to modules and this is the number of elinks that is important for instance for BestChoice.

[snwebb]

Thanks - I have updated the code to take this into account.

[jbsauvan]

Hi @snwebb
I have checked in more details the lpgbt, stage-1 and stage-2 mappings.

Stage-1 and lpgbt mappings

I have added some checks for the module <-> stage-1 mapping and found some issues [1]. The commit containing these updates is is jbsauvan@61d12bb
I think we miss proper detids for lpgbts and FPGAs.
For instance, here:

cmssw/L1Trigger/L1THGCal/plugins/geometries/HGCalTriggerGeometryV9Imp3.cc

Lines 591 to 600 in 1f37d1a

	HGCalTriggerGeometryV9Imp3::geom_set HGCalTriggerGeometryV9Imp3::getLpgbtsFromModule(const unsigned module_id) const {
	geom_set lpgbt_ids;
	auto module_itrs = module_to_lpgbts_.equal_range(module_id);
	for (auto module_itr = module_itrs.first; module_itr != module_itrs.second; module_itr++) {
	lpgbt_ids.emplace(module_itr->second);
	}
	return lpgbt_ids;
	}

what is retrieved are the lpgbt numbers within one sector (as stored in the module_to_lpgbts_ map), while the module id is defined for the whole HGCAL (HGCalModuleDetId).

So detids valid for the whole HGCAl should be defined for lpgbts as well as for Stage-1 FPGAs. Same thing for Stage-2 FPGAs, but that's a bit different in that case. Indeed Stage-2 FPGAs are covering two Stage-1 sectors, so they would probably need a different detid scheme compared to Stage-1 FPGAs.

Stage-2 mapping

Currently in the json it seems that there is a number of Stage-2 FPGA equal to the TMT period (18). But we actually won't simulate the Time Multiplexing, since virtually all the TMT FPGAs will do the exact same thing but on different events. So I would remove the TMT duplicates in the json.

[1]

Checking module -> stage-1 -> module consistency                               
Error:                                                                                                                     
 Module 3003562773(HGCalModuleDetId::HFNose:EE:HSil:HScin=0:0:0:0 type= 0 z= -1 layer=3 sector=3 module(u,v)= (5,1))
 has not been found in                                                         
 stage-1 HGCalModuleDetId::HFNose:EE:HSil:HScin=0:1:0:0 type= 2 z= 1 layer=1 sector=1 module(u,v)= (6,11)
 Available modules are:                                                                                                       
 Connected lpgbts are:                                                    
                                                                                                     
%MSG-w HGCalTriggerGeometryTester:   HGCalTriggerGeomTesterV9Imp3:hgcaltriggergeomtester@streamBeginRun  11-Mar-2021 11:01:39 CET Run: 1 Stream: 0
Problem with the trigger geometry detected. Only the basic cells tree will be filled   

[snwebb]

Thanks @jbsauvan
I've added a new class HGCalBackendDetId to handle lpGBTs and Stage 1 FPGAs as well as fixing various bugs. The tester runs fine now.
I still need to add something to handle the Stage 2 boards.

[jbsauvan]

Hi @snwebb
One more thing that I realize could be useful is a function to go directly from modules to Stage-1 FPGA, without the need to call manually module->lpgbt and lpgbt->stage-1 (as I did in the tester class). The best would probably be to fill an additional map module->stage-1 in the initialization, that can be directly accessed as the other maps.

[snwebb]

Thanks @jbsauvan

I have now added the structure to label the links between Stage1 and Stage2 FPGAs (which would anyway be needed I believe), and in this way obtain the mapping between the Stage1 and Stage2 FPGAs.

Stage2 FPGAs cover ~half the detector although there are 3 per end-cap (ignoring time multiplexing). Therefore I have kept the sector identifier also for Stage2, where the sector is taken as the one corresponding to the maximum overlap with Stage 1 (i.e. the lower value in phi). The sector for the Stage1 links is defined to be the same one as connected to the Stage1 FPGA (which might be different to the Stage2 FPGA it is connected to).

I have also added the direct module->stage-1 maps and associated function.

[snwebb]

Thanks a lot, I have implemented all of your suggestions.

[jbsauvan]

After running some tests, I noticed that the current detid scheme chosen for HGCalModuleDetId cannot work, because it uses the same detector and subdetector types as HGCalDetId.
It means that there is no way to distinguish a HGCalModuleDetId from a HGCalDetId. But the other bits are organized differently; in particular the layer is not encoded in the same way in HGCalDetId and HGCalModuleId. So, for instance, when the layer number of a module is retrieved in HGCalTriggerTools [1], it cannot correctly get both HGCalDetId and HGCalModuleId layers.

What can be done is to still use the Forward detector type, but switch to HGCTrigger subdetector [2]. Which means that 4+3 bits are reserved for det and subdet [3], and only the remaining bits should be used for HGCalModuleDetId specific fields.
The same thing should be done for HGCalBackendDetId, and some bit should probably be used to distinguish HGCalModuleDetId and HGCalBackendDetId.

[1]

cmssw/L1Trigger/L1THGCal/src/HGCalTriggerTools.cc

Line 111 in 9e67c40

unsigned HGCalTriggerTools::layer(const DetId& id) const {

[2]

cmssw/DataFormats/ForwardDetId/interface/ForwardSubdetector.h

Line 12 in 9e67c40

HGCTrigger = 7

[3]

cmssw/DataFormats/DetId/interface/DetId.h

Lines 19 to 22 in 9e67c40

	static const int kDetMask = 0xF;
	static const int kSubdetMask = 0x7;
	static const int kDetOffset = 28;
	static const int kSubdetOffset = 25;

[snwebb]

Thanks @jbsauvan for the comments. I have edited the HGCalModuleDetId and HGCalBackendDetId class, such that the sub-detector is HGCTrigger and there is now an additional function triggerSubdetId() for HGCalModuleDetId to determine whether a module is silicon HF or HB, scintillator, or nose. There is also a bit which distinguishes the two classes as suggested.

[jbsauvan]

Thanks @snwebb
Could you also update the HGCalTriggerTools to take into account HGCalModuleDetId?
In layer(), isEm(), isSilicon(), zside().

[snwebb]

Thanks a lot for the comments - I've performed the updates to HGCalTriggerTools as well.

[jbsauvan]

Thanks @snwebb
Now everything seems to run fine. I will make some final checks and go through the code one last time.

[jbsauvan]

Hi @snwebb
I've seen issues in the TC distributions using BestChoice, which is driven by the number of elinks. Looking back at the number of links distribution given by the geometry tester, the numbers don't seem to correspond to what is in the json file.
Could you check where the issue could come from?
Thanks!

[snwebb]

Hi @jbsauvan
Thanks for noticing - the number of elinks for the scintillator modules was hard-coded rather than taking the values from the json file. Could you check if everything is now as expected?

[jbsauvan]

Actually, I see issues rather in the ECAL part.

For instance here is the distribution of the number of elinks written in the output ntuple of the tester:

Here I looked at the modules with 0 elinks (red circles) function of the layer and radius:

There are much more modules with 0 links on the boundaries than expected.

Same thing looking in the first layer:

[snwebb]

Thanks @jbsauvan - I found a bug whereby the layer was being obtained from the incorrect module class. This is now fixed and the links distribution looks more sensible.

[jbsauvan]

Thanks @snwebb !
That looks much better now. I checked TC distributions with Threshold, BestChoice and STC.
They are all very close to V9Imp2, there are just small differences in the Scintillator part at low eta. e.g. here [1] for the Threshold option (which appeared with your latest scintillator change).
I suspect these are due to disconnected "modules" in the scintillator. Do we expect disconnected scintillator modules from the json?

[1] (red=V9Imp2, blue=V9Imp3)

[snwebb]

I've checked and these scintillator modules don't exist in the json file (so they are treated as disconnected - previously each module was hard-coded to have exactly 1 link). This may be expected, as I remember Paul saying that the scintillator section of his original file was filled 'by-hand' since only the silicon mapping was provided to him.

[jbsauvan]

Do you know how the scintillator modules are defined in the json? Is it the same as in getModuleFromTriggerCell()?
Could you check where the numbers of elinks in the scintillator come from? So far we have always been using 2 elinks per scintillator "module".

[snwebb]

Yes the function getModuleFromTriggerCell() should work for the scintillator too (I basically copied the code there from the V2 with minimal changes).
The original list of scintillator modules I have been used is here. https://github.com/snwebb/hgcal-linkmapping/blob/master/data/FeMappingV7.txt#L4886-L5508
The 13th column is the number of trigger e-links, which may be 1, 2 or 3 it seems.

[jbsauvan]

@snwebb please find here remaining comments on tiny details

[jbsauvan]

These includes should not be needed in this file

[snwebb]

The first include can be omitted, but the other two are necessary. I have however moved these to HGCalModuleDetId.h and HGCalBackendDetId.h which might make it cleaner/clearer.

[snwebb]

Hi @jbsauvan - thanks for the comments, which I have all implemented.

I have also added a new version of hgcal_trigger_link_mapping_v1.json using an updated definition of the scintillator module that Paul provided me. If this causes too many issues, then I can revert that change for now (but hopefully the changes are minor and this mapping is slightly more realistic).

[jbsauvan]

Thanks @snwebb
The TC distributions using the new json look fine, just a bit more different in the scintillator section (see e.g. [1] for BC with PU-driven allocation).

Now you can:

• run scram b code-format
• rebase on top of the latest devel branch hgc-tpg-devel-CMSSW_11_3_0_pre6, without the changes already in CMSSW (related to HGCalGeomRotation)
• and squash

[1] (red=V9Imp2, blue=V9Imp3), BC PU-driven in both cases

[snwebb]

Thanks @jbsauvan , that's done.