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

test: Check if we end up in the expected volume in Gen1 Navigator #3442

Merged
merged 15 commits into from
Sep 1, 2024

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Jul 25, 2024

I think this is one of the big navigation pitfalls. If we end up in the wrong volume the navigation is practically lost. This is too expensive to check in production so I opted for an assert.

Not sure if that is a great solution since users might try to measure performance with non release build by accident. But that was the only way I could think of.

blocked by

@andiwand andiwand added this to the next milestone Jul 25, 2024
@github-actions github-actions bot added the Component - Core Affects the Core module label Jul 25, 2024
Copy link

github-actions bot commented Jul 25, 2024

📊: Physics performance monitoring for f1e66d1

Full contents

physmon summary

@paulgessinger
Copy link
Member

paulgessinger commented Jul 25, 2024

I really like the idea to have this (conditionally) be a hard failure.

One alternative to the assert would be an explicit configuration that sets up a hardened navigation mode where this check is performed.

On another note: we force-enable assert even on non-Debug builds in the CI so that we gain exposure to assertion failures. So, depending on the slowdown, this might be an issue.

@andiwand
Copy link
Contributor Author

Problem with the GSF and GX2F can be observed here @benjaminhuth @AJPfleger

@andiwand andiwand added the 🚧 WIP Work-in-progress label Jul 25, 2024
@andiwand
Copy link
Contributor Author

I really like the idea to have this (conditionally) be a hard failure.

One alternative to the assert would be an explicit configuration that sets up a hardened navigation mode where this check is performed.

On another note: we force-enable assert even on non-Debug builds in the CI so that we gain exposure to assertion failures. So, depending on the slowdown, this might be an issue.

I did not really think of the CI. We have to see how bad it is. But I would trade some performance for this because otherwise we might just not end up checking this at all

@github-actions github-actions bot added the Infrastructure Changes to build tools, continous integration, ... label Jul 25, 2024
@andiwand
Copy link
Contributor Author

physmon ttbar changes because we now cut on different particles

Copy link

sonarcloud bot commented Jul 26, 2024

@AJPfleger
Copy link
Contributor

Problem with the GSF and GX2F can be observed here @benjaminhuth @AJPfleger

I might have fixed the gx2f issue with

ready to update? :)

@andiwand
Copy link
Contributor Author

andiwand commented Aug 6, 2024

@AJPfleger it is now failing inside the unit test

13:43:23    Navigator      VERBOSE   No Volume | Initialization.
13:43:23    Navigator      VERBOSE   No Volume | Slow start initialization through search.
13:43:23    Navigator      VERBOSE   No Volume | Starting from position (750.0221, -749.9779, -0.1743) and direction (-0.2816, 0.9595, 0.0001)
13:43:23    Navigator      VERBOSE   TestVolume | Start volume resolved.
ActsUnitTestGx2f: /__w/acts/acts/Core/include/Acts/Propagator/Navigator.hpp:325: void Acts::Navigator::initialize(propagator_state_t&, const stepper_t&) const [with propagator_state_t = Acts::PropagatorState<Acts::PropagatorOptions<Acts::EigenStepper<>::Options, Options, Acts::ActionList<Acts::Experimental::Gx2Fitter<const Acts::Propagator<Acts::EigenStepper<>, Acts::Navigator>, Acts::VectorMultiTrajectory>::Actor<Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis> > >, Acts::AbortList<Acts::Experimental::Gx2Fitter<const Acts::Propagator<Acts::EigenStepper<>, Acts::Navigator>, Acts::VectorMultiTrajectory>::Aborter<Acts::GenericBoundTrackParameters<Acts::ParticleHypothesis> >, Acts::PathLimitReached> >, Acts::EigenStepper<>::State, State, Acts::Experimental::Gx2FitterResult<Acts::VectorMultiTrajectory> >; stepper_t = Acts::EigenStepper<>]: Assertion `state.navigation.startVolume->inside( stepper.position(state.stepping), state.options.surfaceTolerance) && "We did not end up inside the volume."' failed.
unknown location(0): fatal error: in "Gx2fTest/UpdatePushedToNewVolume": signal: SIGABRT (application abort requested)

@andiwand andiwand added the 🛑 blocked This item is blocked by another item label Aug 20, 2024
@andiwand
Copy link
Contributor Author

This is not blocked by #3481. lowestTrackingVolume determines a volume for a position we are not in which then causes the assert to fire.

@andiwand andiwand removed the 🚧 WIP Work-in-progress label Aug 30, 2024
@andiwand andiwand marked this pull request as ready for review August 31, 2024 22:37
@andiwand andiwand removed the 🛑 blocked This item is blocked by another item label Sep 1, 2024
Copy link
Contributor

@AJPfleger AJPfleger 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 in general :)

@kodiakhq kodiakhq bot merged commit b656c03 into acts-project:main Sep 1, 2024
43 checks passed
@github-actions github-actions bot removed the automerge label Sep 1, 2024
Copy link

sonarcloud bot commented Sep 1, 2024

@acts-project-service acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Sep 1, 2024
@andiwand andiwand deleted the navigator-check-inside-volume branch September 2, 2024 05:34
@andiwand andiwand modified the milestones: next, v36.3.0 Sep 2, 2024
paulgessinger added a commit that referenced this pull request Sep 19, 2024
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 Fails Athena tests This PR causes a failure in the Athena tests Infrastructure Changes to build tools, continous integration, ...
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants