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

refactor: New DetectorNavigator tests and some fixes #2988

Merged
merged 16 commits into from
Mar 6, 2024

Conversation

ssdetlab
Copy link
Contributor

@ssdetlab ssdetlab commented Feb 26, 2024

Adding new tests for the DetectorNavigator and introducing some fixes.

In the DetectorNavigator itself:

  1. Adding DetectorVolume, CandidateSurfaces and NavigationState initialization into initialize. Previously, if you ran with the endOfWorld aborter, Propagation didn't enter the stepping loop, because the current volume was nullptr and there were no candidates to try.

  2. Adding implicit endOfWorld check after the Portal update. Previously, the initialize was called and volume was set to the one the Navigator is currently exiting (volume finders treat the boundary as a part of the volume). This resulted in the stepping loop continuing until the limit in the number of steps is reached.

  3. Adding navigation breaks. Some of the aborters (related to the KF and its tests) were not exiting properly before.

  4. Removed initialize call in the post step to insure the proper abortion.

  5. Added iteration over the intersection list to store candidates. Previously, if the surface was intersected more than once, only one of the intersections was taken into account, even if more of them were valid.

  6. Took direction (e.g. Acts::Forward) into account during the NavigationState filling. Previously, the Backward direction propagation didn't work.

  7. Small changes in the material-related files to resolve the currentVolume conflicts in the compilation.

In the tests:

  1. Added initialization tests, analogous to the ones for the Navigator

  2. Navigation through a simple cubic geometry in the Forward and Backward directions. Consistency checks, and things like that.

  3. Tests that ensure the proper behaviour when the multiple valid intersections are encountered. Like the double crossing of the Cylindrical surface.

Core/include/Acts/Navigation/DetectorNavigator.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Navigation/DetectorNavigator.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Navigation/DetectorNavigator.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Navigation/DetectorNavigator.hpp Outdated Show resolved Hide resolved
Core/include/Acts/Navigation/DetectorNavigator.hpp Outdated Show resolved Hide resolved
ssdetlab and others added 2 commits February 26, 2024 12:21
Co-authored-by: Andreas Stefl <stefl.andreas@gmail.com>
Copy link

codecov bot commented Feb 26, 2024

Codecov Report

Attention: Patch coverage is 43.75000% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 48.68%. Comparing base (61b5ca0) to head (b4b6d9a).

Files Patch % Lines
Core/include/Acts/Navigation/DetectorNavigator.hpp 46.15% 4 Missing and 10 partials ⚠️
...nclude/Acts/Navigation/NavigationStateUpdaters.hpp 20.00% 0 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2988      +/-   ##
==========================================
+ Coverage   48.63%   48.68%   +0.05%     
==========================================
  Files         493      493              
  Lines       29043    29053      +10     
  Branches    13849    13854       +5     
==========================================
+ Hits        14124    14144      +20     
+ Misses       4971     4963       -8     
+ Partials     9948     9946       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ssdetlab
Copy link
Contributor Author

Returned the errors but in the exception throwing form.

asalzburger
asalzburger previously approved these changes Feb 28, 2024
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.

The changes look like they make sense to me.

I think we should get them in and start a testing setup for the ODD and the Telescope detector.

@ssdetlab
Copy link
Contributor Author

Added a simple concept to check the currentVolume return types. The module itself is more of an example/direction that we can take during the transition between the old- and the new-style geometry descriptions.

Something like that may already be useful in some parts of the codebase, so we may want to create later a separate PR with a bit more thought through implementation and a bit more wider coverage.

@asalzburger asalzburger added this to the next milestone Mar 5, 2024
@asalzburger asalzburger merged commit b7f6cc0 into acts-project:main Mar 6, 2024
54 checks passed
@asalzburger
Copy link
Contributor

I will try merge the Open Data Detector v4.0.1 today/tomorrow, so we can set up a first geometry building and propagation test.

@paulgessinger paulgessinger modified the milestones: next, v33.0.0 Mar 6, 2024
Ragansu pushed a commit to Ragansu/acts that referenced this pull request Mar 7, 2024
Adding new tests for the `DetectorNavigator` and introducing some fixes.
 
In the `DetectorNavigator` itself:

1) Adding `DetectorVolume`, `CandidateSurfaces` and `NavigationState`
initialization into `initialize`. Previously, if you ran with the
`endOfWorld` aborter, Propagation didn't enter the stepping loop,
because the current volume was `nullptr` and there were no candidates to
try.

2) Adding implicit `endOfWorld` check after the `Portal` update.
Previously, the `initialize` was called and volume was set to the one
the Navigator is currently exiting (volume finders treat the boundary as
a part of the volume). This resulted in the stepping loop continuing
until the limit in the number of steps is reached.

