fix: TGeoSurfaceConverter toRadian conversion

[plariono]

This PR fixes the incorrect conversion to radians for angles > 360. in TGeoSurfaceConverter.

[plariono]

The ROOT TGeo phi convention is the following:

Here phi1 and phi2are the starting and ending phivalues in degrees. The general phi convention is that the shape ranges from phi1 to phi2 going counterclockwise. The angles can be defined with either negative or positive values. They are stored such that phi1 is converted to [0,360] and phi2 > phi1.

In my case it was:

tubeSeg->GetPhi1() = 316.79, tubeSeg->GetPhi2() = 403.21

The radian_sym method from https://github.com/acts-project/acts/blob/main/Core/include/Acts/Utilities/detail/periodic.hpp should do the job to make this [-180, 180).

And the Unit test @asalzburger, what exactly should it check?

[andiwand]

some unit tests seem to fail - any ideas @plariono @asalzburger ?

[plariono]

some unit tests seem to fail - any ideas @plariono @asalzburger ?

@andiwand Might be because I forgot to run clang tidy.

[plariono]

Guys @asalzburger @andiwand this looks fine on the TGeo side but not on the DD4HEP one. I have troubles debugging it bc I didn't manage to build the dd4hep plugins and unit tests on my system. Could you help with the issue please?

unknown location(0): fatal error: in "DD4hepPlugin/DD4hepPluginCylinderLayerStructure": std::invalid_argument: CylinderBounds: invalid phi sector setup.
/__w/acts/acts/Tests/UnitTests/Plugins/DD4hep/DD4hepCylinderLayerStructureTests.cpp(140): last checkpoint: "DD4hepPluginCylinderLayerStructure" test entry
Evaluator        WARN  +++ Overwriting variable: name=world_size value=10.*m  Evaluator::Object : existing variable [setVariable error]
Detector         WARN  +++ Object 'world_size' is already defined. New value will be ignored
Evaluator        WARN  +++ Overwriting variable: name=world_x value=world_size  Evaluator::Object : existing variable [setVariable error]
Detector         WARN  +++ Object 'world_x' is already defined. New value will be ignored
Evaluator        WARN  +++ Overwriting variable: name=world_y value=world_size  Evaluator::Object : existing variable [setVariable error]
Detector         WARN  +++ Object 'world_y' is already defined. New value will be ignored
Evaluator        WARN  +++ Overwriting variable: name=world_z value=world_size  Evaluator::Object : existing variable [setVariable error]
Detector         WARN  +++ Object 'world_z' is already defined. New value will be ignored
unknown location(0): fatal error: in "DD4hepPlugin/DD4hepPluginCylinderLayerStructureAutoRange": std::runtime_error: dd4hep: Attempt to add an already existing object:PixelReadout.

[paulgessinger]

@plariono I think the underlying issue with DD4hep is the first line: the test sets up a cylinder surface that, with this PR, produces defining parameters that are outside the accepted valid range. I'm not exactly sure why that happens. The remaining errors are, I think, noise from DD4hep after the exception is thrown.

[paulgessinger]

I ran the test locally, and the phi value that's being handed over to CylinderBounds is 0. This is checked in the constructor and throws an exception.

I don't really understand how DD4hep even sees the changes from this PR though...

Ah, the DD4hep plugin calls into the TGeo surface converter for the actual surface conversion.

So what happens is this:

toRadian: in=360 orig=6.28319 new=0

The original function returns $2\pi$ the new version returns 0, which is invalid for cylinder bounds.

I imagine what we want here is to return a range between $[-\pi,\pi)$ (that's also what the comment says). What do you think @plariono?

[plariono]

Thank you for the check @paulgessinger. Yes, we need to return the $[-\pi, \pi)$ range. However, the conversion won't work for full cylinders then.

So I added a new method to calculate the halfPhi and avgPhi using the degree input which returns the average $\phi$ within the $[-\pi, \pi)$ range. And the unit test. Could you test it again?

[paulgessinger]

@plariono I don't see a change in the DD4hep unit test unfortunately. I think the issue remains that 360 deg is mapped to 0 deg, which CylinderBounds doesn't like.

[plariono]

@plariono I don't see a change in the DD4hep unit test unfortunately. I think the issue remains that 360 deg is mapped to 0 deg, which CylinderBounds doesn't like.

@paulgessinger I see, thanks. Maybe it should be defined according to the $[-\pi, \pi)$ range in the DD4hep file then?

[asalzburger]

Will be superseded.