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: Add method to reset jacobian in stepper #1171

Merged
merged 1 commit into from
Feb 21, 2022

Conversation

Corentin-Allaire
Copy link
Contributor

When trying to use the ATLAS stepper with the KF I noticed that it wouldn't work since we use to set the stepping state jacobian to a BoundMatrix::Identity() which isn't compatible with the ATLAS stepper.
This PR add a method to the stepper to set the jacobian to the identity, and thus solve this issue.
Similarly I had to transform the update to a reset for the propagation to the Perigee as it relied on manually setting some matrix afterward (which was not compatible with the ATLAS stepper).

@Corentin-Allaire Corentin-Allaire added Component - Core Affects the Core module Impact - Minor Nuissance bug and/or affects only a single module Improvement Changes to an existing feature labels Feb 18, 2022
@Corentin-Allaire Corentin-Allaire added this to the next milestone Feb 18, 2022
@Corentin-Allaire Corentin-Allaire changed the title Add method to reset jacobian in stepper fix: Add method to reset jacobian in stepper Feb 18, 2022
@codecov
Copy link

codecov bot commented Feb 18, 2022

Codecov Report

Merging #1171 (f2b0f9d) into main (9294d61) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1171      +/-   ##
==========================================
- Coverage   47.89%   47.89%   -0.01%     
==========================================
  Files         359      359              
  Lines       18503    18502       -1     
  Branches     8732     8730       -2     
==========================================
- Hits         8862     8861       -1     
- Misses       3603     3605       +2     
+ Partials     6038     6036       -2     
Impacted Files Coverage Δ
Core/include/Acts/Propagator/AtlasStepper.hpp 68.55% <ø> (ø)
Core/include/Acts/Propagator/EigenStepper.hpp 69.23% <ø> (ø)
Core/include/Acts/Propagator/EigenStepper.ipp 49.24% <0.00%> (-0.76%) ⬇️
Core/include/Acts/TrackFitting/KalmanFitter.hpp 37.94% <0.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9294d61...f2b0f9d. Read the comment docs.

@paulgessinger
Copy link
Member

This looks good to me generally. I'm very curious to see if the CI reports changed outputs on this.

@paulgessinger
Copy link
Member

CI reports no output changes (as expected I guess). So let's go.

@paulgessinger paulgessinger changed the title fix: Add method to reset jacobian in stepper refactor: Add method to reset jacobian in stepper Feb 21, 2022
@kodiakhq kodiakhq bot merged commit f9dbc02 into acts-project:main Feb 21, 2022
@paulgessinger paulgessinger modified the milestones: next, v17.1.0 Mar 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
automerge Component - Core Affects the Core module Impact - Minor Nuissance bug and/or affects only a single module Improvement Changes to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants