Skip to content

Conversation

henryg7vdj
Copy link

Briefly, what does this PR introduce?

First attempt to model curved silicon surfaces on the OB staves. Not yet working. See README for details.

What kind of change does this PR introduce?

  • Bug fix (issue #__)
  • [x ] New feature (issue #__)
  • Documentation update
  • Other: __

Please check if this PR fulfills the following:

  • Tests for the changes have been added
  • Documentation has been added / updated
  • Changes have been communicated to collaborators

Does this PR introduce breaking changes? What changes might users need to make to their code?

Does this PR change default behavior?

@github-actions github-actions bot added topic: documentation Improvements or additions to documentation topic: tracking topic: barrel Mid-rapidity detectors topic: forward Positive-rapidity detectors (hadron-going side) labels Aug 22, 2025
@@ -0,0 +1,4 @@
#!/bin/bash
ddsim --compactFile epic_craterlake_tracking_only_cut.xml --outputFile test.edm4hep.root --numberOfEvents 1000 --enableGun --gun.thetaMin 50*deg --gun.thetaMax 130*deg --gun.distribution eta --gun.particle pi- --gun.momentumMin 10*GeV --gun.momentumMax 10*GeV --gun.multiplicity 1
eicrecon -Pnthreads=1 -Pjana:debug_plugin_loading=1 -Ppodio:output_file=reconTest.edm4eic.root -Pdd4hep:xml_files=epic_craterlake_tracking_only_cut.xml -Ppodio:output_collections="MCParticles,CentralCKFTrajectories,CentralCKFTrackParameters,CentralCKFSeededTrackParameters,CentralCKFTruthSeededTrackParameters,CentralTrackVertices,ReconstructedChargedParticles,ReconstructedChargedParticleAssociations" test.edm4hep.root
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is something I was just discussing with @veprbl yesterday. When using the xml_files interface, that makes sure that the specified geometry is used, but it crucially does not change the $DETECTOR_PATH variable. So, the correct top-level entry point will be used but potentially not the underlying detector-subsystem-specific files that are included by the toplevel.

(more later, MSc defense to attend now)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. This is the method I have been using to run a simulation with just a few sub-detectors - in this case the silicon and vertex barrels - I take one of the xml files in epic/install/share/epic and cut out the bits I don't want. I assume this (usually) works as the links in these files all use $DETECTOR_PATH, but it feels like a very hacky way to do it and I can see it may not always work. What is the correct way to do it? (Where do these xml files come from anyway? Are they generated automatically by cmake --install?)

Comment on lines +192 to +198
<readout name="SiBarrelHits">
<segmentation type="MultiSegmentation" key="layer">
<segmentation name="L3" type="CylindricalGridPhiZ" key_value="3" grid_size_phi="0.02*mm/(SiBarrelMod1_rmin+6*mm)" grid_size_z="0.02*mm" radius="SiBarrelMod1_rmin+6*mm" />
<segmentation name="L4" type="CylindricalGridPhiZ" key_value="4" grid_size_phi="0.04*mm/(SiBarrelMod2_rmin+6*mm)" grid_size_z="0.04*mm" radius="SiBarrelMod2_rmin+6*mm" />
</segmentation>
<id>system:8,layer:4,module:12,sensor:2,phi:32:-16,z:-16</id>
</readout>
Copy link
Contributor

@wdconinc wdconinc Aug 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you had some questions on the segmentation, essentially the maximum extent in phi and z, divided by the grid size in that dimensions, gives you a maximum number of grid points. That number of grid points should fit within the number of bits specified for that dimensions.

E.g. assuming 360 degrees for phi, and grid_size_phi = 0.02*mm/(27.1*cm+6*mm) = 7.2e-5 rad, that means 4986000 grid points, which corresponds to 22.2 i.e. 23 whole bits. So the 16 bits reserved for this dimension is not sufficient.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I am still puzzling this out - please bear with me.

For this example, I have:

2pi/7.2e-5 = 87022 = 2^16.4 - so we need 17 bits for phi?
522/0.02 = 26100 = 2^14.7 - so we need 15 bits for z?

What does "phi:32:-16,z:-16" actually mean? Is it 32 bits for phi and whatever-is-left for z (then, if the total is 64 would be 64-8-4-12-2-32 = 6, so clearly not enough, unless it is 64 bits just for phi and z?), or are the 32 bits split between phi and z? Is the -16 an offset to allow for negative numbers? I'm guessing it is 32-16=16 bits for phi and then 64-8-4-12-2-16 = 22 for z?

More questions: are these local or global coordinates? Each stave is only 5-8 degrees wide, so do we need to allow for a maximum of 360 degrees? In the CartesianGridXY setup, I assumed the x and y were local coordinates in the plane of each stave. If the CylindricalGridPhiZ is a global coordinate system, that would explain why my model where each stave is a separate cylinder doesn't work. Presumably I need to define the offset axis for each one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "phi:32:-16,z:-16" actually mean?

This means that the field phi starts at bit 32, has a width of 16 bits and the value is stored as a signed integer. Then the field z follows, also with a width of 16 bit, and also as a signed integer. Signed makes sense here. For r/phi segmentation it would not make sense. But there really is no difference since the conversion from bits to position works regardless and should give the same position.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: barrel Mid-rapidity detectors topic: documentation Improvements or additions to documentation topic: forward Positive-rapidity detectors (hadron-going side) topic: tracking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants