Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HCalThreePartsEndcap questions regarding z-reflection symmetry #455

Open
scott-snyder opened this issue Mar 27, 2025 · 3 comments
Open

HCalThreePartsEndcap questions regarding z-reflection symmetry #455

scott-snyder opened this issue Mar 27, 2025 · 3 comments

Comments

@scott-snyder
Copy link
Contributor

Hello -

I some questions about details of the endcal hcal geometry in
HCalThreePartsEndcap_o1_v02_geo.cpp.

In particular, i was comparing cell positions in the positive vs negative
endcaps and finding that they were not symmetric.

I think there are three separate issues.

The first is for the sequence positions in part 1.
We can explore this interactively using python.

>>> import os
>>> import DDG4
...
>>> kernel = DDG4.Kernel()
...
>>> kernel.loadGeometry("file:" + os.environ['K4GEO'] + "/FCCee/ALLEGRO/compact/ALLEGRO_o1_v03/ALLEGRO_o1_v03.xml")
...
>>> detectorDescription = kernel.detectorDescription()
>>> hce=detectorDescription.detector('HCalThreePartsEndcap')
>>> >>> hce.child('layer1').placement().position().Z()
315.0
>>> hce.child('layer-1').placement().position().Z()
-315.0

Now look at the sequence positions in the two halves.
There are 27 sequences, numbered 0..26.

>>> hce.child('layer1').child('seq0').placement().position().Z()
-23.950000000000003
>>> hce.child('layer1').child('seq26').placement().position().Z()
22.85000000000001
>>> hce.child('layer-1').child('seq0').placement().position().Z()
-23.950000000000003
>>> hce.child('layer-1').child('seq26').placement().position().Z()
22.85000000000001

There's no reflection applied in these coordinate transforms,
just translation. That means that the innermost sequence is seq0
in the positive endcap, but seq26 in the negative endcap.
And they are positioned differently with respect to the origin.
Each sequence has a total width of 1.8cm, so the innermost sequence
in the positive endcap has z range 290.15..291.95 while in the negative
endcap it has z range -293.05..-291.25.

Further, the inner endplate is positioned as

>>> hce.child('endPlate_2').placement().position().Z()
290.4
>>> hce.child('endPlate_-2').placement().position().Z()
-290.4

and has a thickness of 5mm. So the endplate occupies z ranges
290.15..290.65 and -290.65..-290.15 in the + and - endcaps
respectively. However, in the positive endcap, this overlaps
with the innermost sequence!

I think the problem is that the endplate isn't properly taken into account
when the sequences are positioned. If i make this change:

