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!: Remove navigator layer bounds check option #2851

Merged

Conversation

andiwand
Copy link
Contributor

This option is supposed to improve the navigation but can also cause regressions. Layers without bounds overlap with boundaries therefore we might end up outside the current tracking volume while targeting a layer. I propose that this should be rather fixed by the used with proper layer bounds that extend to the volume boundaries.

@andiwand andiwand added this to the next milestone Dec 21, 2023
@github-actions github-actions bot added the Component - Core Affects the Core module label Dec 21, 2023
Copy link

codecov bot commented Dec 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (73553d3) 48.87% compared to head (bff10ef) 48.87%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2851   +/-   ##
=======================================
  Coverage   48.87%   48.87%           
=======================================
  Files         485      485           
  Lines       28206    28205    -1     
  Branches    13287    13286    -1     
=======================================
  Hits        13786    13786           
  Misses       4829     4829           
+ Partials     9591     9590    -1     

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

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.

Are we sensitive to navigation failures in these tests? Would an increase in navigation failures show up anywhere?

@andiwand
Copy link
Contributor Author

I think we do not use this option anywhere on our side

@kodiakhq kodiakhq bot merged commit c51f48a into acts-project:main Dec 21, 2023
55 checks passed
@andiwand andiwand deleted the remove-navigator-layer-bounds-option branch December 21, 2023 21:50
@acts-project-service
Copy link
Collaborator

✅ Athena integration test results [c51f48a]

✅ All tests successful

status job report
🟢 run_unit_tests
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsBenchmarkWithSpot.sh 8 100
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsWorkflow.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateAmbiguityResolution.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateResolvedTracks.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateTracks.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateActsCoreSpacePoints.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateActsSpacePoints.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateSeeds.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateOrthogonalSeeds.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateClusters.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsPersistifyEDM.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsGSFRefitting.sh
🟢 run_ci_tests: ../athena/AtlasTest/CITest/test/ActsKfRefitting.sh
🟢 run_ci_tests: python3 ../athena/Tracking/Acts/ActsGeometry/test/ActsExtrapolationAlgTest.py
🟢 run_ci_tests: python3 ../athena/Tracking/Acts/ActsGeometry/test/ActsITkTest.py
🟢 run_workflow_tests_run4_mc
🟢 run_workflow_tests_run2_mc
🟢 run_workflow_tests_run2_data
🟢 run_workflow_tests_run3_mc
🟢 run_workflow_tests_run3_data
🟢 run_art_test: test_data18_13TeV_1000evt
🟢 run_art_test: test_ttbarPU40_reco

@paulgessinger
Copy link
Member

Athena CI looks clean after reference update

paulgessinger added a commit to paulgessinger/acts that referenced this pull request Dec 30, 2023
@paulgessinger paulgessinger modified the milestones: next, v32.0.0 Jan 19, 2024
LaraCalic pushed a commit to LaraCalic/acts that referenced this pull request Feb 10, 2024
)

This option is supposed to improve the navigation but can also cause regressions. Layers without bounds overlap with boundaries therefore we might end up outside the current tracking volume while targeting a layer. I propose that this should be rather fixed by the used with proper layer bounds that extend to the volume boundaries.
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.

3 participants