HCal readout segmentation update #409

Archil-AD · 2024-11-09T21:13:08Z

BEGINRELEASENOTES

HCalBarrel_TileCal_v03.xml and HCalEndcaps_ThreeParts_TileCal_v03.xml files have been created with new readouts: phi-theta and phi-row segmentation
Two new segmentation classes are created for HCal: FCCSWHCalPhiTheta_k4geo and FCCSWHCalPhiRow_k4geo
- New segmentation classes can calculate the global positions of the cells and determine the cell neighbours for TopoClustering

ENDRELEASENOTES

detectorSegmentations/src/FCCSWHCalPhiRow_k4geo.cpp

FCCee/ALLEGRO/compact/ALLEGRO_o1_v03/HCalBarrel_TileCal_v03.xml

FCCee/ALLEGRO/compact/ALLEGRO_o1_v03/HCalEndcaps_ThreeParts_TileCal_v03.xml

fixing typo Co-authored-by: Andre Sailer <andre.philippe.sailer@cern.ch>

…ow segmentation

…mentation

atolosadelgado · 2024-11-26T14:48:52Z

Hi @Archil-AD

Thank you for this PR!

I check that it runs in version 3 and 4 of ALLEGRO. Also I noticed that the corresponding test for ALLEGRO version 04 is missing, could you add these lines in the file test/CMakeLists.txt, after the test for ALLEGRO v03?

#--------------------------------------------------
# test for ALLEGRO o1 v04
if(DCH_INFO_H_EXIST)
SET( test_name "test_ALLEGRO_o1_v04" )
ADD_TEST( t_${test_name} "${CMAKE_INSTALL_PREFIX}/bin/run_test_${PackageName}.sh"
  ddsim --compactFile=${CMAKE_CURRENT_SOURCE_DIR}/../FCCee/ALLEGRO/compact/ALLEGRO_o1_v04/ALLEGRO_o1_v04.xml --runType=batch -G -N=1 --outputFile=testALLEGRO_o1_v04.root --gun.direction "1,1.2,0.2" )
SET_TESTS_PROPERTIES( t_${test_name} PROPERTIES FAIL_REGULAR_EXPRESSION  " Exception;EXCEPTION;ERROR;Error" )
endif()

I have specified a random direction --gun.direction "1,1.2,0.2", but you can specify any other you consider better suited (I think default direction is the Z axis, which is not very useful to test when a particle crosses a detector).

Thank you for your time.

Best,
Alvaro

Archil-AD · 2024-11-26T15:10:09Z

Hi @Archil-AD

Thank you for this PR!

I check that it runs in version 3 and 4 of ALLEGRO. Also I noticed that the corresponding test for ALLEGRO version 04 is missing, could you add these lines in the file test/CMakeLists.txt, after the test for ALLEGRO v03?
#--------------------------------------------------
# test for ALLEGRO o1 v04
if(DCH_INFO_H_EXIST)
SET( test_name "test_ALLEGRO_o1_v04" )
ADD_TEST( t_${test_name} "${CMAKE_INSTALL_PREFIX}/bin/run_test_${PackageName}.sh"
  ddsim --compactFile=${CMAKE_CURRENT_SOURCE_DIR}/../FCCee/ALLEGRO/compact/ALLEGRO_o1_v04/ALLEGRO_o1_v04.xml --runType=batch -G -N=1 --outputFile=testALLEGRO_o1_v04.root --gun.direction "1,1.2,0.2" )
SET_TESTS_PROPERTIES( t_${test_name} PROPERTIES FAIL_REGULAR_EXPRESSION  " Exception;EXCEPTION;ERROR;Error" )
endif()
I have specified a random direction --gun.direction "1,1.2,0.2", but you can specify any other you consider better suited (I think default direction is the Z axis, which is not very useful to test when a particle crosses a detector).

Thank you for your time.

Best, Alvaro

Hi @atolosadelgado ,
thanks a lot. I have added this test.
Best regards,
Archil

BrieucF · 2024-11-27T16:53:17Z

Hi Archil, the test crash because there is a check that two files with the same name can not have different content. In this particular case, I actually don't see why they should have different contents (HCalBarrel_TileCal_v03.xml and HCalEndcaps_ThreeParts_TileCal_v03.xml in ALLEGRO v03 and ALLEGRO v04). Can you check? By the way, for now, in ALLEGRO v04 (which is still under preparation), files that were not modified have been set as symlinks.

Archil-AD · 2024-11-28T10:54:49Z

Hi Archil, the test crash because there is a check that two files with the same name can not have different content. In this particular case, I actually don't see why they should have different contents (HCalBarrel_TileCal_v03.xml and HCalEndcaps_ThreeParts_TileCal_v03.xml in ALLEGRO v03 and ALLEGRO v04). Can you check? By the way, for now, in ALLEGRO v04 (which is still under preparation), files that were not modified have been set as symlinks.

Hi Brieuc, I found that the difference between ALLEGRO v03 vs ALLEGRO v04 was "PhiRow" vs "RowPhi" in the readout name. Now I have created the symlinks of HCalBarrel_TileCal_v03.xml and HCalEndcaps_ThreeParts_TileCal_v03.xml in ALLEGRO v04 from ALLEGRO v03).
many thanks,
Best regards,
Archil

giovannimarchiori

Hello @Archil-AD

thanks a lot for this extremely useful contribution!
I added some suggestions to the phi-row segmentation but similar ones apply to the phi-theta class.
I would suggest:

in the top of the .h files to add a bit more of general documentation on the segmentation (maybe pointing also to your presentation on that, but as much as possible self-contained)
in the .h files, expand a bit the comments
replace hardcoded "row" or "layer" with m_rowID, m_layerID
to make the code a bit more generic so that it could work also with a 2-part endcap for instance, or even a 1-part endcap, introducing one more parameter that acts as a switch between barrel and endcap, rather than relying on the number of parts to decide if a system is barrel or endcap

Thanks again, cheers,
Giovanni

giovannimarchiori · 2024-11-28T14:20:19Z

detectorSegmentations/include/detectorSegmentations/FCCSWHCalPhiTheta_k4geo.h

+        */
+        inline double offsetPhi() const { return m_offsetPhi; }
+
+        /**  Get the vector of theta bins (cells) in a give layer.


give -> given

giovannimarchiori · 2024-11-28T14:20:57Z

detectorSegmentations/include/detectorSegmentations/FCCSWHCalPhiTheta_k4geo.h

+	std::vector<uint64_t> neighbours(const CellID& cID, bool aDiagonal) const;
+
+        /**  Calculate layer radii and edges in z-axis, then define cell edges in each layer using defineCellEdges().
+        */


might help to state which member variables are calculated/updated

giovannimarchiori · 2024-11-28T14:21:21Z

detectorSegmentations/include/detectorSegmentations/FCCSWHCalPhiTheta_k4geo.h

+        *   Logic:
+        *      1) Find theta bin centers that fit within the given layer; 
+        *      2) Define a cell edge in z-axis as the middle of each pair of theta bin centers
+        *   @param[in] layer index 


might help to state which member variables are calculated/updated

giovannimarchiori · 2024-11-28T14:22:53Z

detectorSegmentations/include/detectorSegmentations/FCCSWHCalPhiTheta_k4geo.h

+        inline int phiBins() const { return m_phiBins; }
+
+        /**  Get the coordinate offset in azimuthal angle.
+        *   return The offset in phi.


is that the phi coordinate of the center or edge of the cell with phiID = 0? might help to clarify here

it's a center. Now this is mentioned.

giovannimarchiori · 2024-11-28T14:24:17Z

detectorSegmentations/include/detectorSegmentations/FCCSWHCalPhiTheta_k4geo.h

+        }
+
+        /**  Get the coordinate offset in z-axis.
+        *   return The offset in z.


can you please write why this is a vector (what is being indexed? layer?) and how is the offset defined?

Added the following:
* Offset is the middle position of the Barrel or each section of the Endcap.
* For the Barrel, the vector size is 1, while for the Endcap - number of section.

giovannimarchiori · 2024-11-28T14:58:43Z

detectorSegmentations/src/FCCSWHCalPhiRow_k4geo.cpp

+  // get the row number from volumeID (starts from 0!)
+  int nrow = _decoder->get(vID, "row");
+  // get the layer number from volumeID
+  uint layer = _decoder->get(vID,"layer");


"layer"->m_layerID (if you add it)?

giovannimarchiori · 2024-11-28T15:01:06Z

detectorSegmentations/src/FCCSWHCalPhiRow_k4geo.cpp

+  _decoder->set(cID, m_rowID, idx);
+  _decoder->set(cID, m_phiID, positionToBin(dd4hep::DDSegmentation::Util::phiFromXYZ(globalPosition), 2 * M_PI / (double)m_phiBins, m_offsetPhi));
+
+  // For endcap, the volume ID comes with "type" field information which would screw up the topo-clustering,


aren't HCAL barrel and endcap already differentiated by system ID? Is type set to something different from 0 elsewhere?

yes, they are already differentiated by systemID.
The "type" is set different from zero in the geo builder:
https://github.com/key4hep/k4geo/blob/main/detector/calorimeter/HCalThreePartsEndcap_o1_v02_geo.cpp#L242

I am not sure if it is used elsewhere, it was already like that and I did not touched it.
I guess this field is set to differentiate different parts of the endcap.

giovannimarchiori · 2024-11-28T15:02:23Z

detectorSegmentations/src/FCCSWHCalPhiRow_k4geo.cpp

+   std::vector<int> maxLayerIdEndcap = {-1, 0, 0};
+   if(m_offsetZ.size() > 1)
+   {
+     uint Nl = m_numLayers.size()/3;


again, hardcoded 3 ..

fixed. the code is generalized for N-parts endcap

giovannimarchiori · 2024-11-28T15:05:05Z

detectorSegmentations/src/FCCSWHCalPhiRow_k4geo.cpp

+   if(m_radii.empty()) return minMaxLayerId;
+
+
+   std::vector<int> minLayerIdEndcap = {0, 0, 0};


the vectors have size=3 because you assume at most 3 parts, right? this could be generalised

fixed. the code is generalized for N-parts endcap

giovannimarchiori · 2024-11-28T15:06:37Z

detectorSegmentations/src/FCCSWHCalPhiRow_k4geo.cpp

+   int minLayerId = -1;
+   int maxLayerId = -1;
+
+   int currentLayerId = _decoder->get(cID,"layer");


Archil-AD · 2024-11-30T15:47:34Z

Hello @giovannimarchiori ,

many thanks for your comments. I have tried to implement them in the last commit.

Best regards,
Archil

… clustering

giovannimarchiori · 2024-12-02T09:54:30Z

detectorSegmentations/src/FCCSWHCalPhiTheta_k4geo.cpp

+
+  auto pos = positionFromRThetaPhi(radius, theta(cID), phi(cID));
+
+  // retrun the position with corrected z corrdinate to match to the geometric center


giovannimarchiori · 2024-12-02T09:57:20Z

Hello @Archil-AD
thanks a lot for all the changes you made, the code looks very good to me. May I ask you one last thing, to also comment the xml files? In particular for the endcap, which is not self-explanatory, it would help if you could add a few comments inline to explain the format of the various rows and what they define.
Thanks a lot!
Cheers,
Giovanni

Archil-AD · 2024-12-02T12:06:16Z

Hello @Archil-AD thanks a lot for all the changes you made, the code looks very good to me. May I ask you one last thing, to also comment the xml files? In particular for the endcap, which is not self-explanatory, it would help if you could add a few comments inline to explain the format of the various rows and what they define. Thanks a lot! Cheers, Giovanni

Hi @giovannimarchiori ,
I have tried to add some description in the xml files.
Best regards,
Archil

giovannimarchiori · 2024-12-02T13:06:27Z

detectorSegmentations/src/FCCSWHCalPhiTheta_k4geo.cpp

+
+    if(m_detLayout==0)  dd4hep::printout(dd4hep::INFO, "FCCSWHCalPhiRow_k4geo","Barrel configuration found!");
+    else dd4hep::printout(dd4hep::INFO, "FCCSWHCalPhiRow_k4geo","EndCap configuration found!");
+


Would it make sense here to add some sanity checks of the xml:

m_offsetZ.size()==m_offsetR.size()

m_dRlayer.size()==m_offsetR.size()

m_widthZ.size()==m_offsetR.size()

m_offsetZ.size()==1 if barrel

m_numLayers.size() % m_offsetZ.size() == 0

m_gridSizeRow.size() == sum of contents of m_numLayers
?
And similarly for the FCCSWCHCalPhiRow_k4geo?

indeed, that would help to debug if someone will try to update the xml files.
I have added it.

giovannimarchiori · 2024-12-02T13:07:27Z

Hello @Archil-AD thanks a lot for all the changes you made, the code looks very good to me. May I ask you one last thing, to also comment the xml files? In particular for the endcap, which is not self-explanatory, it would help if you could add a few comments inline to explain the format of the various rows and what they define. Thanks a lot! Cheers, Giovanni

Hi @giovannimarchiori , I have tried to add some description in the xml files. Best regards, Archil

Excellent! The description is very useful.
I left one last comment about checking that the vectors in the xml have proper sizes, that's it from my side!
Cheers,
Giovanni

giovannimarchiori · 2024-12-02T14:52:17Z

detectorSegmentations/src/FCCSWHCalPhiTheta_k4geo.cpp

+    // check if all necessary variables are available
+    if(m_detLayout==-1 || m_offsetZ.empty() || m_widthZ.empty() || m_offsetR.empty() || m_numLayers.empty() || m_dRlayer.empty())
+    {
+       dd4hep::printout(dd4hep::ERROR, "FCCSWHCalPhiRow_k4geo","Please check the readout description in the XML file!\n%s",


should be PhiTheta... also in following if (..) blocks

thanks for spotting that, I have fixed it

giovannimarchiori · 2024-12-02T14:59:06Z

OK for me, thanks again!

giovannimarchiori

Approved from my side

Archil Durglishvili added 6 commits October 30, 2024 15:50

HCal new readout implementation

850a4dc

Additional PhiRow segmentation for HCal

3b008db

Updates for HCal PhiTheta segmentation: determine neighbours

3a79ed0

fix in the HCal neighbours

6fac8a8

Merge remote-tracking branch 'upstream/main'

6c95673

New HCal readouts in ALLEGRO_o1_v04

af6c969

andresailer reviewed Nov 13, 2024

View reviewed changes

Archil-AD and others added 6 commits November 14, 2024 10:31

Update detectorSegmentations/src/FCCSWHCalPhiRow_k4geo.cpp

4cfd769

fixing typo Co-authored-by: Andre Sailer <andre.philippe.sailer@cern.ch>

Update description of grid_size_row parameter

c12027a

Merge remote-tracking branch 'upstream/main'

aaf1531

Added units in HCal xml files

73a3fb6

Fix in phi position calculation and added phi neigbours for HCal PhiR…

d91be86

…ow segmentation

Added an option to add diagonal neighbour cells for HCal PhiTheta seg…

bda1517

…mentation

adding a test for ALLEGRO v04 in test/CMakeLists.txt

69c28eb

creating HCal symlinks in ALLEGRO_o1_v04 from ALLEGRO_o1_v03

20e98c7

BrieucF requested a review from giovannimarchiori November 28, 2024 11:03

giovannimarchiori reviewed Nov 28, 2024

View reviewed changes

HCal Endcap segmentation class is generalized for N-parts endcap

8cc31d8

Add the method thetaMax() for HCal phi-row segmentation needed for SW…

e752b14

… clustering

giovannimarchiori reviewed Dec 2, 2024

View reviewed changes

Adding some description in the HCal xml files

6527391

giovannimarchiori reviewed Dec 2, 2024

View reviewed changes

Added some sanity checks of the HCal xml

77b5bf3

giovannimarchiori reviewed Dec 2, 2024

View reviewed changes

Fixing typo: PhiRow to PhiTheta

4031252

giovannimarchiori approved these changes Dec 2, 2024

View reviewed changes

BrieucF mentioned this pull request Dec 3, 2024

Changes for new HCal segmentations HEP-FCC/k4RecCalorimeter#128

Merged

BrieucF merged commit 05a200b into key4hep:main Dec 3, 2024
5 of 6 checks passed

		if(m_radii.empty()) return minMaxLayerId;


		std::vector<int> minLayerIdEndcap = {0, 0, 0};


		auto pos = positionFromRThetaPhi(radius, theta(cID), phi(cID));

		// retrun the position with corrected z corrdinate to match to the geometric center


		if(m_detLayout==0) dd4hep::printout(dd4hep::INFO, "FCCSWHCalPhiRow_k4geo","Barrel configuration found!");
		else dd4hep::printout(dd4hep::INFO, "FCCSWHCalPhiRow_k4geo","EndCap configuration found!");

HCal readout segmentation update #409

HCal readout segmentation update #409

Conversation

Archil-AD commented Nov 9, 2024 • edited by andresailer Loading

atolosadelgado commented Nov 26, 2024 • edited Loading

Archil-AD commented Nov 26, 2024

BrieucF commented Nov 27, 2024

Archil-AD commented Nov 28, 2024

giovannimarchiori left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Archil-AD commented Nov 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giovannimarchiori commented Dec 2, 2024

Archil-AD commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giovannimarchiori commented Dec 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giovannimarchiori commented Dec 2, 2024

giovannimarchiori left a comment

Choose a reason for hiding this comment

Archil-AD commented Nov 9, 2024 •

edited by andresailer

Loading

atolosadelgado commented Nov 26, 2024 •

edited

Loading