fix: minor bugs in grid vertex seeder

[felix-russo]

Some small improvements in AdaptiveGridTrackDensity and GaussianGridTrackDensity:

• 1/(2 * M_PI) is removed from the track density since constant factors have no influence on the position of the maximum z value.
• The central bin was twice as large as the others in AdaptiveGridTrackDensity. I repositioned the grid such that the central bin corresponds to the interval [-binSize/2, binSize/2). As a consequence, I had to modify the unit tests.
• There was a sign mistake in the computation of the exponent. I removed it and check if the new sign is correct in the unit test compare_to_analytical_solution_for_single_track.
• There was a sign mistake and a typo in the computation of the seed width. I removed them and check that the new computation is correct in the unit tests check_seed_width_estimation and compare_to_analytical_solution_for_single_track.

[codecov]

Codecov Report

Merging #2376 (f8a6c10) into main (ed0bb27) will decrease coverage by 0.02%.
The diff coverage is 63.15%.

@@            Coverage Diff             @@
##             main    #2376      +/-   ##
==========================================
- Coverage   49.69%   49.68%   -0.02%     
==========================================
  Files         454      454              
  Lines       25796    25799       +3     
  Branches    11850    11849       -1     
==========================================
- Hits        12820    12818       -2     
- Misses       4575     4580       +5     
  Partials     8401     8401              

Files Changed	Coverage Δ
...nclude/Acts/Vertexing/AdaptiveGridTrackDensity.hpp	100.00% <ø> (ø)
...nclude/Acts/Vertexing/GaussianGridTrackDensity.hpp	100.00% <ø> (ø)
...nclude/Acts/Vertexing/AdaptiveGridTrackDensity.ipp	61.15% <56.81%> (-1.26%)	⬇️
...nclude/Acts/Vertexing/GaussianGridTrackDensity.ipp	66.40% <84.61%> (-1.54%)	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[github-actions]

📊: Physics performance monitoring for f8a6c10

Full contents

physmon summary

• ✅ CKF truth_smeared
• ✅ IVF truth_smeared
• ✅ AMVF truth_smeared
• ✅ Track Summary CKF truth_smeared
• ✅ Seeding truth_estimated
• ✅ CKF truth_estimated
• ✅ IVF truth_estimated
• ✅ AMVF truth_estimated
• ✅ Track Summary CKF truth_estimated
• ✅ Seeding seeded
• ✅ CKF seeded
• ✅ IVF seeded
• ✅ AMVF seeded
• ✅ AMVF (+grid seeder) seeded
• ✅ Track Summary CKF seeded
• ✅ Seeding orthogonal
• ✅ CKF orthogonal
• ✅ IVF orthogonal
• ✅ AMVF orthogonal
• ✅ Track Summary CKF orthogonal
• ✅ Ambisolver seeded
• ✅ Ambisolver orthogonal
• ✅ Seeding ttbar
• ✅ CKF ttbar
• ✅ Ambisolver
• ✅ AMVF ttbar
• ✅ AMVF (+grid seeder) ttbar
• ✅ Truth tracking (GSF)
• ✅ Truth tracking
• ✅ Particles inital fatras
• ✅ Particles final fatras
• ✅ Particles inital geant4
• ✅ Particles final geant4

[baschlag]

Hi. Thanks for the PR!
First comment: You wrote '... such that the central bin corresponds to the interval [-binSize, binSize)'. If that's really the case, the central bin would be of size 2*binSize again (i.e. everything in this interval gets mapped to 0), wouldn't it? Can't we just make sure that everything in [0,binSize) gets mapped to bin number 0, whereas the interval [-binSize, 0) gets mapped to -1?
I'll have a closer look at the code soon and get back to you.

[baschlag]

Hi. Thanks for the PR!
First comment: You wrote '... such that the central bin corresponds to the interval [-binSize, binSize)'. If that's really the case, the central bin would be of size 2*binSize again (i.e. everything in this interval gets mapped to 0), wouldn't it? Can't we just make sure that everything in [0,binSize) gets mapped to bin number 0, whereas the interval [-binSize, 0) gets mapped to -1?
I'll have a closer look at the code soon and get back to you.

Just to add: in my understanding, the previous implementation also mapped [-binSize, 0) to 0(which is clearly wrong, so thanks for catching this!), and bin number -1 was just skipped. So we effectively had a binning like ..., -3, -2, 0, 1, 2, 3, ..., skipping -1 and having bin 0 twice as large, right?

[felix-russo]

Hi. Thanks for the PR!

Thanks for having a look at it!

First comment: You wrote '... such that the central bin corresponds to the interval [-binSize, binSize)'. If that's really the case, the central bin would be of size 2*binSize again (i.e. everything in this interval gets mapped to 0), wouldn't it?

Sorry, that was a very misleading typo, I meant to write [-binSize/2, binSize/2). In the code this is however implemented correctly I think: in the function getBin; we have:
return static_cast<int>(std::floor(value / m_cfg.binSize - 0.5) + 1);.
Substituting

• value = binSize/2 yields 1,
• value = binSize/2 - epsilon, where epsilon is a small positive number, yields 0
• value = -binSize/2 yields 0.

Here is some code you can have a look at https://onlinegdb.com/UGjDeZYK2.

Can't we just make sure that everything in [0,binSize) gets mapped to bin number 0, whereas the interval [-binSize, 0) gets mapped to -1?

We could, but since we impose an uneven number of bins this would introduce some asymmetry (e.g., for a track centered at z=0, there would be more positive than negative bins (or vice versa)).

Just to add: in my understanding, the previous implementation also mapped [-binSize, 0) to 0(which is clearly wrong, so thanks for catching this!), and bin number -1 was just skipped. So we effectively had a binning like ..., -3, -2, 0, 1, 2, 3, ..., skipping -1 and having bin 0 twice as large, right?

I think bin -1 was not skipped, rather

• values from bin -2 were mapped to bin -1,
• values from bin -3 were mapped -2,
• etc.

You can have a look at https://onlinegdb.com/mjoUXyTTT to confirm this.

I'll have a closer look at the code soon and get back to you.

Great, thanks a lot for your time!

[baschlag]

Ok, looks good to me! Thanks for the PR!

Invalidated by push of 9044fb2

[andiwand]

I looked at this again to make a port for the original AdaptiveGridTrackDensity here #3340 - that was really nice work @felix-russo 👏