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: make stepping logger volume agnostic #2454

Conversation

asalzburger
Copy link
Contributor

This PR removes the explicit setting of TrackingVolume in favour of the GeometryIdentifier (which is the only thing that is accessed from the TrackingVolume in the Step struct of the SteppingLogger and thus makes the step logging infrastructure accessible to the new Acts::Detector geometry as well.

@asalzburger asalzburger added this to the next milestone Sep 15, 2023
@github-actions github-actions bot added Component - Core Affects the Core module Component - Examples Affects the Examples module labels Sep 15, 2023
@paulgessinger
Copy link
Member

Should this change the actual output of the logger? It seems like it does, but I don't think we expect that.

@asalzburger
Copy link
Contributor Author

Should this change the actual output of the logger? It seems like it does, but I don't think we expect that.

It eventually could - the root-outout logger has had an internal logic when to use which volumeID, which for boundary surfaces can be ambiguous. Now, this is changed to a very simple logic:

  • if a surface exists (i.e. the Logger logs an intersection with a surface) the surface purely determines the geoID
  • if no surface exist, the geoID is given by the volume where the logging happened

@asalzburger
Copy link
Contributor Author

@paulgessinger can I download the artifacts of the failed job, so I can inspect if that's what is actually happening?

@paulgessinger
Copy link
Member

@asalzburger I'm afraid the file is not in the artifact. But if you confirm this change is intentional, I think it's fine to merge, I just want to avoid an "accidental" change we don't intend, that's all.

@codecov
Copy link

codecov bot commented Sep 15, 2023

Codecov Report

Merging #2454 (b8fc643) into main (d62a42f) will increase coverage by 0.00%.
The diff coverage is 66.66%.

@@           Coverage Diff           @@
##             main    #2454   +/-   ##
=======================================
  Coverage   49.78%   49.78%           
=======================================
  Files         461      461           
  Lines       26015    26017    +2     
  Branches    11921    11922    +1     
=======================================
+ Hits        12952    12953    +1     
  Misses       4610     4610           
- Partials     8453     8454    +1     
Files Changed Coverage Δ
.../include/Acts/Propagator/detail/SteppingLogger.hpp 53.33% <66.66%> (-0.52%) ⬇️

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

@asalzburger
Copy link
Contributor Author

Why @andiwand - this should be relatively quickly. The reason why the stepping test fails is explained above ... some boundary surfaces are now differently flagged.

@asalzburger asalzburger merged commit 68658f1 into acts-project:main Sep 19, 2023
57 checks passed
@asalzburger asalzburger deleted the feat-make-stepping-logger-volume-agnostic branch September 19, 2023 14:48
@paulgessinger paulgessinger removed this from the next milestone Sep 20, 2023
@paulgessinger paulgessinger added this to the v30.0.0 milestone Sep 20, 2023
AJPfleger pushed a commit to AJPfleger/acts that referenced this pull request Sep 29, 2023
This PR removes the explicit setting of `TrackingVolume` in favour of
the `GeometryIdentifier` (which is the only thing that is accessed from
the `TrackingVolume` in the `Step` struct of the `SteppingLogger` and
thus makes the step logging infrastructure accessible to the new
`Acts::Detector` geometry as well.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes Performance Component - Core Affects the Core module Component - Examples Affects the Examples module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants