Changes for new HCal segmentations

[Archil-AD]

BEGINRELEASENOTES

• The CellPositionsHCalPhiThetaSegTool is modified for new HCal segmentations: now it uses the position() method from segmentation classes that returns directly the global position of the cells
• TopoClustering is enable for HCal Endcap
• CaloTowerToolFCCee is updated for new HCal segmentation classes: FCCSWHCalPhiTheta_k4geo and FCCSWHCalPhiRow_k4geo
• CreateFCCeeCaloNeighbours code is updated for new HCal segmentations: FCCSWHCalPhiTheta_k4geo and FCCSWHCalPhiRow_k4geo

ENDRELEASENOTES

[giovannimarchiori]

Hi @Archil-AD,

thanks a lot, very useful!

I didn't do a full review of the code but I looked at the overall structure and I have a couple of doubts about (1) why the fixed size SW clustering does not work (or did I miss it) for the phi-row segmentation and (2) whether rather than overriding the phi-theta HCAL positioning tool it might have been better to add the handling of the new phi-theta (non-theta-projective) readout in a new positioning tool - this is rather a question for @BrieucF

[Archil-AD]

Hi @giovannimarchiori ,

I was more focused on the topo-clustering, therefore, I did not implement the HCal phi-row segmentation in SW clustering.
Now with the last commit it is implemented.

Best regards,
Archil

[BrieucF]

Hi @giovannimarchiori ,

I was more focused on the topo-clustering, therefore, I did not implement the HCal phi-row segmentation in SW clustering. Now with the last commit it is implemented.

Best regards, Archil

Thanks Archil! Can you reflect this also in the release note of the pull request?

[BrieucF]

We need to first merge key4hep/k4geo#409 to get the test working

[Archil-AD]

Thanks Archil! Can you reflect this also in the release note of the pull request?

Hi Brieuc, the note now mentions the clustering is updated for both segmentation classes.

Best regards,
Archil

[BrieucF]

I launched the tests with the new nightlies. @giovannimarchiori all good for you?

[giovannimarchiori]

I launched the tests with the new nightlies. @giovannimarchiori all good for you?

Hi @BrieucF I had some doubts and tagged you where I would like to have your opinion

[BrieucF]

Hi Archil, can you fix the tests: https://github.com/HEP-FCC/k4RecCalorimeter/actions/runs/12099331416/job/33906423969?pr=128#step:3:5266?

[BrieucF]

Hi @Archil-AD,

thanks a lot, very useful!

I didn't do a full review of the code but I looked at the overall structure and I have a couple of doubts about (1) why the fixed size SW clustering does not work (or did I miss it) for the phi-row segmentation and (2) whether rather than overriding the phi-theta HCAL positioning tool it might have been better to add the handling of the new phi-theta (non-theta-projective) readout in a new positioning tool - this is rather a question for @BrieucF

Sorry I missed that. (1) is addressed, right? For (2), if I understand correctly your doubt @giovannimarchiori , I agree that we should keep CellPositionsHCalPhiThetaSegTool with the fictive grid FCCSWGridPhiTheta_k4geo 'alive' for reproducibility. It was used in previous versions of ALLEGRO, right?

[giovannimarchiori]

For (2), if I understand correctly your doubt @giovannimarchiori , I agree that we should keep CellPositionsHCalPhiThetaSegTool with the fictive grid FCCSWGridPhiTheta_k4geo 'alive' for reproducibility. It was used in previous versions of ALLEGRO, right?

Yes!

[Archil-AD]

Hi @BrieucF and @giovannimarchiori ,

should I revert back the CellPositionsHCalPhiThetaSegTool and create a new tool?

Best regards,
Archil

[BrieucF]

Hi @BrieucF and @giovannimarchiori ,

should I revert back the CellPositionsHCalPhiThetaSegTool and create a new tool?

Best regards, Archil

Can't we keep both alive by adding an if here:

k4RecCalorimeter/RecFCCeeCalorimeter/src/components/CellPositionsHCalPhiThetaSegTool.cpp

Lines 32 to 49 in fa85e57

	if(segmentationType == "FCCSWHCalPhiTheta_k4geo")
	{
	// get PhiTheta segmentation
	m_segmentation = dynamic_cast<dd4hep::DDSegmentation::FCCSWHCalPhiTheta_k4geo *>(
	m_geoSvc->getDetector()->readout(m_readoutName).segmentation().segmentation());
	}
	if(segmentationType == "FCCSWHCalPhiRow_k4geo")
	{
	// get PhiRow segmentation
	m_segmentation = dynamic_cast<dd4hep::DDSegmentation::FCCSWHCalPhiRow_k4geo *>(
	m_geoSvc->getDetector()->readout(m_readoutName).segmentation().segmentation());
	}
	if (m_segmentation == nullptr)
	{
	error() << "There is no phi-theta segmentation!!!!" << endmsg;
	return StatusCode::FAILURE;
	}

?

I think you also need to add another case here: https://github.com/HEP-FCC/k4RecCalorimeter/pull/128/files#diff-292f794eb82781d28954a82a7e6a84e1b4b9d6c0ffa27859a539b9d68943dd05R140-R146

[Archil-AD]

Hi @BrieucF and @giovannimarchiori ,
should I revert back the CellPositionsHCalPhiThetaSegTool and create a new tool?
Best regards, Archil

Can't we keep both alive by adding an if here:

k4RecCalorimeter/RecFCCeeCalorimeter/src/components/CellPositionsHCalPhiThetaSegTool.cpp

Lines 32 to 49 in fa85e57

	if(segmentationType == "FCCSWHCalPhiTheta_k4geo")
	{
	// get PhiTheta segmentation
	m_segmentation = dynamic_cast<dd4hep::DDSegmentation::FCCSWHCalPhiTheta_k4geo *>(
	m_geoSvc->getDetector()->readout(m_readoutName).segmentation().segmentation());
	}
	if(segmentationType == "FCCSWHCalPhiRow_k4geo")
	{
	// get PhiRow segmentation
	m_segmentation = dynamic_cast<dd4hep::DDSegmentation::FCCSWHCalPhiRow_k4geo *>(
	m_geoSvc->getDetector()->readout(m_readoutName).segmentation().segmentation());
	}
	if (m_segmentation == nullptr)
	{
	error() << "There is no phi-theta segmentation!!!!" << endmsg;
	return StatusCode::FAILURE;
	}

?

can not be done by just adding an if here, since the original CellPositionsHCalPhiThetaSegTool tool relies on the geo builder to calculate the radius of each layer, then the local position retrieved from the FCCSWGridPhiTheta_k4geo segmentation is converted into the global position using the calculated radii.
New segmentation classes calculate layer radii directly from the xml files and then they are able to return directly the global positions, so there is no need to recalculate radii in positioning tool. Therefore, I have removed the code that calculates layer radii in positioning tool

[Archil-AD]

if I revert back the layer radii calculation code in the positioning tool then I think it is possible to modify the code to work for old and new segmentation classes.
Should I do like that?

[BrieucF]

if I revert back the layer radii calculation code in the positioning tool then I think it is possible to modify the code to work for old and new segmentation classes. Should I do like that?

If not too complicated yes, that would be nice!

[Archil-AD]

if I revert back the layer radii calculation code in the positioning tool then I think it is possible to modify the code to work for old and new segmentation classes. Should I do like that?

If not too complicated yes, that would be nice!

ok, let me see what I can

[Archil-AD]

if I revert back the layer radii calculation code in the positioning tool then I think it is possible to modify the code to work for old and new segmentation classes. Should I do like that?

If not too complicated yes, that would be nice!

Done.

[Archil-AD]

I think you also need to add another case here: https://github.com/HEP-FCC/k4RecCalorimeter/pull/128/files#diff-292f794eb82781d28954a82a7e6a84e1b4b9d6c0ffa27859a539b9d68943dd05R140-R146

Done.

[Archil-AD]

Hi Archil, can you fix the tests: https://github.com/HEP-FCC/k4RecCalorimeter/actions/runs/12099331416/job/33906423969?pr=128#step:3:5266?

as I understand, it needs some updates in:
https://github.com/HEP-FCC/k4RecCalorimeter/blob/main/RecFCCeeCalorimeter/tests/options/ALLEGRO_o1_v03_digi_reco.py

should I rewrite this for new HCal segmentations? and add the clustering in the HCal endcap?

[BrieucF]

Hi Archil, can you fix the tests: https://github.com/HEP-FCC/k4RecCalorimeter/actions/runs/12099331416/job/33906423969?pr=128#step:3:5266?

as I understand, it needs some updates in: https://github.com/HEP-FCC/k4RecCalorimeter/blob/main/RecFCCeeCalorimeter/tests/options/ALLEGRO_o1_v03_digi_reco.py

should I rewrite this for new HCal segmentations? and add the clustering in the HCal endcap?

Yes :-) can you also apply these changes here: https://github.com/HEP-FCC/FCC-config/blob/main/FCCee/FullSim/ALLEGRO/ALLEGRO_o1_v03/run_digi_reco.py ?

[Archil-AD]

Hi Archil, can you fix the tests: https://github.com/HEP-FCC/k4RecCalorimeter/actions/runs/12099331416/job/33906423969?pr=128#step:3:5266?

as I understand, it needs some updates in: https://github.com/HEP-FCC/k4RecCalorimeter/blob/main/RecFCCeeCalorimeter/tests/options/ALLEGRO_o1_v03_digi_reco.py
should I rewrite this for new HCal segmentations? and add the clustering in the HCal endcap?

Yes :-) can you also apply these changes here: https://github.com/HEP-FCC/FCC-config/blob/main/FCCee/FullSim/ALLEGRO/ALLEGRO_o1_v03/run_digi_reco.py ?

OK, I will do that.
just there is one more point, the neighbours map must be updated to avoid a crash of topo-clustering.
currently the test download the following file:
https://fccsw.web.cern.ch/fccsw/filesForSimDigiReco/ALLEGRO/ALLEGRO_o1_v03/neighbours_map_ecalB_thetamodulemerged_hcalB_thetaphi.root

where should I place the updated neighbours map to be accessible by the test?

[Archil-AD]

Hi Archil, can you fix the tests: https://github.com/HEP-FCC/k4RecCalorimeter/actions/runs/12099331416/job/33906423969?pr=128#step:3:5266?

as I understand, it needs some updates in: https://github.com/HEP-FCC/k4RecCalorimeter/blob/main/RecFCCeeCalorimeter/tests/options/ALLEGRO_o1_v03_digi_reco.py
should I rewrite this for new HCal segmentations? and add the clustering in the HCal endcap?

Yes :-) can you also apply these changes here: https://github.com/HEP-FCC/FCC-config/blob/main/FCCee/FullSim/ALLEGRO/ALLEGRO_o1_v03/run_digi_reco.py ?

OK, I will do that. just there is one more point, the neighbours map must be updated to avoid a crash of topo-clustering. currently the test download the following file: https://fccsw.web.cern.ch/fccsw/filesForSimDigiReco/ALLEGRO/ALLEGRO_o1_v03/neighbours_map_ecalB_thetamodulemerged_hcalB_thetaphi.root

where should I place the updated neighbours map to be accessible by the test?

[BrieucF]

Hi Archil, can you fix the tests: https://github.com/HEP-FCC/k4RecCalorimeter/actions/runs/12099331416/job/33906423969?pr=128#step:3:5266?

as I understand, it needs some updates in: https://github.com/HEP-FCC/k4RecCalorimeter/blob/main/RecFCCeeCalorimeter/tests/options/ALLEGRO_o1_v03_digi_reco.py
should I rewrite this for new HCal segmentations? and add the clustering in the HCal endcap?

Yes :-) can you also apply these changes here: https://github.com/HEP-FCC/FCC-config/blob/main/FCCee/FullSim/ALLEGRO/ALLEGRO_o1_v03/run_digi_reco.py ?

OK, I will do that. just there is one more point, the neighbours map must be updated to avoid a crash of topo-clustering. currently the test download the following file: https://fccsw.web.cern.ch/fccsw/filesForSimDigiReco/ALLEGRO/ALLEGRO_o1_v03/neighbours_map_ecalB_thetamodulemerged_hcalB_thetaphi.root

where should I place the updated neighbours map to be accessible by the test?

You can place the new neighbor map here /eos/project/f/fccsw-web/www/filesForSimDigiReco/ALLEGRO/ALLEGRO_o1_v03/ and here /eos/project/f/fccsw-web/www/filesForSimDigiReco/ALLEGRO/ALLEGRO_o1_v04/. I just gave you the rights to write there, it may take some time to synchronize.

[Archil-AD]

hi @BrieucF and @giovannimarchiori ,

I have updated the RecFCCeeCalorimeter/tests/options/ALLEGRO_o1_v03_digi_reco.py file and now tests seem to be passed in nightly.
Please let me know if the changes look fine with you, and if so, I can apply same changes on:
https://github.com/HEP-FCC/FCC-config/blob/main/FCCee/FullSim/ALLEGRO/ALLEGRO_o1_v03/run_digi_reco.py

best regards,
Archil

[BrieucF]

LGTM (the stable release tests are expected to fail). Thanks a lot Archil! Waiting for Giovanni to sign-off as well.

[giovannimarchiori]

I believed we tried to tie one positioning tool to one segmentation but maybe this is not so crucial and we do not need to enforce it strictly.
Being this said, the tool was called CellPositionsHCalPhiThetaSegTool because it was used for positioning the cells with the phi-theta grid segmentation. If it's used now also for the positioning of cells with different segmentations such as HCalPhiTheta and HCalPhiRow, do we need to find a better name for it? However, even if we do, we can also do it in a separate MR - I let @BrieucF comment and merge if OK

[BrieucF]

I believed we tried to tie one positioning tool to one segmentation but maybe this is not so crucial and we do not need to enforce it strictly. Being this said, the tool was called CellPositionsHCalPhiThetaSegTool because it was used for positioning the cells with the phi-theta grid segmentation. If it's used now also for the positioning of cells with different segmentations such as HCalPhiTheta and HCalPhiRow, do we need to find a better name for it? However, even if we do, we can also do it in a separate MR - I let @BrieucF comment and merge if OK

Thanks Giovanni. Yes, given Archil's timeline, I'll merge now. We can indeed apply this change later in case having one positioning tool for multiple segmentations reveals itself to be confusing.

[BrieucF]

Thanks a lot Archil!