acts-project / acts

Experiment-independent toolkit for (charged) particle track reconstruction in (high energy) physics experiments implemented in modern C++
https://acts.readthedocs.io
Mozilla Public License 2.0
102 stars 165 forks source link

Cut tube segment dd4hep conversion tests #1146

Open rahmans1 opened 2 years ago

rahmans1 commented 2 years ago

Proposed labels: Improvement, Needs Discussion

Proposed next steps on cut tube support that touch the Acts code base:

1) Extend TGeoTubeConversionTests.cpp with a TGeoCtub (like done here for TGeoTubeSeg), this test will fail until support below is added,

a) Extend Acts::TGeoSurfaceConverter::cylinderComponents (like done [here](https://github.com/acts-project/acts/blob/main/Plugins/TGeo/src/TGeoSurfaceConverter.cpp#L88) 
    for TGeoTubeSeg) to support TGeoCtub (TGeoCtub inherits TGeoTubeSeg inherits TGeoTube, so 
    the same strategy taken there simply extends), and similarly extends 
   Acts::TGeoSurfaceConverter::discComponents  (like [here](https://github.com/acts-project/acts/blob/main/Plugins/TGeo/src/TGeoSurfaceConverter.cpp#L240))

The test above should now succeed.

2) extend Acts::DD4hepLayerBuilder::endcapLayers (like done here for TGeoTubeSeg) and Acts::DD4hepLayerBuilder::centralLayers (like done here for TGeoTubeSeg) to support TGeoCtub.

3) Are there any DD4hep unit tests that should be updated to test?

rahmans1 commented 2 years ago

Suggestions provided by @wdconinc. Just wanted to make sure that nobody else is actively working or planning to work on it. If not, I will be doing it in this branch.

paulgessinger commented 2 years ago

ping @asalzburger

rahmans1 commented 2 years ago

Incidental questions brought up in the ATHENA-ACTS meeting.

  1. Relevant Issue: Extending Acts::TGeoSurfaceConverter::cylinderComponents and Acts::TGeoSurfaceConverter::discComponents to support TGeoCtub Question: Why is the condition "halfZ>deltaR" used before checking if the shape is TGeoTubeSeg when translating to cylinder components but not disc components? Description: TGeoSurfaceConverter::cylinderComponents sequentially checks if the TGeoShape to convert is TGeoTube or TGeoTubeSeg. TGeoTubeSeg is derived from TGeoTube. So, a TGeoTubeSeg can be cast into a TGeoTube which contains information regarding rmin, rmax and z. But before casting and checking if it's TGeoTubeSeg with specific phi range, there is an additional constraint used in cylinderComponents. But the same constraint is not used in discComponents.
  2. Relevant Issue: Extending Acts::DD4hepLayerBuilder::endcapLayers and Acts::DD4hepLayerBuilder::centralLayers. Question: Why is direct casting to TGeoTubeSeg employed? Description: If a TGeoTube is force casted to a TGeoTubeSeg, that returns a null pointer. TGeoCtub is derived from TGeoTubeSegment which is derived from TGeoTube.
  3. We also didn't see any unit tests for the DD4hep case similar to TGeo case? Do these need to be added? Not a priority for us but would be good to know if someone was already working on this.
rahmans1 commented 2 years ago
  1. Relevant Issue: Extending Acts::TGeoSurfaceConverter::cylinderComponents and Acts::TGeoSurfaceConverter::discComponents to support TGeoCtub Question: Are tubes with bevel cuts not allowed for discSurfaces? Seems like there is no natural way to extend the unit test for disc/endcaps similar to cylinder/barrels
paulgessinger commented 2 years ago

@noraemi do you want to chime in?

noraemi commented 2 years ago

I didn't extend the RadialBounds to include the bevel as the bevel would be a property of the cylinder and otherwise its just a tilted disc? But if necessary I could add it

rahmans1 commented 2 years ago

Intuitively speaking, it doesn't make sense for radial bounds to include a bevel. But we can either force it in for consistency. Or an alternative may be to assign a warning to the user that TGeoCtub cannot be used for discs.

stale[bot] commented 2 years ago

This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs.

rahmans1 commented 2 years ago

Adding an update to this discussion https://indico.bnl.gov/event/15458/contributions/62426/attachments/40571/67801/ACTS%20integration%20for%20B0%20tracker.pdf

stale[bot] commented 2 years ago

This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs.