Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: ITK SeedFinder integration #1084

Merged
merged 39 commits into from
Jan 14, 2022

Conversation

LuisFelipeCoelho
Copy link
Member

This PR introduces several changes to the SeedFinder algorithm:

  1. Introduced a variable radius range (rMinMiddleSP, rMaxMiddleSP) for the middle SP:
  • Obtained by a floor approximation to an even number of the minimum and maximum SP radius found plus a ΔR (deltaRMiddleSPRange) defined by the user
  • An Extent rRangeSPExtent is used to get the min and max values
  • This cut is only applied if a boolean useVariableMiddleSPRange defined by the user is true
  • If useVariableMiddleSPRange is set to false, the vector rRangeMiddleSP can be used to define a fixed r range. If the vector is empty the cuts won't be applied.
  1. Check if middle SP is on the first or last disk with zMax and zMin

  2. Introduced seedConfirmation step that requires a minimum number of top SP depending on whether the middle SP is in the central or forward region:

  • If the boolean seedConfirmation is set to true the cut will be applied
  1. Changed the order of the loops over the top and bottom space points:
  • Looping over the top SP first. The algorithm doesn’t need to unnecessarily loop over the bottom SP if the minimum number of top SP is not greater than nTopSeedConf when seedConfirmation is set to true
  1. Introduced deltaRMinBottomSP, deltaRMaxBottomSP, deltaRMinTopSP and deltaRMaxTopSP:
  • ΔR minimum and maximum between middle and top/bottom SP can now be defined separately, as opposed to having the same value for all
  1. Added a cut on A and B coefficients for the top and bottom SP

During SP comparison:

  1. linCircleBottom and linCircleTop are sorted by cotTheta of the bottom and top space points inside transformCoordinates

  2. Added the t0 index

  3. In the cut on the compatibility between the r-z slope of the two seed segments, deltaCotTheta2 - error2 > 0 was changed to deltaCotTheta2 - error2 > sigmaSquaredScatteringMinPt, where sigmaSquaredScatteringMinPt is an approximate worst case multiple scattering term assuming the lowest pT we allow and the estimated theta angle

  • dCotThetaMinusError2 > scatteringInRegion2 cut removed
  • if deltaCotTheta < 0 → break
  • else → t0=t+1 and continue
  1. Added a second 1/tan(theta) compatibility cut (deltaCotTheta2 - error2) * S2 > B2 * iSinTheta2 * m_config.kConstant * std::pow(2 / m_config.pTPerHelixRadius, 2)
  • if deltaCotTheta < 0 → break
  • else → t0=t and continue
  1. (deltaCotTheta2-error2 > 0) && (dCotThetaMinusError2 > p2scatter * m_config.sigmaScattering * m_config.sigmaScattering cut removed

  2. Added the evaluation of eta of the seed

@noemina @asalzburger @paulgessinger @robertlangenberg

@LuisFelipeCoelho LuisFelipeCoelho changed the title ITK SeedFinder integration feat: ITK SeedFinder integration Nov 25, 2021
@codecov
Copy link

codecov bot commented Nov 25, 2021

Codecov Report

Merging #1084 (b44ab7a) into main (f4cc9e3) will decrease coverage by 0.22%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1084      +/-   ##
==========================================
- Coverage   48.40%   48.18%   -0.23%     
==========================================
  Files         341      341              
  Lines       17563    17644      +81     
  Branches     8296     8323      +27     
==========================================
  Hits         8502     8502              
- Misses       3286     3367      +81     
  Partials     5775     5775              
Impacted Files Coverage Δ
Core/include/Acts/Seeding/SeedFinderUtils.ipp 0.00% <0.00%> (ø)
Core/include/Acts/Seeding/Seedfinder.hpp 0.00% <ø> (ø)
Core/include/Acts/Seeding/Seedfinder.ipp 0.00% <0.00%> (ø)
Core/include/Acts/Seeding/SeedfinderConfig.hpp 0.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4cc9e3...b44ab7a. Read the comment docs.

@robertlangenberg
Copy link
Contributor

robertlangenberg commented Nov 25, 2021

Hi @LuisFelipeCoelho, that's an impressive list of changes :)

I have a couple of clarification questions: Could you explain these two points in a bit more detail? I'm not sure what you mean.

Obtained by a floor approximation to an even number of the minimum and maximum SP radius found plus a ΔR (deltaRMiddleSPRange) defined by the user

An Extent rRangeSPExtent is used to get the min and max values

For efficiency I tried to exclude space points which can be cut by looking only at a single space point already during the grid building step, but middle sp being in the outermost loop it probably doesn't matter all that much. The check for zMin and zMax is one such example, which is already applied during the grid creation (so there shouldn't be any sp with z > zMax.

3 ) seems very ATLAS specific, isn't this something that could be implemented in the experiment specific cuts?

4 ) did you measure the performance change from this?

6 ) I don't exaclty understand the cut, but it also sounds like it would be more efficient to exclude these in the grid building step. The Grid is separate from the Seedfinder exactly for this reason, to be able to more intelligently select space points than what the SPGrid offers, i.e. replacing the SPGrid with something else. Using cut 1) and 5) e.g. it sounds like it would be reasonable to have three grids: one with all SP in the middle range, one with SP in the bottom range and one with SP in the top range, which could reduce the number of iterations significantly.

9 ) and 10 ) Please check to factor this out as discussed. also we should find out if it is (significantly) faster and how much and under which conditions this deviates from the accurate scattering?

@robertlangenberg
Copy link
Contributor

