-
Notifications
You must be signed in to change notification settings - Fork 61
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
[IDEA] Adding the detailed version of the muon system. #322
Conversation
…the detailed version(barrel and endcap in the same builder)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Mahmoud, thanks a lot for this! Here are already a few comments. Can you also try to run the overlap check?
create a geant4 macro named overlap.mac with this inside:
/geometry/test/run
exit
Then run the overlap checker with this:
ddsim --compactFile ./compact/simple_detector.xml --runType run --part.userParticleHandler='' --macroFile overlap.mac >> overlapDump.txt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to remove references to IDEA in this README, the detector builder is quite generic and could be used for other implementation. e.g. IDEA Muon System
--> µRWELL Based Muon System
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Brieuc, I have edited the code after checking all the overlaps, and now it's clean.. I'm attaching the overlapDump file.
overlapDump-151.txt
<detectors> | ||
|
||
<!-- mRWELL envelope --> | ||
<detector name="muonSystem1" type="muonSystem_o1_v00" vis="MuonVisEnv" id="51" readout="mRWELLChamberReadout"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and in other places: the "id" should be defined in DectDimensions_IDEA_o1_v01.xml
and called here (see and modify
<constant name="DetID_Yoke_Barrel" value=" 13"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also suspect this should crash as it is now (did you try running it with ddsim?) because we have 4 bits for system
in the readout which only allows to have "id's" from 0 to 15.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the 9 muon system layers IDs to the DectDimensions_IDEA_o1_v01.xml, and ddsim run without crashes even if the system has 4 bits. Isn't the 4 bits in that case only for the muon system, since for example I have here 9 detectors (layers)? since also every sub-detector has its own readout description?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Mahmoud, this is fantastic! Tahnks! Here is a first batch of comment.
Can you also update FCCee/IDEA/compact/README.md
under IDEA_o1_v02 with something like:
"Based on o1_v01 but with a detailed description of the vertex detector, drift chamber and muon system, a place holder solenoid and the endplate absorber. Missing: SiWrapper, calorimeter."?
I am also a bit puzzled by the fact of having one detID per layer but I guess having one detID for the whole barrel and one for the whole endcap would imply a complete refactoring of the code?
…or homogeneity with the other detectors
|
||
for (int rectangle = 0; rectangle < (numRectangles + 1); ++rectangle) { | ||
|
||
double rectangleEnvX = detectorVolumeThickness/4.5; // dividing by 4.5 gets the best thickness for the recatngle to avoid any overlap ~ in our case the uRWELL the best rectangle thickness is 26.667 mm which is 120/4.5. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This /4.5
seems awfully fine-tuned? Or is this OK also for other thinknesses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @andresailer, I have changed the parameters, and now it is OK with other thicknesses to. Whatever the chamber dimensions the user will put in the compact file, It will work. I have made the variable detectorVolumeThickness
is determined by the builder depending on the chamber dimensions and the overlap angle, instead before it was determined by the user.
// // B A R R E L // | ||
// ---------------------------------------------------------------------------------------------------- | ||
|
||
if (numBarrelDetectorLayers > 0){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it makes sense to split this into a barrel and an endcap constructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the barrel last layer is enclosing all the endcap layers inside, we preferred to keep them in the same builder. Both Barrel and Endcap are sharing some common variables like numSides
of the main polygon shape. and the length of the last layer of the barrel depending on the total thickness of the endcap detector. And to distinguish between the collection hits of barrel and endcap, a bit-field component has been defined. Maybe we might tend to separate them in the future if needed.
|
||
int numBarrelLayer = 1; | ||
double barrelLayerRMin = radius - detectorVolumeThickness/2.0; // have to be changed depends on xml impementation | ||
double barrelLayerRMax = radius + 1.5 * detectorVolumeThickness; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this 1.5 factor always correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since the detector volume was centered in the 0.25 of the thickness of that volume. But I have also changed it in the new commits, and I centered it in the middle instead.
double angle_clearance = 0.0; // 0.02 works good but needs the detector volume thick to be more than 60 mm. // it's less than 1 degree, we use the clearnce to avoid overlapping | ||
|
||
double sideLength = 2 * radius * std::tan(shapeAngle_radians); | ||
double sideLength2 = 2 * (radius + 1.5 * detectorVolumeThickness) * std::tan(shapeAngle_radians); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does the 2 signify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's coming from the formula to calculate the polygon side length Side Length a. a = 2r tan(π/n)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you mean the number 2 in sideLength2
and other variable as well, that represents here the trapezoid's parallel sides lengths. just to distinguish between the two sides lengths.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, yes I mean the 2 in sideLength2
, can you please add what you wrote as a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, I Have added the comments.
Thanks @BrieucF and @andresailer for your comments,
|
<readouts> | ||
<readout name="MuonSystemCollection"> | ||
<segmentation type="CartesianGridYZ" grid_size_y="1.2*mm" grid_size_z="1.2*mm"/> <!-- Depending on strip pitch 1.4 mm --> | ||
<id>system:5,type:2,layer:4,chamber:15,slice:1,y:-10,z:-10</id> <!-- The bit field is divided into 2^5 systems(IDEA sub-detectors), 2^2 types(Barrel Muon System, positive endcap, and negative endcap"), 2^4 layers(number of layers in barrel for example) ,2^15 chambers(the number of muRWELL chambers in every layer), 2^1 slice(number of sensitive layers inside every chambers), and 2^10 y&z strips in every sensitive layer--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<id>system:5,type:2,layer:4,chamber:15,slice:1,y:-10,z:-10</id> <!-- The bit field is divided into 2^5 systems(IDEA sub-detectors), 2^2 types(Barrel Muon System, positive endcap, and negative endcap"), 2^4 layers(number of layers in barrel for example) ,2^15 chambers(the number of muRWELL chambers in every layer), 2^1 slice(number of sensitive layers inside every chambers), and 2^10 y&z strips in every sensitive layer--> | |
<id>system:5,type:2,layer:4,chamber:15,slice:1,y:-10,z:-10</id> <!-- The bit field is divided into 2^5 systems(IDEA sub-detectors), 2^2 types(0 = Barrel Muon System, +1 = positive endcap and -1 = negative endcap"), 2^4 layers(number of layers in barrel for example) ,2^15 chambers(the number of muRWELL chambers in every layer), 2^1 slice(number of sensitive layers inside every chambers), and 2^10 y&z strips in every sensitive layer--> | |
```? | |
I'd also propose to change `type` to `side`, but it is not a strong request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid using side
instead of type
would cause confusion with the detector side variable that I use to determine the polygon shape sides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! But can you still say in the xml which value is given to barrel/endcap etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I just added it in the last commit
<composite n="0.40" ref="CF4"/> | ||
</material> | ||
|
||
<material name="G4_Fe" state="solid"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these materials still needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's the material of the uRWELL.
dd4hep::Transform3D unionTransform2(unionRot, unionPos2); | ||
|
||
//Combining two shapes by UnionSolid: the first shape is centralized and the second transform around the first.. | ||
dd4hep::UnionSolid barrelAndPositiveEndcap(BarrelEnv, EndcapEnv, unionTransform); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a cylinder would be more performant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cylinder will cause overlaps with different layers, and sub-detectors before the muon-system (in our case is DR-Calo).
dd4hep::Rotation3D barrelUnionRot(dd4hep::RotationY(0.0 * dd4hep::degree)); | ||
dd4hep::Transform3D barrelUnionTransform(barrelUnionRot, barrelUnionPos); | ||
|
||
dd4hep::UnionSolid barrelUnion(BarrelEnvWithoutLastLayer, BarrelLastLayerEnv, barrelUnionTransform); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a cylinder would be more performant?
dd4hep::Transform3D boxTransform(boxRot, boxPos); | ||
|
||
//Combining two shapes by UnionSolid: the first shape is centralized and the second transform around the first.. | ||
dd4hep::UnionSolid endcapDetectorSideEnvelope(endcapDetectorSideTrap, endcapDetectorSideBox, boxTransform); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe a cylinder would be more performant?
dd4hep::Volume sideVol2(sideName, sideEnvelope2, mat); | ||
|
||
double angle_degrees = shapeAngle * side; // Calculate the angle for each side | ||
double angle_radians = angle_degrees * M_PI / 180.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
to avoid confusion I would suggest to work on angle natural units of dd4hep. In addition, conversion of deg <-> rad can be done using ROOT factosTMath::DegToRad()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it is good, here are minimal comments, @BrieucF can tell if they can be ignored :)
-
typo: CarnonFiber :)
-
Is there a real need of defining the following materials? G4_Fe, G4_Si0x56335d3d1480, Ca400x5633439c2580, ... G10_FR40x5633439ca570. These materials are introduced in this PR, lines 490-645 of the material XML file
https://github.com/key4hep/k4geo/blob/931d8d1e96ac5f05e8239db4c2319207f11741f4/FCCee/IDEA/compact/IDEA_o1_v02/materials_o1_v01.xml -
would it be possible to the replace the union solids that make up the envelopes by cylinders for example?
I'd say the baseline is to implement them all :-P if some are causing problems or require significant change we de-discuss ;-) |
Co-authored-by: Brieuc Francois <brieuc.francois@cern.ch>
Co-authored-by: Brieuc Francois <brieuc.francois@cern.ch>
Hi @atolosadelgado , Thanks very much for your comments, here are some clarifications;
|
I had a chat with Brieuc, and these are minor comments which can be ignored |
Thanks a lot Mahmoud, and congratulations!
|
BEGINRELEASENOTES
ENDRELEASENOTES