diff --git a/detector/calorimeter/HCalThreePartsEndcap_o1_v02_geo.cpp b/detector/calorimeter/HCalThreePartsEndcap_o1_v02_geo.cpp
index 30235a9..9545afe 100644
--- a/detector/calorimeter/HCalThreePartsEndcap_o1_v02_geo.cpp
+++ b/detector/calorimeter/HCalThreePartsEndcap_o1_v02_geo.cpp
@@ -266,7 +267,7 @@ static dd4hep::Ref_t createHCalEC(dd4hep::Detector& lcdd, xml_h xmlElement, dd4h
 
             // second z loop (place sequences in layer)
             std::vector<dd4hep::PlacedVolume> seqs; 
-            double zOffset = - dzDetector1 + 0.5 * dzSequence; //2*dZEndPlate + space + 0.5 * dzSequence;
+            double zOffset = - dzDetector1 + 0.5 * dzSequence + 2*dZEndPlate + space;
          
             for (uint numSeq=0; numSeq < numSequencesZ1; numSeq++){
                 dd4hep::Position tileSequencePosition(0, 0, zOffset);

then the sequences are symmetric:

>>> hce.child('layer1').child('seq0').placement().position().Z()
-23.400000000000002
>>> hce.child('layer1').child('seq26').placement().position().Z()
23.40000000000001
>>> hce.child('layer-1').child('seq0').placement().position().Z()
-23.400000000000002
>>> hce.child('layer-1').child('seq26').placement().position().Z()
23.40000000000001

and so the innermost sequence has z range 290.7..292.5 in the positive endcap
and -292.5..290.7 in the negative endcap. And in both cases, the distance
between the inner edge of this sequence and the outer edge of the endplate
is 0.05, matching the space parameter.

Part 2 is ok as far as this goes, but the second issue is that part 3 has
essentially the same issue. Part 3 is layers 16-27, and there are
are 85 sequences, numbered 0..84:

>>> hce.child('layer16').placement().position().Z()
467.5
>>> hce.child('layer-16').placement().position().Z()
-467.5
>>> hce.child('layer16').child('seq0').placement().position().Z()
-76.14999999999999
>>> hce.child('layer16').child('seq84').placement().position().Z()
75.0499999999999
>>> hce.child('layer-16').child('seq0').placement().position().Z()
-76.14999999999999
>>> hce.child('layer-16').child('seq84').placement().position().Z()
75.0499999999999

In this case, the outermost sequence has z range 541.65..543.45 in the
positive endcap and -544.55..-542.75 in the negative endcap. The outer
endplates are located at:

>>> hce.child('endPlate_1').placement().position().Z()
544.3
>>> hce.child('endPlate_-1').placement().position().Z()
-544.3

so they have z ranges of 544.05..544.55 and -544.55..-544.05 in the two
endcaps. So here we have an overlap on the negative side.

The fix here, i think, is slightly different, because the endplate is on the
outside rather than the inside:

diff --git a/detector/calorimeter/HCalThreePartsEndcap_o1_v02_geo.cpp b/detector/calorimeter/HCalThreePartsEndcap_o1_v02_geo.cpp
index 30235a9..2a8207c 100644
--- a/detector/calorimeter/HCalThreePartsEndcap_o1_v02_geo.cpp
+++ b/detector/calorimeter/HCalThreePartsEndcap_o1_v02_geo.cpp
@@ -131,7 +131,7 @@ static dd4hep::Ref_t createHCalEC(dd4hep::Detector& lcdd, xml_h xmlElement, dd4h
     // Calculate correction along z based on the module size (can only have natural number of modules)
     double dzDetector1 = (numSequencesZ1 * dzSequence) / 2 + 2 * dZEndPlate + space;
     double dzDetector2 = (numSequencesZ2 * dzSequence) / 2;
-    double dzDetector3 = (numSequencesZ3 * dzSequence) / 2 + 2 * dZEndPlate + space;
+    double dzDetector3 = (numSequencesZ3 * dzSequence) / 2;
 
     dd4hep::printout(dd4hep::DEBUG, "HCalThreePartsEndcap_o1_v02", "correction of dz (negative = size reduced) first part EC: %.2f", dzDetector1*2 - dimensions.width()*2); 
     dd4hep::printout(dd4hep::DEBUG, "HCalThreePartsEndcap_o1_v02", "dz second part EC: %.2f", dzDetector2 * 2); 
@@ -191,7 +191,7 @@ static dd4hep::Ref_t createHCalEC(dd4hep::Detector& lcdd, xml_h xmlElement, dd4h
 
         // Endplates placed for the extended Barrels in front and in the back to the central Barrel
         DetElement endPlatePos(caloDetElem, "endPlate_" + std::to_string(1 * sign), 0);
-        dd4hep::Position posOffset(0, 0, sign * (extBarrelOffset3 + dzDetector3 - dZEndPlate));
+        dd4hep::Position posOffset(0, 0, sign * (extBarrelOffset3 + dzDetector3 + dZEndPlate + space));
         PlacedVolume placedEndPlatePos = envelopeVolume.placeVolume(endPlateVol3, posOffset);
         endPlatePos.setPlacement(placedEndPlatePos);
 

With this change, we have

>>> hce.child('layer16').child('seq0').placement().position().Z()
-75.6
>>> hce.child('layer16').child('seq84').placement().position().Z()
75.5999999999999
>>> hce.child('layer-16').child('seq0').placement().position().Z()
-75.6
>>> hce.child('layer-16').child('seq84').placement().position().Z()
75.5999999999999
>>> hce.child('endPlate_1').placement().position().Z()
544.3
>>> hce.child('endPlate_-1').placement().position().Z()
-544.3

which puts the outermost sequences at z-ranges of
542.2..544.0 and -544.0..-542.2 and the endplates at z-ranges of
544.05..544.55 and -544.55..-544.05.

Finally, there is the offset of the tiles within the sequences.
These have the same sign on both halves of the endcap (again, there's
no reflection in the transformation, just translation):

>>> hce.child('layer1').child('seq0').child('tile0').placement().position().Z()
-0.2
>>> hce.child('layer2').child('seq0').child('tile0').placement().position().Z()
0.7000000000000001
>>> hce.child('layer-1').child('seq0').child('tile0').placement().position().Z()
-0.2
>>> hce.child('layer-2').child('seq0').child('tile0').placement().position().Z()
0.7000000000000001

So for layer1 (part1), the tile is closer to the endplate in the positive
endcap than it is in the negative endcap.
This would imply that the modules for the positive and negative halves
are assembled differently, which i doubt is intended.
If we flip the signs in the negative endcap, then they become symmetric:

diff --git a/detector/calorimeter/HCalThreePartsEndcap_o1_v02_geo.cpp b/detector/calorimeter/HCalThreePartsEndcap_o1_v02_geo.cpp
index 30235a9..2e8c462 100644
--- a/detector/calorimeter/HCalThreePartsEndcap_o1_v02_geo.cpp
+++ b/detector/calorimeter/HCalThreePartsEndcap_o1_v02_geo.cpp
@@ -254,7 +254,7 @@ static dd4hep::Ref_t createHCalEC(dd4hep::Detector& lcdd, xml_h xmlElement, dd4h
                 Volume tileVol("HCalECTileVol_"+ xComp.materialStr(), tileShape, lcdd.material(xComp.materialStr()));
                 tileVol.setVisAttributes(lcdd, xComp.visStr());
            
-                dd4hep::Position tileOffset(0, 0, tileZOffset + 0.5 * xComp.thickness() );
+                dd4hep::Position tileOffset(0, 0, sign * (tileZOffset + 0.5 * xComp.thickness()) );
                 dd4hep::PlacedVolume placedTileVol = tileSequenceVolume.placeVolume(tileVol, tileOffset);
            
                 if (xComp.isSensitive()){
@@ -314,7 +314,7 @@ static dd4hep::Ref_t createHCalEC(dd4hep::Detector& lcdd, xml_h xmlElement, dd4h
                 Volume tileVol("HCalECTileVol_", tileShape, lcdd.material(xComp.materialStr()));
                 tileVol.setVisAttributes(lcdd, xComp.visStr());
 
-                dd4hep::Position tileOffset(0, 0, tileZOffset + 0.5 * xComp.thickness() );
+                dd4hep::Position tileOffset(0, 0, sign * (tileZOffset + 0.5 * xComp.thickness()) );
                 dd4hep::PlacedVolume placedTileVol = tileSequenceVolume.placeVolume(tileVol, tileOffset);
 
                 if (xComp.isSensitive()){
@@ -384,7 +384,7 @@ static dd4hep::Ref_t createHCalEC(dd4hep::Detector& lcdd, xml_h xmlElement, dd4h
                 Volume tileVol("HCalECTileVol_" , tileShape, lcdd.material(xComp.materialStr()));
                 tileVol.setVisAttributes(lcdd, xComp.visStr());
 
-                dd4hep::Position tileOffset(0, 0, tileZOffset + 0.5 * xComp.thickness() );
+                dd4hep::Position tileOffset(0, 0, sign * (tileZOffset + 0.5 * xComp.thickness()) );
                 dd4hep::PlacedVolume placedTileVol = tileSequenceVolume.placeVolume(tileVol, tileOffset);
 
                 if (xComp.isSensitive()){

so then:

>>> hce.child('layer1').child('seq0').child('tile0').placement().position().Z()
-0.2
>>> hce.child('layer2').child('seq0').child('tile0').placement().position().Z()
0.7000000000000001
>>> hce.child('layer-1').child('seq0').child('tile0').placement().position().Z()
0.2
>>> hce.child('layer-2').child('seq0').child('tile0').placement().position().Z()
-0.7000000000000001

And then the layout seems to be symmetric between the positive and negative
endcaps.

There is then still an issue that the row numbering is different in the
two endcaps. It always increases in the direction of positive z, which
means that the row numbering goes from the inside out in the positive
endcap but from outside in in the negative endcap. Unlike the actual
positions, this is just a convention and doesn't really matter as long
as it is handled consistently, but this is not what i would naively expect
and hence likely to be confusing. If you agree, i can look into
changing that as well, but that should be done as a separate change.

Anyway, if you agree, i can make a PR with the changes discussed here.
I guess a question is, should this be a new geometry version, or just
change the existing one?

@BrieucF
Copy link
Contributor

BrieucF commented Mar 27, 2025

The following persons might be interested: @Archil-AD @mmlynari @giovannimarchiori @faltovaj

@giovannimarchiori
Copy link
Contributor

Hi @scott-snyder
you seem to have already the code ready for a PR, so why don't you create one and @mmlynari or @Archil-AD review it?
Thanks
Giovanni

@mmlynari
Copy link
Contributor

mmlynari commented Apr 1, 2025

Hi @scott-snyder,
Apologies for the delayed response, and thanks a lot for looking into this! This is indeed something we discussed with @Archil-AD, as we also noticed the issue but hadn’t had the chance to implement the fix properly. If you already have these changes ready, please feel free to open a PR - that would be a huge help for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants