fix: update jacToGlobal after KF filtering

[XiaocongAi]

Currently, the stepper only update the cov and pars in the update invocation. However, the updated pars means the jacToGlobal should be updated as well, in particular the pars has underwent some big change after the KF filtering.

So this PR fixes the issue. It adds the update of the jacToGlobal in the stepper update method.

[XiaocongAi]

The pull of perigee parameters for telescope detector with DiscSurface (eta=2.0, B = (0,0,2) T) are shown below:

• WITHOUT this fix:
  
• WITH this fix:
  

We can see that this can successfully fix the biased pull for d0.

[codecov]

Codecov Report

Merging #935 (08055c4) into main (cb728a4) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #935   +/-   ##
=======================================
  Coverage   48.75%   48.75%           
=======================================
  Files         331      331           
  Lines       17101    17103    +2     
  Branches     8070     8070           
=======================================
+ Hits         8337     8339    +2     
  Misses       3057     3057           
  Partials     5707     5707           

Impacted Files	Coverage Δ
Core/include/Acts/Propagator/AtlasStepper.hpp	68.55% <ø> (ø)
Core/include/Acts/Propagator/EigenStepper.hpp	67.56% <ø> (ø)
...re/include/Acts/Propagator/StraightLineStepper.hpp	67.27% <ø> (ø)
Core/include/Acts/Surfaces/Surface.hpp	60.00% <ø> (ø)
...de/Acts/TrackFinding/CombinatorialKalmanFilter.hpp	30.16% <ø> (ø)
Core/include/Acts/TrackFitting/KalmanFitter.hpp	38.06% <ø> (ø)
Core/include/Acts/Propagator/EigenStepper.ipp	50.00% <100.00%> (+0.38%)	⬆️
Core/src/Propagator/StraightLineStepper.cpp	69.44% <100.00%> (+0.87%)	⬆️

---

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 cb728a4...08055c4. Read the comment docs.

[FabianKlimpel]

This solves the problem pretty well. I just have two rather general comments:

1. I built the covariance transport such that you calculate the 'jacToGlobal' as soon as the transport is performed. The intention was to always have the 'jacToGlobal' present and nobody has to care about this. However, as you figured (correctly) out that this is not correct if the parameters change, we might want to disable this transport.

2. The parameters you hand over to the 'update()' are a bit duplicated. Would it make a difference if you would a) just pass the bound parameters and the surface but no free parameters or b) pass the free parameters and the jacobian?

I'm fine with the PR as it is but 1) should be considered as a follow-up and 2) is just up to your choice. I will approve this in case you want to follow 1) up in a separate PR and keep the current interface.

[asalzburger]

This looks fine & should go in.

[XiaocongAi]

This solves the problem pretty well. I just have two rather general comments:

1. I built the covariance transport such that you calculate the 'jacToGlobal' as soon as the transport is performed. The intention was to always have the 'jacToGlobal' present and nobody has to care about this. However, as you figured (correctly) out that this is not correct if the parameters change, we might want to disable this transport.
2. The parameters you hand over to the 'update()' are a bit duplicated. Would it make a difference if you would a) just pass the bound parameters and the surface but no free parameters or b) pass the free parameters and the jacobian?

I'm fine with the PR as it is but 1) should be considered as a follow-up and 2) is just up to your choice. I will approve this in case you want to follow 1) up in a separate PR and keep the current interface.

Thank you for your suggestions.
For 1., sorry I'm a bit confused. What do you mean by "disable this transport"?
For 2., there is indeed some duplication for the input parameters, but it's done in this way just to avoid extra cost of redoing the transform from global to local or local to global. There are a few places in acts where we redo the transform. So a general solution might be needed to avoid this.
Yes, I will open a follow-up PR for 1. to resolve the possible remaing issue.

[FabianKlimpel]

Thank you for your suggestions.
For 1., sorry I'm a bit confused. What do you mean by "disable this transport"?

The 'jacToGlobal' is calculated twice in case of an 'update()' call. The first time when you transport the covariance matrix to the current surface and then a second time after the update. The first calculation is not necessary if you know that you will update the parameters, so you could add e.g. a flag that disables the 'jacToGlobal' calculation when transporting the covariance matrix.

For 2., there is indeed some duplication for the input parameters, but it's done in this way just to avoid extra cost of redoing the transform from global to local or local to global. There are a few places in acts where we redo the transform. So a general solution might be needed to avoid this.

This is just a style-related comment. If that appears more often then a refactoring might be necessary, indeed.

[XiaocongAi]

The 'jacToGlobal' is calculated twice in case of an 'update()' call. The first time when you transport the covariance matrix to the current surface and then a second time after the update. The first calculation is not necessary if you know that you will update the parameters, so you could add e.g. a flag that disables the 'jacToGlobal' calculation when transporting the covariance matrix.

Thank you for the clarification. That's a good point. I will open another PR for this.

[asalzburger]

We had another (C)KF related PR going in, so I will update this branch first - but merge when gone through.

[asalzburger]

Re-approving.