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: Rework interactions interface - more explicit params and types #2366

Merged

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Aug 14, 2023

Refactors the interactions interface to be minimal. That means only passing absolute charge and absolute PDG and only where necessary.

At the same time I made the types explicit. For some reason we use float all across the interaction code which I kept and made more explicit.

also fixes a weird FPE discovered in

pulled these changes out of

blocked by

@andiwand andiwand added this to the next milestone Aug 14, 2023
@github-actions github-actions bot added Component - Core Affects the Core module Component - Fatras Affects the Fatras module Component - Examples Affects the Examples module labels Aug 14, 2023
@andiwand andiwand changed the title fix weird fpe refactor: Rework interactions interface - more explicit params and types Aug 14, 2023
@github-actions github-actions bot added the Component - Plugins Affects one or more Plugins label Aug 14, 2023
@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Merging #2366 (1f4d612) into main (cb8362c) will increase coverage by 0.01%.
The diff coverage is 74.65%.

@@            Coverage Diff             @@
##             main    #2366      +/-   ##
==========================================
+ Coverage   49.58%   49.60%   +0.01%     
==========================================
  Files         453      452       -1     
  Lines       25517    25515       -2     
  Branches    11706    11701       -5     
==========================================
+ Hits        12653    12656       +3     
+ Misses       4581     4579       -2     
+ Partials     8283     8280       -3     
Files Changed Coverage Δ
Core/include/Acts/Propagator/AtlasStepper.hpp 72.19% <ø> (ø)
...ore/include/Acts/Propagator/MaterialInteractor.hpp 39.72% <0.00%> (ø)
Core/include/Acts/Propagator/Propagator.hpp 93.33% <ø> (ø)
...agator/detail/GenericDenseEnvironmentExtension.hpp 52.70% <37.50%> (ø)
Core/src/Material/Interactions.cpp 77.94% <76.15%> (+3.75%) ⬆️
...Propagator/detail/PointwiseMaterialInteraction.hpp 97.14% <100.00%> (ø)
...ts/Propagator/detail/VolumeMaterialInteraction.hpp 90.47% <100.00%> (ø)
...Propagator/detail/PointwiseMaterialInteraction.cpp 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@andiwand andiwand marked this pull request as ready for review August 15, 2023 12:56
@andiwand
Copy link
Contributor Author

AMVF physmon is failing for unknown reasons. I experienced this already multiple times - minimal changes not directly related to the AMVF will be amplified greatly.

I will try a few things here to nail this down #2370

My guess is that either there is some weird interference because we run the IVF in the same chain or the AMVF is operating on a critical point in our setup which causes vertices to appear/disappear based on small numerical differences which then show up as bigger differences in the hist cmp

benjaminhuth
benjaminhuth previously approved these changes Aug 15, 2023
Copy link
Member

@benjaminhuth benjaminhuth left a comment

Choose a reason for hiding this comment

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

Looks good!

Core/include/Acts/Material/Interactions.hpp Outdated Show resolved Hide resolved
@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... Changes Performance labels Aug 15, 2023
@andiwand andiwand added the 🛑 blocked This item is blocked by another item label Aug 15, 2023
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.

You remove some assertions. Are you removing them because we're confident they should never trigger?

Core/src/Material/Interactions.cpp Outdated Show resolved Hide resolved
Core/src/Material/Interactions.cpp Outdated Show resolved Hide resolved
Core/src/Material/Interactions.cpp Show resolved Hide resolved
@andiwand
Copy link
Contributor Author

You remove some assertions. Are you removing them because we're confident they should never trigger?

I moved the assertions into the constructor of RelativisticQuantities so it should result in the same behavior

@andiwand andiwand removed the 🛑 blocked This item is blocked by another item label Aug 17, 2023
paulgessinger
paulgessinger previously approved these changes Aug 17, 2023
@andiwand
Copy link
Contributor Author

I am not sure where the remaining reference changes are coming from but I will try to pin it down

kodiakhq bot pushed a commit that referenced this pull request Aug 21, 2023
…ma` (#2372)

Pulled out of #2366 to pin down the hash and physmon diff

blocked by
- #2374
@kodiakhq kodiakhq bot merged commit b41968c into acts-project:main Aug 21, 2023
54 checks passed
@acts-project-service
Copy link
Collaborator

🔴 Athena integration test results

Build job with this PR failed!

Please investigate the build job for the pipeline!

@acts-project-service acts-project-service added the Breaks Athena build This PR breaks the Athena build label Aug 21, 2023
@andiwand andiwand deleted the refactor-interactions-interface-and-types branch August 22, 2023 05:04
@paulgessinger paulgessinger modified the milestones: next, v29.0.0 Aug 22, 2023
kodiakhq bot pushed a commit that referenced this pull request Sep 18, 2023
In this PR I make the particle hypothesis part of the track parameters. This allows us to consistently use the same hypothesis across various parts of our code. Before that the particle hypothesis was a bit scattered in the propagator and stepper.

replaces #2181 after splitting it up

blocked by
- #2366
- #2396
- #2398
- #2397
andiwand added a commit that referenced this pull request Sep 22, 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
- #2366
- #2368

---------

Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Co-authored-by: Benjamin Huth <[email protected]>
Co-authored-by: Benjamin Huth <[email protected]>
Co-authored-by: Paul Gessinger <[email protected]>
AJPfleger pushed a commit to AJPfleger/acts that referenced this pull request Sep 29, 2023
In this PR I make the particle hypothesis part of the track parameters. This allows us to consistently use the same hypothesis across various parts of our code. Before that the particle hypothesis was a bit scattered in the propagator and stepper.

replaces acts-project#2181 after splitting it up

blocked by
- acts-project#2366
- acts-project#2396
- acts-project#2398
- acts-project#2397
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 <[email protected]>
Co-authored-by: Benjamin Huth <[email protected]>
Co-authored-by: Paul Gessinger <[email protected]>
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 Component - Plugins Affects one or more Plugins Event Data Model
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants