The current binning tools use floats everywhere, which is prone to conversion warnings. This commit converts that code to use the ActsScalar type alias instead.

Aug 06 '24 09:08 stephenswat

📊: Physics performance monitoring for 05f88054d2cb90ea95ccf10af7e604f79f3f1b25

Full contents

physmon summary

Aug 08 '24 19:08 github-actions[bot]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
81.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

Aug 08 '24 20:08 sonarqubecloud[bot]

always interesting to see what triggers the 3 ttbar events to change

Aug 09 '24 08:08 andiwand

I was looking at the physmon output. Some of the plots have random spikes. The outliers in CKF | trackfinding | ttbar with 200 pileup | default seeding change a lot.

The use of double instead of float seems correct, but I think we should investigate the underlying cause first before getting this in. What do you think?

Sep 09 '24 16:09 AJPfleger

Huh, that's peculiar. :thinking:

Sep 10 '24 09:09 stephenswat

Ultimately I think this changed the seeding a bit which then changes the track finding a bit. The plots don't scare me too much since we have very low stats there.

What I am not sure about is IF we actually want to change the seeding here? @stephenswat @paulgessinger @asalzburger

If that is clear I am happy to approve and bring this in.

Oct 02 '24 14:10 andiwand

Regarding the IF, I have no strong opinion.

It's annoying for the low stats, but maybe we should re-run with better stats. I will check the logs, how expensive the individual tests are, but if we are lucky we could upscale them at a cost of an extra minute or so :3

Oct 02 '24 14:10 AJPfleger

Why this affects the seeding is not entirely clear to me.

I guess it could affect Fatras and then subsequently reconstruction.

Oct 02 '24 16:10 paulgessinger

The current physmon tests take around 11-18 minutes. In the trackfinding_ttbar_pu200 we spend around 75-135 seconds. I think, we could double the number of events from 3 to 6, but everything else would take too much time. I tried it with 30 events and it takes 850s on github. As expected, we scale here linearly.

Oct 02 '24 21:10 AJPfleger

Why this affects the seeding is not entirely clear to me.

I guess it could affect Fatras and then subsequently reconstruction.

Isn't

Core/include/Acts/Utilities/BinUtility.hpp
Core/include/Acts/Utilities/BinningData.hpp something that is used in seeding?

The current physmon tests take around 11-18 minutes. In the trackfinding_ttbar_pu200 we spend around 75-135 seconds. I think, we could double the number of events from 3 to 6, but everything else would take too much time. I tried it with 30 events and it takes 850s on github. As expected, we scale here linearly.

I think we would need 100-1000 events so I would rather keep it as it is for now if we cannot reach that

Oct 03 '24 07:10 andiwand

I was under the impression it uses the Grid for the internal binning, but I could be wrong.

Oct 03 '24 10:10 paulgessinger

In the seeding we use the Acts::Grid, but I'm not familiar with neither of Core/include/Acts/Utilities/BinUtility.hpp or Core/include/Acts/Utilities/BinningData.hpp and I don't know if they may be use internally somewhere

Oct 04 '24 08:10 CarloVarni

Invalidated by #3873.

Nov 20 '24 13:11 stephenswat

we could still have this with double instead of the floats, no?

Nov 20 '24 13:11 AJPfleger

fix!: Use `ActsScalar` in binning tools

📊: Physics performance monitoring for 05f88054d2cb90ea95ccf10af7e604f79f3f1b25

physmon summary

Quality Gate passed