robertlangenberg commented Nov 29, 2021

Regarding point 3)

In the SeedFilter, the experimentCuts are applied in two locations. The "minimum number of top SP for confirmation, depending on the detector region" introduced in this PR is very specific. The Seedfinder itself is unaware of any layers (though of course you could define a forward region for any detector). Instead of putting this functionality into the Seedfinder algorithm itself, I think this is so ATLAS-specific that it would make sense to move it to such an experiment-specific cut which would be ATLAS-only and not necessarily implemented in Acts.


after a chat we decided to put this in the algorithm directly as it allows rejecting middle SP for which only few compatible top SP exist at an early stage (before even looking at bottom SP), such that this could save time.

@LuisFelipeCoelho
Copy link
Member Author

Hi! @robertlangenberg

Regarding the other comments:

  1. Obtained by a floor approximation to an even number of the minimum and maximum SP radius found plus a ΔR (deltaRMiddleSPRange) defined by the user
    An Extent rRangeSPExtent is used to get the min and max values

What I meant is that I am filling an extent with (x, y, z) information of the SP in the SeedingAlgorithm.cpp. Then, in the SeedFinder, I get the maximum and minimum R values from that extent and use them to calculate rMinMiddleSP and rMaxMiddleSP. In Athena 21.9 they do an approximation to an even number, which I believe is the same as std::floor( value/2 ) * 2

  1. I removed the cut on zMin and zMax, since it is already applied during the grid creation, as you said.
    Do you think check 1 should also be applied during grid creation?

  2. I compared the performance of this change, and looping over the top SP first seems to be a bit faster.

Screenshot 2021-11-29 at 13 09 02

  1. and 10. I actually managed to get the same results without the magic kConstant.
    I also checked the performance and it is the same, so I don’t think it is worth using the approximation.

@LuisFelipeCoelho LuisFelipeCoelho added this to the next milestone Dec 2, 2021
@paulgessinger
Copy link
Member

paulgessinger commented Jan 6, 2022

I think the only thing that remains are the root file hash failures. This means that the output of the following root files changes:

  • test_seeding__estimatedparams.root
  • test_seeding__performance_seeding_trees.root
  • test_ckf_tracks_example_full_seeding__performance_seeding_trees.root
  • test_ckf_tracks_example_full_seeding__performance_seeding_trees.root
  • test_ckf_tracks_example_full_seeding__trackstates_ckf.root
  • test_ckf_tracks_example_full_seeding__tracksummary_ckf.root

However, that doesn't mean that there's something wrong with this PR necessarily. I assume some change in output is expected, because the hash will change if the outputs are not exactly identical (event ordering is ignored). If the seeding algorithms are changed, the output might change as well.

Can you

  1. Check whether you expect the output to be the same? I.e. do you expect the "default" case to produce identical output as before? If you do, then there's maybe a bug somewhere
  2. If you don't, check (at least briefly) that the new root output files contain sensible contents. Basically just check that it doesn't look broken, since a broken reference doesn't help us a lot.

If you conclude the root file changes are expected and acceptable, we can just update the reference hash, which should make the CI go green this PR ready to go.

@LuisFelipeCoelho
Copy link
Member Author

We have studied the performance and we saw a degradation in tracking efficiency both for seeds and tracks when sorting the SP in cotangent of theta and applying cuts based on the sorting. However, since we want to have this code in for emulating the ITk behaviour, I added a new boolean in the SeedFinderConfig called EnableCutsForSortedSP which enables the sorting and the cuts in cotTheta if needed. It is false by default and the code runs as before.

The performance should also be tested once the code is integrated in Athena for proper testing with ITk.

@paulgessinger
Copy link
Member

I think the reduction in coverage is acceptable.

@paulgessinger paulgessinger merged commit 25a0f0b into acts-project:main Jan 14, 2022
@paulgessinger paulgessinger modified the milestones: next, v17.0.0 Mar 1, 2022
kodiakhq bot pushed a commit that referenced this pull request Mar 18, 2022
This PR adds the `ITkSeedingExample.cpp` to run the ITk seeding.
It shows how to configure the non-equidistant binning in z (#1005), the seed confirmation cuts (#1084), the radial range for middle SP cut (#1084) and the vector containing the map of z neighbours (#1052 and #1038).

It contains all the parameters to run the seeding for ITk **pixel** space points (I intend to extend this to ITk strip SPs soon).

@noemina @paulgessinger
kodiakhq bot pushed a commit that referenced this pull request Mar 21, 2022
In PR #1084 I mentioned that we were seeing a degradation in tracking efficiency when sorting the SP in cotangent of theta in the `SeedFinder`. However I realised that I was sorting only the `linCircleVec` vector in `SeedFinder` and the `compatBottomSP` and `compatTopSP` vectors should also be sorted. 

This PR solves the bug and now the efficiency and the output should be the same as without the sorting.

@paulgessinger @robertlangenberg @noemina
kodiakhq bot pushed a commit that referenced this pull request Mar 29, 2022
A cut on the compatibility between the SP and the interaction point was added to the seedFinder in PR #1084

The cut may not always be necessary or wanted so this PR makes the cut optional by introducing a boolean `interactionPointCut`

This is also useful for the ITk implementation because this cut is used for the pixel SPs but not for the strip SPs
kodiakhq bot pushed a commit that referenced this pull request Nov 15, 2022
This was included in the orthodox seeding in #1084
CarloVarni pushed a commit to CarloVarni/acts that referenced this pull request Dec 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants