acts icon indicating copy to clipboard operation
acts copied to clipboard

Cut tube segment dd4hep conversion tests

Open rahmans1 opened this issue 3 years ago • 10 comments

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 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) 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 avatar Feb 02 '22 18:02 rahmans1

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.

rahmans1 avatar Feb 02 '22 18:02 rahmans1

ping @asalzburger

paulgessinger avatar Feb 03 '22 09:02 paulgessinger

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 avatar Feb 07 '22 17:02 rahmans1

  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

rahmans1 avatar Mar 09 '22 21:03 rahmans1

@noraemi do you want to chime in?

paulgessinger avatar Mar 10 '22 08:03 paulgessinger

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

noraemi avatar Mar 10 '22 09:03 noraemi

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.

rahmans1 avatar Mar 10 '22 15:03 rahmans1

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.

stale[bot] avatar Apr 11 '22 01:04 stale[bot]

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

rahmans1 avatar Apr 19 '22 15:04 rahmans1

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.

stale[bot] avatar May 25 '22 23:05 stale[bot]