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.

Sign up for GitHub

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

Jump to bottom

refactor: resurrect detector root volumes #2044

Merged
merged 8 commits into from
Apr 20, 2023
Merged

refactor: resurrect detector root volumes #2044

merged 8 commits into from
Apr 20, 2023

Conversation

andiwand
Copy link
Contributor

with #2027 I removed the ability to have multiple root volumes. this functionality is brought back in this PR because it is necessary for the KDT detector builder

@andiwand andiwand added the Component - Core Affects the Core module label Apr 20, 2023
@andiwand andiwand added this to the next milestone Apr 20, 2023
@andiwand andiwand requested a review from asalzburger April 20, 2023 10:08
asalzburger
asalzburger approved these changes Apr 20, 2023
View reviewed changes
Copy link
Contributor

@asalzburger asalzburger left a comment

Choose a reason for hiding this comment

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

I like this, we have now the possibility to create one or many root volumes, bravo!

@asalzburger
Copy link
Contributor

You need to add the mockup detector test as well ..

@codecov
Copy link

codecov bot commented Apr 20, 2023 •
edited
Loading

Codecov Report

Merging #2044 (ed5f5ae) into main (61c067e) will increase coverage by 0.01%.
The diff coverage is 56.52%.

❗ Current head ed5f5ae differs from pull request most recent head 83b9615. Consider uploading reports for the commit 83b9615 to get more accurate results

@@            Coverage Diff             @@
##             main    #2044      +/-   ##
==========================================
+ Coverage   49.82%   49.83%   +0.01%     
==========================================
  Files         420      420              
  Lines       23955    23958       +3     
  Branches    10857    10858       +1     
==========================================
+ Hits        11935    11940       +5     
- Misses       4388     4389       +1     
+ Partials     7632     7629       -3
Impacted Files Coverage Δ
Core/src/Detector/Detector.cpp 59.42% <50.00%> (+2.70%) ⬆️
.../include/Acts/Navigation/DetectorVolumeFinders.hpp 66.66% <71.42%> (-1.63%) ⬇️

... 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 20, 2023 •
edited
Loading

📊 Physics performance monitoring for ed5f5ae

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
Copy link
Contributor

Schaun mer mal ...

@andiwand
Copy link
Contributor Author

massaging clang tidy right now otherwise this should be done I think

@asalzburger asalzburger merged commit a1226a1 into acts-project:main Apr 20, 2023
@andiwand andiwand deleted the refactor-resurrect-detector-root-volumes branch April 20, 2023 14:02
@paulgessinger paulgessinger modified the milestones: next, v25.0.0 Apr 21, 2023
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Apr 25, 2023
@andiwand @asalzburger @paulgessinger
refactor: resurrect detector root volumes (acts-project#2044)
f329474 
with acts-project#2027 I removed the
ability to have multiple root volumes. this functionality is brought
back in this PR because it is necessary for the KDT detector builder

---------

Co-authored-by: Andreas Salzburger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asalzburger asalzburger asalzburger approved these changes

Assignees
No one assigned
Labels
Component - Core Affects the Core module
Projects
None yet
Milestone
v25.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@andiwand @asalzburger @paulgessinger