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!: change BinFinder logic #1038

Merged
merged 43 commits into from
Nov 4, 2021

Conversation

LuisFelipeCoelho
Copy link
Member

@LuisFelipeCoelho LuisFelipeCoelho commented Oct 11, 2021

This PR changes the BinFinder algorithm that returns the connected bins used to build seeds. Instead of always returning the 8 surrounding bins, the user may define a vector containing a map of z bins in the top and bottom sets of space points for each middle space point bin. The algorithm will only search for SP in the bins defined in the map. In case no map is defined, the algorithm returns the 8 surrounding bins for top and bottom layers.
That will allow us to configure the top and bottom maps independently form each other and also to
return the bins based on geometrical assumptions as it is done by the ATLAS algorithm, for example:

//  1     2     3     4     5     6     7     8     9     10     11     z bin index
// -------------------------------------------------------------------> Z[mm]
// vector containing the map of z bins in the top layer
seedingCfg.binFinderConfigTop = {{1},{1, 2},{2, 3},{3, 4},{4, 5},{5, 6, 7},{7, 8},{8, 9},{9, 10},{10, 11},{11}};
// vector containing the map of z bins in the bottom layer
seedingCfg.binFinderConfigBottom = {{1, 2},{2, 3},{3, 4},{4, 5},{5, 6},{6},{6, 7},{7, 8},{8, 9},{9, 10},{10, 11}};

these vectors are passed to BinFinder by the SeedingAlgorithm.

@noemina

Co-authored-by: robertlangenberg <56069170+robertlangenberg@users.noreply.github.com>
LuisFelipeCoelho and others added 5 commits October 20, 2021 12:17
Co-authored-by: robertlangenberg <56069170+robertlangenberg@users.noreply.github.com>
Co-authored-by: robertlangenberg <56069170+robertlangenberg@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Nov 3, 2021

Codecov Report

Merging #1038 (7b980b7) into main (20ca618) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1038      +/-   ##
==========================================
- Coverage   48.57%   48.54%   -0.04%     
==========================================
  Files         338      339       +1     
  Lines       17369    17380      +11     
  Branches     8216     8221       +5     
==========================================
  Hits         8437     8437              
- Misses       3179     3190      +11     
  Partials     5753     5753              
Impacted Files Coverage Δ
Core/include/Acts/Seeding/BinFinder.hpp 0.00% <ø> (ø)
Core/include/Acts/Seeding/BinFinder.ipp 0.00% <0.00%> (ø)
Core/include/Acts/Seeding/SpacePointGrid.hpp 0.00% <ø> (ø)
Core/include/Acts/Seeding/SpacePointGrid.ipp 0.00% <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 20ca618...7b980b7. Read the comment docs.

Copy link
Contributor

@robertlangenberg robertlangenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good!

Copy link
Member

@paulgessinger paulgessinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minimal changes after @robertlangenberg's recent approval, reapproving to get this in.

@paulgessinger
Copy link
Member

@LuisFelipeCoelho the SYCL build seems to choke:

FAILED: Tests/UnitTests/Plugins/Sycl/Seeding/CMakeFiles/ActsUnitTestSeedfinderSycl.dir/SeedfinderSyclTest.cpp.o 
/opt/intel/oneapi/compiler/2021.1.1/linux/bin/clang++  -DBOOST_ALL_NO_LIB -DBOOST_PROGRAM_OPTIONS_DYN_LINK -DVECMEM_DEBUG_MSG_LVL=0 -DVECMEM_HAVE_EXPERIMENTAL_PMR_MEMORY_RESOURCE -DVECMEM_SOURCE_DIR_LENGTH=15 -I../Core/include -ICore -I../Plugins/Sycl/include -I_deps/vecmem-src/core/include -I_deps/vecmem-src/sycl/include -isystem /usr/include/eigen3 -Werror -Wall -Wextra -Wpedantic -Wshadow -Wunused-local-typedefs -O3 -DNDEBUG    -std=gnu++17 -MD -MT Tests/UnitTests/Plugins/Sycl/Seeding/CMakeFiles/ActsUnitTestSeedfinderSycl.dir/SeedfinderSyclTest.cpp.o -MF Tests/UnitTests/Plugins/Sycl/Seeding/CMakeFiles/ActsUnitTestSeedfinderSycl.dir/SeedfinderSyclTest.cpp.o.d -o Tests/UnitTests/Plugins/Sycl/Seeding/CMakeFiles/ActsUnitTestSeedfinderSycl.dir/SeedfinderSyclTest.cpp.o -c ../Tests/UnitTests/Plugins/Sycl/Seeding/SeedfinderSyclTest.cpp
../Tests/UnitTests/Plugins/Sycl/Seeding/SeedfinderSyclTest.cpp:106:7: error: unused variable 'numPhiNeighbors' [-Werror,-Wunused-variable]
  int numPhiNeighbors = 1;
      ^
../Tests/UnitTests/Plugins/Sycl/Seeding/SeedfinderSyclTest.cpp:146:35: error: use of undeclared identifier 'zBinNeighborsBottom'
      Acts::BinFinder<SpacePoint>(zBinNeighborsBottom, numPhiNeighbors));
                                  ^
../Tests/UnitTests/Plugins/Sycl/Seeding/SeedfinderSyclTest.cpp:146:56: error: use of undeclared identifier 'numPhiNeighbors'
      Acts::BinFinder<SpacePoint>(zBinNeighborsBottom, numPhiNeighbors));
                                                       ^
../Tests/UnitTests/Plugins/Sycl/Seeding/SeedfinderSyclTest.cpp:148:35: error: use of undeclared identifier 'zBinNeighborsTop'
      Acts::BinFinder<SpacePoint>(zBinNeighborsTop, numPhiNeighbors));
                                  ^
../Tests/UnitTests/Plugins/Sycl/Seeding/SeedfinderSyclTest.cpp:148:53: error: use of undeclared identifier 'numPhiNeighbors'
      Acts::BinFinder<SpacePoint>(zBinNeighborsTop, numPhiNeighbors));
                                                    ^
5 errors generated.

@kodiakhq kodiakhq bot merged commit c8ef092 into acts-project:main Nov 4, 2021
@LuisFelipeCoelho LuisFelipeCoelho deleted the itk-binfinder branch November 4, 2021 12:08
@paulgessinger paulgessinger modified the milestones: next, v15.0.0 Nov 17, 2021
@paulgessinger
Copy link
Member

I now realize this was actually a breaking change. (Just commenting here for reference)

@paulgessinger paulgessinger changed the title feat: change BinFinder logic feat!: change BinFinder logic Dec 2, 2021
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants