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

fix: Fix Fatras end of world #2580

Merged
merged 8 commits into from
Oct 27, 2023

Conversation

andiwand
Copy link
Contributor

Sometimes the Fatras propagation will not terminate and step with double max distance which results in FPEs

After these changes the FPEs during propagation disappeared

@andiwand andiwand added this to the next milestone Oct 25, 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 Oct 25, 2023
@codecov
Copy link

codecov bot commented Oct 25, 2023

Codecov Report

Merging #2580 (5cb0590) into main (3f7b544) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2580   +/-   ##
=======================================
  Coverage   49.63%   49.63%           
=======================================
  Files         471      471           
  Lines       26687    26687           
  Branches    12277    12277           
=======================================
  Hits        13245    13245           
  Misses       4746     4746           
  Partials     8696     8696           
Files Coverage Δ
Core/include/Acts/Propagator/Navigator.hpp 57.25% <100.00%> (ø)

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

paulgessinger
paulgessinger previously approved these changes Oct 25, 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.

So this introduces both an end of world check in the simulation actor, and also sets up and wires through the regular maximum step size and path length configuration options, correct?

@andiwand
Copy link
Contributor Author

So this introduces both an end of world check in the simulation actor, and also sets up and wires through the regular maximum step size and path length configuration options, correct?

yes exactly - the end of the world check and path limit should terminate the propagation if we are going too far and the step size limit should protect us from an unconstrained straight line step into infinity

@github-actions github-actions bot added Infrastructure Changes to build tools, continous integration, ... Changes Performance labels Oct 26, 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.

This picked up conflicts now.

@kodiakhq kodiakhq bot merged commit 825828c into acts-project:main Oct 27, 2023
55 checks passed
@andiwand andiwand deleted the fix-fatras-end-of-world branch October 27, 2023 11:16
@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Oct 27, 2023
kodiakhq bot pushed a commit that referenced this pull request Oct 28, 2023
We are currently mixing where we set masks and some of them are not accurate anymore. Here I try to reduce the masks to a minimum and set them in the code when appropriate.

blocked by:
- #2580
@paulgessinger paulgessinger modified the milestones: next, v31.0.0 Nov 6, 2023
@paulgessinger paulgessinger removed the Fails Athena tests This PR causes a failure in the Athena tests label Nov 16, 2023
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 Component - Fatras Affects the Fatras module Infrastructure Changes to build tools, continous integration, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants