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: adding portal, detector volume & detector #1645

Merged
merged 45 commits into from
Nov 22, 2022

Conversation

asalzburger
Copy link
Contributor

This PR adds the new Portal, DetectorVolume and Detector geometry which is meant to eventually replace the Layer, TrackingVolume, TrackingGeometry infrastructure.

The new classes are currently put under the Experimental namespace.

Only the new classes, the necessary definitions and unit tests are added with this PR, which will be followed up by the visualisation, the detector building and the navigation code.

A description of the concepts and classes is added to the
docs/experimental_geometry.md to this PR.

@asalzburger asalzburger added the 🚧 WIP Work-in-progress label Nov 3, 2022
@asalzburger asalzburger added this to the next milestone Nov 3, 2022
@asalzburger asalzburger added the Component - Core Affects the Core module label Nov 3, 2022
@codecov
Copy link

codecov bot commented Nov 6, 2022

Codecov Report

Merging #1645 (ed0b274) into main (4e2c890) will increase coverage by 0.39%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main    #1645      +/-   ##
==========================================
+ Coverage   48.58%   48.98%   +0.39%     
==========================================
  Files         382      397      +15     
  Lines       21061    21497     +436     
  Branches     9645     9801     +156     
==========================================
+ Hits        10233    10530     +297     
- Misses       4147     4171      +24     
- Partials     6681     6796     +115     
Impacted Files Coverage Δ
Core/include/Acts/Geometry/VolumeBounds.hpp 40.00% <ø> (ø)
Core/src/Geometry/DetectorVolume.cpp 47.25% <47.25%> (ø)
Core/src/Geometry/Detector.cpp 47.61% <47.61%> (ø)
.../include/Acts/Geometry/detail/PortalGenerators.hpp 55.00% <55.00%> (ø)
Core/include/Acts/Geometry/detail/PortalHelper.hpp 66.66% <66.66%> (ø)
Core/include/Acts/Utilities/Helpers.hpp 68.60% <66.66%> (-0.08%) ⬇️
...Acts/Geometry/detail/SurfaceCandidatesUpdators.hpp 70.73% <70.73%> (ø)
...e/Acts/Geometry/detail/NavigationStateUpdators.hpp 72.41% <72.41%> (ø)
...de/Acts/Geometry/detail/DetectorVolumeUpdators.hpp 73.07% <73.07%> (ø)
Core/src/Geometry/Portal.cpp 73.46% <73.46%> (ø)
... and 12 more

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

@github-actions
Copy link

github-actions bot commented Nov 6, 2022

📊 Physics performance monitoring for ed0b274

🟥 ERROR The result has missing elements!
This is likely a physmon job failure

Full report
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_smeared

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

Core/include/Acts/Geometry/DetectorVolume.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Geometry/NavigationDelegates.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Geometry/Portal.hpp Show resolved Hide resolved
Core/include/Acts/Definitions/Common.hpp Outdated Show resolved Hide resolved
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.

took a first look but left out the test code for now

Core/include/Acts/Definitions/Common.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Geometry/Detector.hpp Show resolved Hide resolved
Core/include/Acts/Geometry/DetectorVolume.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Geometry/DetectorVolume.hpp Show resolved Hide resolved
Core/include/Acts/Geometry/NavigationDelegates.hpp Outdated Show resolved Hide resolved
Core/src/Geometry/DetectorVolume.cpp Outdated Show resolved Hide resolved
Core/src/Geometry/DetectorVolume.cpp Outdated Show resolved Hide resolved
Core/src/Geometry/Portal.cpp Outdated Show resolved Hide resolved
Core/src/Geometry/Portal.cpp Outdated Show resolved Hide resolved
@asalzburger asalzburger removed the 🚧 WIP Work-in-progress label Nov 8, 2022
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.

This is how far I got so far. Commenting this first batch now, but might have more later.

Core/include/Acts/Geometry/NavigationDelegates.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Definitions/Common.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Geometry/Detector.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Geometry/Detector.hpp Show resolved Hide resolved
Core/include/Acts/Geometry/Detector.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Geometry/NavigationDelegates.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Geometry/DetectorVolume.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Geometry/DetectorVolume.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Geometry/DetectorVolume.hpp Show resolved Hide resolved
@asalzburger asalzburger force-pushed the feat-portal-volume-detector branch 2 times, most recently from 1ef4a8c to beee566 Compare November 10, 2022 12:42
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.

I can't really provide any more feedback right now. for me it would be good to bring it in and then test it with some real examples

@asalzburger
Copy link
Contributor Author

There's one discussion point open, i.e. whether we should give the access to the const object to the iterators.
My suggestion is:

  • we bring it in like this, and I try in a separate PR how an iterator access would look like and if it's feasible.

@asalzburger
Copy link
Contributor Author

I've updated this to the new OwningDelegate infrastructure, it made the things quite easier, in fact.

@asalzburger asalzburger requested review from paulgessinger and removed request for andiwand November 18, 2022 10:19
@asalzburger
Copy link
Contributor Author

The debug unit test seems to fail, checking it now.

@asalzburger
Copy link
Contributor Author

Hey @andiwand - I think now all the clang-tidy fixes are also through, and the PR is reworked after @paulgessinger 's introduction of the OwningDelegate.

There will be some ironing out of a few things certainly along the way, but since this is under Experimental namespace, I hope we can move that in, in order not to block other development.

@asalzburger
Copy link
Contributor Author

@andiwand - if you're happy, please re-approve

@asalzburger
Copy link
Contributor Author

Since @andiwand approved and gave a thumbs up, I will merge that in.

@asalzburger asalzburger merged commit 3898a7f into acts-project:main Nov 22, 2022
@paulgessinger paulgessinger modified the milestones: next, v22.0.0 Dec 21, 2022
CarloVarni pushed a commit to CarloVarni/acts that referenced this pull request Dec 22, 2022
This PR adds the new Portal, DetectorVolume and Detector geometry which is meant to eventually replace the Layer, TrackingVolume, TrackingGeometry infrastructure.

The new classes are currently put under the Experimental namespace.

Only the new classes, the necessary definitions and unit tests are added with this PR, which will be followed up by the visualisation, the detector building and the navigation code.

A description of the concepts and classes is added to the
docs/experimental_geometry.md to this PR.
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.

4 participants