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: cylindrical detector component builder #2065

Merged

Conversation

asalzburger
Copy link
Contributor

This PR binds the development of the:

  • internal DetectorVolume structure
  • external DetectorVolume glueing

together and establishes a first set of tools for cylindrical detectors.

It also - with everything in place - introduced a clear separation between:

  • virtual interfaces for detector building in interface
  • implementation details that are not exposed in detail
  • data classes and builders in top level

It is currently blocked by #1983

@asalzburger asalzburger added Component - Core Affects the Core module 🛑 blocked This item is blocked by another item labels Apr 26, 2023
@asalzburger asalzburger added this to the next milestone Apr 26, 2023
@codecov
Copy link

codecov bot commented Apr 26, 2023

Codecov Report

Merging #2065 (d121e62) into main (3918f38) will increase coverage by 0.07%.
The diff coverage is 46.39%.

@@            Coverage Diff             @@
##             main    #2065      +/-   ##
==========================================
+ Coverage   49.32%   49.40%   +0.07%     
==========================================
  Files         427      434       +7     
  Lines       24833    24984     +151     
  Branches    11444    11524      +80     
==========================================
+ Hits        12250    12343      +93     
+ Misses       4514     4486      -28     
- Partials     8069     8155      +86     
Impacted Files Coverage Δ
Core/include/Acts/Detector/DetectorVolume.hpp 74.28% <ø> (+30.28%) ⬆️
...re/include/Acts/Detector/LayerStructureBuilder.hpp 100.00% <ø> (ø)
.../Acts/Detector/detail/IndexedSurfacesGenerator.hpp 60.00% <ø> (ø)
...clude/Acts/Detector/detail/KdtSurfacesProvider.hpp 57.14% <ø> (+5.86%) ⬆️
...s/Detector/interface/IInternalStructureBuilder.hpp 100.00% <ø> (ø)
.../src/Detector/detail/CylindricalDetectorHelper.cpp 34.65% <31.95%> (+0.71%) ⬆️
Core/src/Detector/LayerStructureBuilder.cpp 35.57% <33.33%> (-0.62%) ⬇️
...clude/Acts/Detector/detail/ReferenceGenerators.hpp 43.75% <43.75%> (ø)
...include/Acts/Detector/detail/IndexedGridFiller.hpp 58.82% <50.00%> (ø)
Core/src/Detector/DetectorVolumeBuilder.cpp 50.00% <50.00%> (ø)
... and 8 more

... and 1 file with indirect coverage changes

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

@github-actions
Copy link

github-actions bot commented Apr 26, 2023

📊 Physics performance monitoring for d121e62

Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
Ambiguity resolution: seeded, orthogonal
Truth tracking
Truth tracking (GSF)

Vertexing

Vertexing vs. mu
IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

@asalzburger asalzburger force-pushed the feat-detector-component-builder branch from 7fb52b8 to 71120ba Compare April 26, 2023 20:43
@asalzburger asalzburger removed the 🛑 blocked This item is blocked by another item label Apr 26, 2023
@asalzburger asalzburger force-pushed the feat-detector-component-builder branch from 33ff861 to e269d2f Compare April 27, 2023 18:11
@asalzburger asalzburger changed the title feat: feat detector component builder feat: cylindrical detector component builder Apr 27, 2023
@asalzburger
Copy link
Contributor Author

Hey @andiwand @noemina - this PR binds it all together & harmonies the prior PRs:

  • it introduces very clear virtual Interfaces for geometry building
  • it introduces everything to build cylindrical-like detectors

I will follow it by one showcasing building the ODD and then eventually the ITk.

Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

part 1

Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

part 2

Core/include/Acts/Detector/CylindricalContainerBuilder.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Detector/CylindricalContainerBuilder.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Detector/DetectorComponents.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Detector/DetectorComponents.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Detector/DetectorVolumeBuilder.hpp Outdated Show resolved Hide resolved
@asalzburger asalzburger requested a review from noemina May 5, 2023 12:07
@asalzburger
Copy link
Contributor Author

asalzburger commented May 5, 2023

Hi @noemina @andiwand - I have addressed all comments (that I then have resolved), the ones I added explanation I kept open for your cross-checking.

Please resolve them if the answer satisfies you.

Copy link
Contributor

@andiwand andiwand left a comment

Choose a reason for hiding this comment

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

lets go

@asalzburger asalzburger removed the request for review from noemina May 5, 2023 19:20
@asalzburger asalzburger merged commit 0558f1d into acts-project:main May 6, 2023
58 checks passed
@paulgessinger paulgessinger modified the milestones: next, v26.0.0 May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants