feat!: (fix + chore) streamline nSegments usage

[asalzburger]

The generation of display vertices for segments was buggy and very unstable, I have put this onto more solid grounds, which helps displaying (not only) AnnulusBounds correctly.

ITk Petal Before:

ITk Petal After:

When checking the code, I see that there was a pretty big inconsistency in how the nSegments have been handled, so I re-worked that:

• nSegments (full 2 * PI segments) -> changed and renamed to quaterSegments

This guarantees that the number of segments for a full circle is a multiplier of 4 and hence the phi extrema points at (-pi,-0.5*pi,0,0.5*pi,pi) are consistently added. This will lead to a correct x and y estimation for the Extent of the Polyhedron.

This also guarantees that e.g. overlapping/attaching surfaces with the same number of segments will have vertices at the same positions, which will be better for the displaying.

• inconsitent use of unsigned int (SurfaceBounds) and std::size_t -> changed to `unsigned int everywhere

• Introduces tests for the vertex generation (which were missing)

[github-actions]

📊: Physics performance monitoring for d3ba2c1

Full contents

physmon summary

• ✅ Particles fatras
• ✅ Particles geant4
• ✅ Particles ttbar
• ✅ Vertices ttbar
• ✅ Truth tracking (KF)
• ✅ Truth tracking (GSF)
• ✅ Truth tracking (GX2F)
• ✅ CKF | trackfinding | single muon | truth smeared seeding
• ✅ Track Summary CKF | trackfinding | single muon | truth smeared seeding
• ✅ Seeding trackfinding | single muon | truth estimated seeding
• ✅ CKF | trackfinding | single muon | truth estimated seeding
• ✅ Track Summary CKF | trackfinding | single muon | truth estimated seeding
• ✅ Seeding trackfinding | single muon | default seeding
• ✅ CKF | trackfinding | single muon | default seeding
• ✅ Track Summary CKF | trackfinding | single muon | default seeding
• ✅ Seeding trackfinding | single muon | orthogonal seeding
• ✅ CKF | trackfinding | single muon | orthogonal seeding
• ✅ Track Summary CKF | trackfinding | single muon | orthogonal seeding
• ✅ Seeding trackfinding | 4 muon x 50 vertices | default seeding
• ✅ CKF | trackfinding | 4 muon x 50 vertices | default seeding
• ✅ Track Summary CKF | trackfinding | 4 muon x 50 vertices | default seeding
• ✅ Ambisolver | trackfinding | 4 muon x 50 vertices | default seeding
• ✅ IVF notime | trackfinding | 4 muon x 50 vertices | default seeding
• ✅ AMVF gauss notime | trackfinding | 4 muon x 50 vertices | default seeding
• ✅ AMVF grid time | trackfinding | 4 muon x 50 vertices | default seeding
• ✅ Seeding trackfinding | ttbar with 200 pileup | default seeding
• ✅ CKF | trackfinding | ttbar with 200 pileup | default seeding
• ✅ Track Summary CKF | trackfinding | ttbar with 200 pileup | default seeding
• ✅ Ambisolver | trackfinding | ttbar with 200 pileup | default seeding
• ✅ AMVF gauss notime | trackfinding | ttbar with 200 pileup | default seeding
• ✅ AMVF grid time | trackfinding | ttbar with 200 pileup | default seeding

[paulgessinger]

Can you avoid the physmon output changes? This should not affect the reconstruction at all should it?

[paulgessinger]

I was wondering generally: wouldn't we be able to get rid of manual passing of segments if we just define a maximum arc length $r\times\phi$ in $mm$ that a single segment is allowed to be? That way, the target bounds could decide on their own how many segments to produce, because they can just calculate the arc length?

[asalzburger]

Can you avoid the physmon output changes? This should not affect the reconstruction at all should it?

I suppose that we get a slightly different bin association for one or another surface now ... hence the fluctuation in the output.

[paulgessinger]

@asalzburger Is this because we use it for the auto-binning in the Gen1 layer building?

[asalzburger]

I was wondering generally: wouldn't we be able to get rid of manual passing of segments if we just define a maximum arc length r×ϕ in mm that a single segment is allowed to be? That way, the target bounds could decide on their own how many segments to produce, because they can just calculate the arc length?

That's a possibility, however:
The polyhedron is used mainly in two different context situations:

• (a) displaying
• (b) space/extent estimation

For latter (in many cases) a choice of quarterSegments =1 is sufficient, as it will generate now the phi values that are need to estimate the maximum extent in x/y. That's the reason why I have changed to quarterSegments rather than full 2*pi.

For displaying, however, this newly implemented schema - where the number of segments is given per quarter (and drives the vertex generation) will guarantee that attaching surfaces (even with different phi sector openings) will have vertices at the same points in space and hence will "look attaching".

That's why I changed to this schema.

[asalzburger]

@asalzburger Is this because we use it for the auto-binning in the Gen1 layer building?

That is my assumption, I will try to dig down a bit on this - I was surprised as well ....

[paulgessinger]

@asalzburger Do we need the extent building from cartesian vertices if we switch to local sizing in Gen3 and potentially reconsider the global root volume finders? Maybe something we can discuss going forward, since relying on the approximation seems a bit fragile to me.

[andiwand]

there is a small build issue @paulgessinger

[232/1487] Building CXX object Core/CMakeFiles/ActsCore.dir/src/Surfaces/PlaneSurface.cpp.o
FAILED: Core/CMakeFiles/ActsCore.dir/src/Surfaces/PlaneSurface.cpp.o 
ccache /usr/bin/c++ -DACTS_ENABLE_LOG_FAILURE_THRESHOLD -DActsCore_EXPORTS -I/__w/acts/acts/Core/include -I/__w/acts/acts/build/Core -isystem /__w/acts/acts/cmake/assert_include -isystem /usr/include/eigen3 -Wall -Wextra -Wpedantic -Wshadow -Wzero-as-null-pointer-constant -Wold-style-cast -Werror -O3 -DNDEBUG -std=c++20 -fPIC -MD -MT Core/CMakeFiles/ActsCore.dir/src/Surfaces/PlaneSurface.cpp.o -MF Core/CMakeFiles/ActsCore.dir/src/Surfaces/PlaneSurface.cpp.o.d -o Core/CMakeFiles/ActsCore.dir/src/Surfaces/PlaneSurface.cpp.o -c /__w/acts/acts/Core/src/Surfaces/PlaneSurface.cpp
/__w/acts/acts/Core/src/Surfaces/PlaneSurface.cpp: In member function 'virtual Acts::Polyhedron Acts::PlaneSurface::polyhedronRepresentation(const Acts::GeometryContext&, unsigned int) const':
/__w/acts/acts/Core/src/Surfaces/PlaneSurface.cpp:119:13: error: declaration of 'faces' shadows a previous local [-Werror=shadow]
  119 |       auto [faces, triangularMesh] =
      |             ^~~~~
/__w/acts/acts/Core/src/Surfaces/PlaneSurface.cpp:94:37: note: shadowed declaration is here
   94 |   std::vector<Polyhedron::FaceType> faces;
      |                                     ^~~~~
/__w/acts/acts/Core/src/Surfaces/PlaneSurface.cpp:119:20: error: declaration of 'triangularMesh' shadows a previous local [-Werror=shadow]
  119 |       auto [faces, triangularMesh] =
      |                    ^~~~~~~~~~~~~~
/__w/acts/acts/Core/src/Surfaces/PlaneSurface.cpp:95:37: note: shadowed declaration is here
   95 |   std::vector<Polyhedron::FaceType> triangularMesh;
      |                                     ^~~~~~~~~~~~~~
/__w/acts/acts/Core/src/Surfaces/PlaneSurface.cpp:126:13: error: declaration of 'faces' shadows a previous local [-Werror=shadow]
  126 |       auto [faces, triangularMesh] =
      |             ^~~~~
/__w/acts/acts/Core/src/Surfaces/PlaneSurface.cpp:94:37: note: shadowed declaration is here
   94 |   std::vector<Polyhedron::FaceType> faces;
      |                                     ^~~~~
/__w/acts/acts/Core/src/Surfaces/PlaneSurface.cpp:126:20: error: declaration of 'triangularMesh' shadows a previous local [-Werror=shadow]
  126 |       auto [faces, triangularMesh] =
      |                    ^~~~~~~~~~~~~~
/__w/acts/acts/Core/src/Surfaces/PlaneSurface.cpp:95:37: note: shadowed declaration is here
   95 |   std::vector<Polyhedron::FaceType> triangularMesh;
      |                                     ^~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

[paulgessinger]

@andiwand meh, I thought that was because of the merge conflict. I'll fix it.

[paulgessinger]

@andiwand Compile fixes should be in now.

[andiwand]

@paulgessinger can you also approve?

[asalzburger]

Good to go from my side.

[sonarqubecloud]

Quality Gate passed

Issues
15 New issues
0 Accepted issues

Measures
0 Security Hotspots
69.0% Coverage on New Code
1.1% Duplication on New Code

See analysis details on SonarCloud

[acts-project-service]

🔴 Athena integration test results [f5a34f3]

🔴 Some tests have failed!

Please investigate the pipeline!

status	job	report
🟢	report_pull_request
🟢	run_art_test: test_data18_13TeV_1000evt
🟢	run_art_test: test_ttbarPU40_reco
🔴	run_workflow_tests_run4_mc
🟢	run_workflow_tests_run2_mc
🟢	run_workflow_tests_run2_data
🟢	run_workflow_tests_run3_mc
🟢	run_workflow_tests_run3_data
🟢	test_ActsDumpGeometryIdentifiers
🟢	test_ActsCheckObjectCountsCachedWorkflow
🟢	test_ActsCheckObjectCountsWorkflow
🟢	test_ActsEFTrackFit
🟢	test_ActsPersistifySeeds
🟢	test_ActsBenchmarkWithSpot
🟢	test_ActsAnalogueClustering
🟢	test_ActsWorkflowHeavyIons
🟢	test_ActsWorkflowFastTracking
🟢	test_ActsWorkflowCached
🟢	test_ActsWorkflow
🟢	test_ActsValidateAmbiguityResolution
🔴	test_ActsValidateResolvedTracks
🟢	test_ActsValidateTracks
🔴	test_ActsValidateActsCoreSpacePoints
🟢	test_ActsValidateActsSpacePoints
🔴	test_ActsValidateSeeds
🟢	test_ActsValidateOrthogonalSeeds
🟢	test_ActsValidateClusters
🟢	test_ActsPersistifyEDM
🟢	test_ActsGSFRefitting
🟢	test_ActsKfRefitting
🟢	test_ActsExtrapolationAlgTest
🟢	test_ActsITkTest
🟢	run_unit_tests