3) Adding navigation breaks. Some of the aborters (related to the KF and
its tests) were not exiting properly before.

4) Removed `initialize` call in the post step to insure the proper
abortion.

5) Added iteration over the intersection list to store candidates.
Previously, if the surface was intersected more than once, only one of
the intersections was taken into account, even if more of them were
valid.

6) Took direction (e.g. `Acts::Forward`) into account during the
`NavigationState` filling. Previously, the `Backward` direction
propagation didn't work.

7) Small changes in the material-related files to resolve the
`currentVolume` conflicts in the compilation.

In the tests:

1) Added initialization tests, analogous to the ones for the `Navigator`

2) Navigation through a simple cubic geometry in the `Forward` and
`Backward` directions. Consistency checks, and things like that.

3) Tests that ensure the proper behaviour when the multiple valid
intersections are encountered. Like the double crossing of the
Cylindrical surface.

---------

Co-authored-by: Andreas Stefl <stefl.andreas@gmail.com>
Co-authored-by: Andreas Salzburger <asalzburger@gmail.com>
EleniXoch pushed a commit to EleniXoch/acts that referenced this pull request May 6, 2024
Adding new tests for the `DetectorNavigator` and introducing some fixes.
 
In the `DetectorNavigator` itself:

1) Adding `DetectorVolume`, `CandidateSurfaces` and `NavigationState`
initialization into `initialize`. Previously, if you ran with the
`endOfWorld` aborter, Propagation didn't enter the stepping loop,
because the current volume was `nullptr` and there were no candidates to
try.

2) Adding implicit `endOfWorld` check after the `Portal` update.
Previously, the `initialize` was called and volume was set to the one
the Navigator is currently exiting (volume finders treat the boundary as
a part of the volume). This resulted in the stepping loop continuing
until the limit in the number of steps is reached.

3) Adding navigation breaks. Some of the aborters (related to the KF and
its tests) were not exiting properly before.

4) Removed `initialize` call in the post step to insure the proper
abortion.

5) Added iteration over the intersection list to store candidates.
Previously, if the surface was intersected more than once, only one of
the intersections was taken into account, even if more of them were
valid.

6) Took direction (e.g. `Acts::Forward`) into account during the
`NavigationState` filling. Previously, the `Backward` direction
propagation didn't work.

7) Small changes in the material-related files to resolve the
`currentVolume` conflicts in the compilation.

In the tests:

1) Added initialization tests, analogous to the ones for the `Navigator`

2) Navigation through a simple cubic geometry in the `Forward` and
`Backward` directions. Consistency checks, and things like that.

3) Tests that ensure the proper behaviour when the multiple valid
intersections are encountered. Like the double crossing of the
Cylindrical surface.

---------

Co-authored-by: Andreas Stefl <stefl.andreas@gmail.com>
Co-authored-by: Andreas Salzburger <asalzburger@gmail.com>
asalzburger added a commit to asalzburger/acts that referenced this pull request May 21, 2024
Adding new tests for the `DetectorNavigator` and introducing some fixes.
 
In the `DetectorNavigator` itself:

1) Adding `DetectorVolume`, `CandidateSurfaces` and `NavigationState`
initialization into `initialize`. Previously, if you ran with the
`endOfWorld` aborter, Propagation didn't enter the stepping loop,
because the current volume was `nullptr` and there were no candidates to
try.

2) Adding implicit `endOfWorld` check after the `Portal` update.
Previously, the `initialize` was called and volume was set to the one
the Navigator is currently exiting (volume finders treat the boundary as
a part of the volume). This resulted in the stepping loop continuing
until the limit in the number of steps is reached.

3) Adding navigation breaks. Some of the aborters (related to the KF and
its tests) were not exiting properly before.

4) Removed `initialize` call in the post step to insure the proper
abortion.

5) Added iteration over the intersection list to store candidates.
Previously, if the surface was intersected more than once, only one of
the intersections was taken into account, even if more of them were
valid.

6) Took direction (e.g. `Acts::Forward`) into account during the
`NavigationState` filling. Previously, the `Backward` direction
propagation didn't work.

7) Small changes in the material-related files to resolve the
`currentVolume` conflicts in the compilation.

In the tests:

1) Added initialization tests, analogous to the ones for the `Navigator`

2) Navigation through a simple cubic geometry in the `Forward` and
`Backward` directions. Consistency checks, and things like that.

3) Tests that ensure the proper behaviour when the multiple valid
intersections are encountered. Like the double crossing of the
Cylindrical surface.

---------

Co-authored-by: Andreas Stefl <stefl.andreas@gmail.com>
Co-authored-by: Andreas Salzburger <asalzburger@gmail.com>
@ssdetlab ssdetlab deleted the detector-navigation-tests branch October 11, 2024 12:41
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.

5 participants