fix: Prevent unphysical number of phi bins (#1161) #1167
Conversation
stephenswat
added
Bug
|
Looks fine from here, @LuisFelipeCoelho can you review this PR? |
|
Looks fine to me. Thanks for finding the problem and fixing it! |
Codecov Report
@@ Coverage Diff @@
## main #1167 +/- ##
==========================================
- Coverage 47.84% 47.84% -0.01%
==========================================
Files 360 360
Lines 18523 18525 +2
Branches 8737 8739 +2
==========================================
Hits 8863 8863
- Misses 3624 3626 +2
Partials 6036 6036
Continue to review full report at Codecov.
|
|
Hmm, it looks like some of the tests fail. |
|
Okay, so one of the Python tests appears to be losing tracks. I would suspect then, that one phi bin neighbour is not enough. Does anyone have a suggestion for a better default value? Otherwise, we might want to look into a mechanism that can ensure that this value is set. |
|
This may be related to #1172 as the size of the phi bins reduced. In principle a single phi bin should cover the maximum deflection in phi such that looking at a single neighbor should be enough as long as numPhiNeighbors is 1. This didn't change, right @noemina @LuisFelipeCoelho ? |
That's exactly what it does. We didn't change the logic there. |
|
From the discussion in #1172 I gather that 0 is probably a better default value than 1 in this case. |
stephenswat
force-pushed
the
fix/unphysical_phi_bins
branch
from
4c3ce61
to
692d30b
Compare
The bug described in issue acts-project#1161 is caused by the creation of negative or (near) infinite number of phi bins, which cause the internal mechanisms of `std::vector` to crash. This effect is caused by the fact that the number of phi neighbours is not properly initialized, causing it to take on uninitialized values. This commit does two things to prevent this issue. First of all, it adds a sensible default value for the number of phi neighbours, ensuring that it never takes on uninitialized values. Secondly, a sanity check is added to the space point grid to ensure that the phi delta value has a physically meaningful value; if this is not the case, an exception is thrown. Resolves acts-project#1161.
stephenswat
force-pushed
the
fix/unphysical_phi_bins
branch
from
692d30b
to
1ec748a
Compare
|
I just had a quick chat with Noemi, and I think I understand the issue. There are configuration parameters called numPhiNeighbors for Grid and BinFinder which don't mean the same. The numPhiNeighbors for the Grid means "how many phiBin neighbors (plus the current bin) should cover the full deflection of a minimum pT particle". Setting this to zero means "no phi bin neighbors needed to cover the full deflection", i.e. 1 phi-bin covers the full deflection. What has been newly added is the consideration of the max impact parameters, which increases the size of the bins a bit and is probably uncontroversial, but may slightly change results. I think this should be renamed and even numbers of phiBins should be allowed to cover the full deflection. The numPhiNeighbors parameter of the BinFinder sets how many neighboring phi bins (plus the current one) are returned for each call to "findBins" (which I think is aptly named). To get the behavior the seeding test expects, (assuming the newly added d_ZERO doesn't change the results as well), the numPhiNeighbors of the Grid has to be set to zero, while the numPhiNeighbors of the BinFinder must be set to 1. |
|
The reason this was changed like this is because for ATLAS at µ=60 the configuration with numPhiNeighbors=1 for both Grid and BinFinder has shown to give acceptable results and reduces CPU usage (due to the reduced combinatorics, this corresponds to a 66% reduction of the phi-range). |
The bug described in issue #1161 is caused by the creation of negative or (near) infinite number of phi bins, which cause the internal mechanisms of
std::vectorto crash. This effect is caused by the fact that the number of phi neighbours is not properly initialized, causing it to take on uninitialized values.This commit does two things to prevent this issue. First of all, it adds a sensible default value for the number of phi neighbours, ensuring that it never takes on uninitialized values. Secondly, a sanity check is added to the space point grid to ensure that the phi delta value has a physically meaningful value; if this is not the case, an exception is thrown.
Resolves #1161.