fix: use correct path length derivatives when computing the curvilinear covariance for a zero step propagation #2910

goetzgaycken · 2024-01-30T12:23:28Z

The propagate can use curvilinear parametrisation for the returned parameters and covariance, However if the propagation step size is below the limit, the propagator does not compute the path length derivatives which leads to an incorrect covariance matrix when enabling curvilinear parameterization.

paulgessinger · 2024-01-30T12:24:56Z

Thanks @goetzgaycken! This would have to be rebased on top of main itself, instead of v31.2.0, which should also resolve the conflict, I believe.

paulgessinger · 2024-01-30T12:26:41Z

@beomki-yeo and @andiwand could you maybe have a look at this?

Core/include/Acts/Propagator/Propagator.ipp

Tests/UnitTests/Core/Propagator/BoundToCurvilinearConversionTests.cpp

andiwand

for my understanding: is the missing term from the covariance depending on the magnetic field? because I would have thought the contribution from the direction should be 0 since we always arrive perpendicular on the curvilinear surface

Core/include/Acts/Propagator/Propagator.ipp

Tests/UnitTests/Core/Propagator/BoundToCurvilinearConversionTests.cpp

goetzgaycken · 2024-01-30T14:31:42Z

for my understanding: is the missing term from the covariance depending on the magnetic field?

It depends on both the direction but also the cross product of the magnetic field and the time components on the propagation velocity. Even without a magnetic field a small correction results from the path length derivatives.

goetzgaycken · 2024-01-31T09:41:32Z

changes: f9938dd..8b49619 :

implemented @andiwand 's comment and moved the derivative computation to the stepper.
the unit test now compares the curvilinear covariance matrices of the explicit computation with small step propagation using eignstepper, and straightline stepper, where for the latter the magnetic field of the test data is forced to zero.

Propagation using the AtlasStepper and /MultiEigenStepperLoop have not been tested and/or fixed.

github-actions · 2024-01-31T12:27:01Z

📊: Physics performance monitoring for `be0ac1d`

Full contents

physmon summary

codecov · 2024-01-31T12:51:57Z

Codecov Report

Attention: Patch coverage is 41.93548% with 18 lines in your changes are missing coverage. Please review.

Project coverage is 48.82%. Comparing base (c3eea1c) to head (be0ac1d).

Files	Patch %	Lines
Core/include/Acts/Propagator/EigenStepper.ipp	29.41%	1 Missing and 11 partials ⚠️
.../include/Acts/Propagator/MultiEigenStepperLoop.hpp	0.00%	2 Missing ⚠️
Core/include/Acts/Propagator/Propagator.ipp	0.00%	1 Missing and 1 partial ⚠️
...re/include/Acts/Propagator/StraightLineStepper.hpp	80.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2910      +/-   ##
==========================================
- Coverage   48.82%   48.82%   -0.01%     
==========================================
  Files         489      489              
  Lines       28860    28891      +31     
  Branches    13695    13711      +16     
==========================================
+ Hits        14092    14105      +13     
- Misses       4953     4957       +4     
- Partials     9815     9829      +14

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

Core/include/Acts/Propagator/Propagator.ipp

beomki-yeo · 2024-02-13T16:11:08Z

The PR looks good to me as long as the comments from others are resolved!

andiwand

I left a few more comments and suggestions.

Generally I feel like there should be an easier solution to this but I also don't have a concrete idea for that so no objection from my side to merge this as it is since it fixes a problem.

Tests/UnitTests/Core/Propagator/BoundToCurvilinearConversionTests.cpp

Core/include/Acts/Propagator/EigenStepper.ipp

Core/include/Acts/Propagator/EigenStepper.hpp

Core/include/Acts/Propagator/AtlasStepper.hpp

Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp

andiwand · 2024-03-01T08:50:38Z

@goetzgaycken there is a clang-tidy failure https://github.com/acts-project/acts/pull/2910/checks?check_run_id=21308679491 can you fix that?

goetzgaycken · 2024-03-04T12:03:00Z

update from 5ec58b2 to 97a96a8

rebase to main of 2024-03-04 12:29
fixed :

@goetzgaycken there is a clang-tidy failure https://github.com/acts-project/acts/pull/2910/checks?check_run_id=21308679491 can you fix that?

…opagation.

The test shows how to compute the Jacobian for the bound to curvilinear conversion and compares the results from a propagation with various step sizes.

goetzgaycken · 2024-03-05T15:07:05Z

There seems to be a problem in "CI Bridge / build_linux_ubuntu". Though I do not see a particular what goes wrong exactly. Is this just an infrastructure problem ?

paulgessinger · 2024-03-05T15:23:14Z

I think this is an OOM. I'm retrying.

acts-project-service · 2024-03-20T12:39:27Z

🔴 Athena integration test results [`8329f20`]

Build job with this PR failed!

Please investigate the build job for the pipeline!

…ar covariance for a zero step propagation (acts-project#2910) The propagate can use curvilinear parametrisation for the returned parameters and covariance, However if the propagation step size is below the limit, the propagator does not compute the path length derivatives which leads to an incorrect covariance matrix when enabling curvilinear parameterization.

github-actions bot added the Component - Core Affects the Core module label Jan 30, 2024

goetzgaycken changed the title ~~fix: use correct path length derivatives when computing curvilinear coviance for a zero step propagation~~ fix: use correct path length derivatives when computing the curvilinear covariance for a zero step propagation Jan 30, 2024

goetzgaycken force-pushed the fix_curvilinear_cov_for_zero_step_propagation branch 2 times, most recently from 560d9e7 to f9938dd Compare January 30, 2024 13:06

goetzgaycken commented Jan 30, 2024

View reviewed changes

Core/include/Acts/Propagator/Propagator.ipp Outdated Show resolved Hide resolved

goetzgaycken commented Jan 30, 2024

View reviewed changes

Core/include/Acts/Propagator/Propagator.ipp Outdated Show resolved Hide resolved

goetzgaycken commented Jan 30, 2024

View reviewed changes

Tests/UnitTests/Core/Propagator/BoundToCurvilinearConversionTests.cpp Show resolved Hide resolved

andiwand reviewed Jan 30, 2024

View reviewed changes

Core/include/Acts/Propagator/Propagator.ipp Outdated Show resolved Hide resolved

Core/include/Acts/Propagator/Propagator.ipp Outdated Show resolved Hide resolved

Tests/UnitTests/Core/Propagator/BoundToCurvilinearConversionTests.cpp Outdated Show resolved Hide resolved

goetzgaycken force-pushed the fix_curvilinear_cov_for_zero_step_propagation branch from f9938dd to 8b49619 Compare January 31, 2024 09:32

goetzgaycken force-pushed the fix_curvilinear_cov_for_zero_step_propagation branch 4 times, most recently from ae55147 to 62b007c Compare January 31, 2024 12:23

goetzgaycken force-pushed the fix_curvilinear_cov_for_zero_step_propagation branch from 62b007c to 5ec58b2 Compare January 31, 2024 17:39

andiwand reviewed Feb 1, 2024

View reviewed changes

Core/include/Acts/Propagator/Propagator.ipp Show resolved Hide resolved

AJPfleger marked this pull request as ready for review February 6, 2024 16:44

AJPfleger requested review from asalzburger and beomki-yeo February 6, 2024 16:44

goetzgaycken force-pushed the fix_curvilinear_cov_for_zero_step_propagation branch from 5ec58b2 to e53fd71 Compare February 7, 2024 08:22

andiwand previously approved these changes Feb 14, 2024

View reviewed changes

paulgessinger reviewed Feb 19, 2024

View reviewed changes

Core/include/Acts/Propagator/EigenStepper.hpp Show resolved Hide resolved

Core/include/Acts/Propagator/AtlasStepper.hpp Show resolved Hide resolved

Core/include/Acts/Propagator/MultiEigenStepperLoop.hpp Show resolved Hide resolved

andiwand added this to the next milestone Mar 1, 2024

goetzgaycken dismissed andiwand’s stale review via 97a96a8 March 4, 2024 11:58

goetzgaycken force-pushed the fix_curvilinear_cov_for_zero_step_propagation branch from e53fd71 to 97a96a8 Compare March 4, 2024 11:58

Goetz Gaycken added 2 commits March 5, 2024 10:56

Compute correct curvilinear covariance matrix also for a zero step pr…

39a7daa

…opagation.

Added unit-test for bound to curvilinear conversion.

472d018

The test shows how to compute the Jacobian for the bound to curvilinear conversion and compares the results from a propagation with various step sizes.

goetzgaycken force-pushed the fix_curvilinear_cov_for_zero_step_propagation branch from 97a96a8 to 472d018 Compare March 5, 2024 09:56

paulgessinger approved these changes Mar 5, 2024

View reviewed changes

Merge branch 'main' into fix_curvilinear_cov_for_zero_step_propagation

7653c90

andiwand added the automerge label Mar 20, 2024

Merge branch 'main' into fix_curvilinear_cov_for_zero_step_propagation

be0ac1d

kodiakhq bot merged commit 8329f20 into acts-project:main Mar 20, 2024
54 checks passed

github-actions bot removed the automerge label Mar 20, 2024

acts-project-service added the Breaks Athena build This PR breaks the Athena build label Mar 20, 2024

paulgessinger modified the milestones: next, v33.1.0 Mar 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: use correct path length derivatives when computing the curvilinear covariance for a zero step propagation #2910

fix: use correct path length derivatives when computing the curvilinear covariance for a zero step propagation #2910

goetzgaycken commented Jan 30, 2024

paulgessinger commented Jan 30, 2024

paulgessinger commented Jan 30, 2024

andiwand left a comment

goetzgaycken commented Jan 30, 2024

goetzgaycken commented Jan 31, 2024

github-actions bot commented Jan 31, 2024 •

edited

Loading

codecov bot commented Jan 31, 2024 •

edited

Loading

beomki-yeo commented Feb 13, 2024

andiwand left a comment

andiwand commented Mar 1, 2024

goetzgaycken commented Mar 4, 2024 •

edited

Loading

goetzgaycken commented Mar 5, 2024

paulgessinger commented Mar 5, 2024

acts-project-service commented Mar 20, 2024 •

edited

Loading

fix: use correct path length derivatives when computing the curvilinear covariance for a zero step propagation #2910

fix: use correct path length derivatives when computing the curvilinear covariance for a zero step propagation #2910

Conversation

goetzgaycken commented Jan 30, 2024

paulgessinger commented Jan 30, 2024

paulgessinger commented Jan 30, 2024

andiwand left a comment

Choose a reason for hiding this comment

goetzgaycken commented Jan 30, 2024

goetzgaycken commented Jan 31, 2024

github-actions bot commented Jan 31, 2024 • edited Loading

📊: Physics performance monitoring for be0ac1d

physmon summary

codecov bot commented Jan 31, 2024 • edited Loading

Codecov Report

beomki-yeo commented Feb 13, 2024

andiwand left a comment

Choose a reason for hiding this comment

andiwand commented Mar 1, 2024

goetzgaycken commented Mar 4, 2024 • edited Loading

goetzgaycken commented Mar 5, 2024

paulgessinger commented Mar 5, 2024

acts-project-service commented Mar 20, 2024 • edited Loading

🔴 Athena integration test results [8329f20]

Build job with this PR failed!

github-actions bot commented Jan 31, 2024 •

edited

Loading

📊: Physics performance monitoring for `be0ac1d`

codecov bot commented Jan 31, 2024 •

edited

Loading

goetzgaycken commented Mar 4, 2024 •

edited

Loading

acts-project-service commented Mar 20, 2024 •

edited

Loading

🔴 Athena integration test results [`8329f20`]