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!: Disambiguate surface intersection solutions #2336

Merged

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Aug 1, 2023

Currently our intersection call can give an alternative solution but it is not clear which one is to be preferred in which situation and the results are unstable meaning that it could be swapped depending on where you are on the ray.

Here I try to make the interface cleaner and the intersection order stable.

blocked by

@github-actions github-actions bot added the Component - Core Affects the Core module label Aug 1, 2023
@andiwand andiwand changed the title refactor: Disambiguate intersection refactor!: Disambiguate intersection Aug 2, 2023
@codecov
Copy link

codecov bot commented Aug 2, 2023

Codecov Report

Merging #2336 (0ca1051) into main (7f8d143) will increase coverage by 0.06%.
The diff coverage is 47.34%.

@@            Coverage Diff             @@
##             main    #2336      +/-   ##
==========================================
+ Coverage   49.81%   49.88%   +0.06%     
==========================================
  Files         466      466              
  Lines       26190    26232      +42     
  Branches    12009    12014       +5     
==========================================
+ Hits        13047    13086      +39     
+ Misses       4620     4614       -6     
- Partials     8523     8532       +9     
Files Changed Coverage Δ
Core/include/Acts/Geometry/ApproachDescriptor.hpp 100.00% <ø> (ø)
...nclude/Acts/Geometry/GenericApproachDescriptor.hpp 85.71% <ø> (ø)
Core/include/Acts/Geometry/TrackingVolume.hpp 70.73% <ø> (ø)
...include/Acts/Navigation/NavigationStateFillers.hpp 85.71% <ø> (ø)
.../include/Acts/Propagator/MultiEigenStepperLoop.ipp 38.28% <0.00%> (ø)
Core/include/Acts/Surfaces/ConeSurface.hpp 100.00% <ø> (ø)
Core/include/Acts/Surfaces/CylinderSurface.hpp 100.00% <ø> (ø)
Core/include/Acts/Surfaces/DiscSurface.hpp 100.00% <ø> (ø)
Core/include/Acts/Surfaces/LineSurface.hpp 100.00% <ø> (ø)
Core/include/Acts/Surfaces/PlaneSurface.hpp 100.00% <ø> (ø)
... and 29 more

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

@andiwand andiwand added this to the v29.0.0 milestone Aug 3, 2023
@andiwand andiwand changed the title refactor!: Disambiguate intersection refactor!: Disambiguate surface intersection solutions Aug 14, 2023
@andiwand
Copy link
Contributor Author

this is causing navigation failure in the full_chain_odd.py which was not the case in a previous version I believe. I am investigating

@andiwand
Copy link
Contributor Author

this seems to work now - even tho I am not super happy with the fix but I don't see an alternative

Copy link
Contributor Author

@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.

these are the new changes in question

@paulgessinger
Copy link
Member

Is this why the physmon still shows differences?

@andiwand
Copy link
Contributor Author

Is this why the physmon still shows differences?

I just didn't update the reference yet because I was unsure if this one or #2470 should go in first

@github-actions github-actions bot added the Infrastructure Changes to build tools, continous integration, ... label Sep 22, 2023
@andiwand andiwand merged commit 45e07d7 into acts-project:main Sep 22, 2023
58 checks passed
@andiwand andiwand deleted the refactor-disambiguate-intersection branch September 22, 2023 20:11
@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Sep 22, 2023
@paulgessinger
Copy link
Member

This does break compilation in the material track writer in Athena.

kodiakhq bot pushed a commit that referenced this pull request Sep 25, 2023
reopened #2318 after #2321

The CKF did not pick up the material in-between the first surface and the target surface. This PR is trying to fix that.

Note: I had to use a not so nice hack and call the navigation pre step routine before we step towards the target. I don't see an easy way around that.

blocked by
- #2336
AJPfleger pushed a commit to AJPfleger/acts that referenced this pull request Sep 29, 2023
…2336)

Currently our intersection call can give an alternative solution but it
is not clear which one is to be preferred in which situation and the
results are unstable meaning that it could be swapped depending on where
you are on the ray.

Here I try to make the interface cleaner and the intersection order
stable.

blocked by
- acts-project#2366
- acts-project#2368

---------

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Co-authored-by: Benjamin Huth <37871400+benjaminhuth@users.noreply.github.com>
Co-authored-by: Benjamin Huth <benjamin.huth@physik.uni-regensburg.de>
Co-authored-by: Paul Gessinger <hello@paulgessinger.com>
AJPfleger pushed a commit to AJPfleger/acts that referenced this pull request Sep 29, 2023
reopened acts-project#2318 after acts-project#2321

The CKF did not pick up the material in-between the first surface and the target surface. This PR is trying to fix that.

Note: I had to use a not so nice hack and call the navigation pre step routine before we step towards the target. I don't see an easy way around that.

blocked by
- acts-project#2336
kodiakhq bot pushed a commit that referenced this pull request Nov 2, 2023
After refactoring the surface intersection #2336 I missed to update this function of the stepper

discovered in #2595 and #2596
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaks Athena build This PR breaks the Athena build Changes Performance Component - Core Affects the Core module Component - Examples Affects the Examples module Component - Fatras Affects the Fatras module Event Data Model Infrastructure Changes to build tools, continous integration, ... Track Finding Track Fitting Vertexing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants