fix: update jacToGlobal after KF filtering (#935)

* update jacToGlobal after KF filtering

* Update RecTruthTracks.cpp

Revert temporary change.

Co-authored-by: Andreas Salzburger <Andreas.Salzburger@cern.ch>

Core/include/Acts/Propagator/AtlasStepper.hpp

@@ -327,7 +327,7 @@ class AtlasStepper {
     update(state,
            detail::transformBoundToFreeParameters(surface, state.geoContext,
                                                   boundParams),
-           cov);
+           boundParams, cov, surface);
     state.navDir = navDir;
     state.stepSize = ConstrainedStep(stepSize);
     state.pathAccumulated = 0.;
@@ -654,10 +654,11 @@ class AtlasStepper {
   /// The state update method
   ///
   /// @param [in,out] state The stepper state for
-  /// @param [in] parameters The new track parameters at start
+  /// @param [in] parameters The new free track parameters at start
   /// @param [in] covariance The updated covariance matrix
   void update(State& state, const FreeVector& parameters,
-              const Covariance& covariance) const {
+              const BoundVector& /*unused*/, const Covariance& covariance,
+              const Surface& /*unused*/) const {
     Vector3 direction = parameters.template segment<3>(eFreeDir0).normalized();
     state.pVector[0] = parameters[eFreePos0];
     state.pVector[1] = parameters[eFreePos1];

Core/include/Acts/Propagator/EigenStepper.hpp

@@ -327,10 +327,13 @@ class EigenStepper {
   /// Method to update a stepper state to the some parameters
   ///
   /// @param [in,out] state State object that will be updated
-  /// @param [in] parameters Parameters that will be written into @p state
+  /// @param [in] freeParams Free parameters that will be written into @p state
+  /// @param [in] boundParams Corresponding bound parameters used to update jacToGlobal in @p state
   /// @param [in] covariance The covariance that will be written into @p state
-  void update(State& state, const FreeVector& parameters,
-              const Covariance& covariance) const;
+  /// @param [in] surface The surface used to update the jacToGlobal
+  void update(State& state, const FreeVector& freeParams,
+              const BoundVector& boundParams, const Covariance& covariance,
+              const Surface& surface) const;
 
   /// Method to update momentum, direction and p
   ///

Core/include/Acts/Propagator/EigenStepper.ipp

@@ -35,7 +35,7 @@ void Acts::EigenStepper<E, A>::resetState(State& state,
   update(state,
          detail::transformBoundToFreeParameters(surface, state.geoContext,
                                                 boundParams),
-         cov);
+         boundParams, cov, surface);
   state.navDir = navDir;
   state.stepSize = ConstrainedStep(stepSize);
   state.pathAccumulated = 0.;
@@ -70,10 +70,14 @@ auto Acts::EigenStepper<E, A>::curvilinearState(State& state,
 
 template <typename E, typename A>
 void Acts::EigenStepper<E, A>::update(State& state,
-                                      const FreeVector& parameters,
-                                      const Covariance& covariance) const {
-  state.pars = parameters;
+                                      const FreeVector& freeParams,
+                                      const BoundVector& boundParams,
+                                      const Covariance& covariance,
+                                      const Surface& surface) const {
+  state.pars = freeParams;
   state.cov = covariance;
+  state.jacToGlobal =
+      surface.boundToFreeJacobian(state.geoContext, boundParams);
 }
 
 template <typename E, typename A>

Core/include/Acts/Propagator/StepperConcept.hpp

@@ -110,7 +110,7 @@ using step_size_t = decltype(std::declval<T>().stepSize);
         static_assert(bound_state_method_exists, "boundState method not found");
         constexpr static bool curvilinear_state_method_exists = has_method<const S, typename S::CurvilinearState, curvilinear_state_method_t, state&, bool>;
         static_assert(curvilinear_state_method_exists, "curvilinearState method not found");
-        constexpr static bool update_method_exists = require<has_method<const S, void, update_t, state&, const FreeVector&, const BoundSymMatrix&>,
+        constexpr static bool update_method_exists = require<has_method<const S, void, update_t, state&, const FreeVector&, const BoundVector&, const BoundSymMatrix&, const Surface&>,
                                                              has_method<const S, void, update_t, state&, const Vector3&, const Vector3&, double, double>>;
         static_assert(update_method_exists, "update method not found");
         constexpr static bool covariance_transport_exists = require<has_method<const S, void, covariance_transport_curvilinear_t, state&>,
@@ -155,4 +155,4 @@ constexpr bool StepperConcept =
 template <typename stepper>
 constexpr bool StepperStateConcept =
     Acts::Concepts ::Stepper::StepperStateConcept<stepper>;
-}  // namespace Acts
+}  // namespace Acts

Core/include/Acts/Propagator/StraightLineStepper.hpp

@@ -297,10 +297,13 @@ class StraightLineStepper {
   /// Method to update a stepper state to the some parameters
   ///
   /// @param [in,out] state State object that will be updated
-  /// @param [in] parameters Parameters that will be written into @p state
+  /// @param [in] freeParams Free parameters that will be written into @p state
+  /// @param [in] boundParams Corresponding bound parameters used to update jacToGlobal in @p state
   /// @param [in] covariance Covariance that willl be written into @p state
-  void update(State& state, const FreeVector& parameters,
-              const Covariance& covariance) const;
+  /// @param [in] surface The surface used to update the jacToGlobal
+  void update(State& state, const FreeVector& freeParams,
+              const BoundVector& boundParams, const Covariance& covariance,
+              const Surface& surface) const;
 
   /// Method to update momentum, direction and p
   ///

Core/include/Acts/Surfaces/Surface.hpp

@@ -320,7 +320,7 @@ class Surface : public virtual GeometryObject,
   /// Calculate the jacobian from local to global which the surface knows best,
   /// hence the calculation is done here.
   ///
-  /// @note In priciple, the input could also be a free parameters
+  /// @note In principle, the input could also be a free parameters
   /// vector as it could be transformed to a bound parameters. But the transform
   /// might fail in case the parameters is not on surface. To avoid the check
   /// inside this function, it takes directly the bound parameters as input

Core/include/Acts/TrackFinding/CombinatorialKalmanFilter.hpp

@@ -641,7 +641,7 @@ class CombinatorialKalmanFilter {
           stepper.update(state.stepping,
                          MultiTrajectoryHelpers::freeFiltered(
                              state.options.geoContext, ts),
-                         ts.filteredCovariance());
+                         ts.filtered(), ts.filteredCovariance(), *surface);
           ACTS_VERBOSE("Stepping state is updated with filtered parameter: \n"
                        << ts.filtered().transpose()
                        << " of track state with tip = "
@@ -1047,11 +1047,15 @@ class CombinatorialKalmanFilter {
            std::abs(lastIntersection.intersection.pathLength));
       if (closerToFirstCreatedMeasurement) {
         stepper.update(state.stepping, firstParams,
-                       firstCreatedMeasurement.smoothedCovariance());
+                       firstCreatedMeasurement.smoothed(),
+                       firstCreatedMeasurement.smoothedCovariance(),
+                       firstCreatedMeasurement.referenceSurface());
         reverseDirection = (firstIntersection.intersection.pathLength < 0);
       } else {
         stepper.update(state.stepping, lastParams,
-                       lastCreatedMeasurement.smoothedCovariance());
+                       lastCreatedMeasurement.smoothed(),
+                       lastCreatedMeasurement.smoothedCovariance(),
+                       lastCreatedMeasurement.referenceSurface());
         reverseDirection = (lastIntersection.intersection.pathLength < 0);
       }
       const auto& surface = closerToFirstCreatedMeasurement

Core/include/Acts/TrackFitting/KalmanFitter.hpp

@@ -547,7 +547,8 @@ class KalmanFitter {
           stepper.update(state.stepping,
                          MultiTrajectoryHelpers::freeFiltered(
                              state.options.geoContext, trackStateProxy),
-                         trackStateProxy.filteredCovariance());
+                         trackStateProxy.filtered(),
+                         trackStateProxy.filteredCovariance(), *surface);
           // We count the state with measurement
           ++result.measurementStates;
         } else {
@@ -748,7 +749,8 @@ class KalmanFitter {
           stepper.update(state.stepping,
                          MultiTrajectoryHelpers::freeFiltered(
                              state.options.geoContext, trackStateProxy),
-                         trackStateProxy.filteredCovariance());
+                         trackStateProxy.filtered(),
+                         trackStateProxy.filteredCovariance(), *surface);
 
           // Update state and stepper with post material effects
           materialInteractor(surface, state, stepper, postUpdate);
@@ -926,11 +928,15 @@ class KalmanFitter {
            std::abs(lastIntersection.intersection.pathLength));
       if (closerToFirstCreatedMeasurement) {
         stepper.update(state.stepping, firstParams,
-                       firstCreatedMeasurement.smoothedCovariance());
+                       firstCreatedMeasurement.smoothed(),
+                       firstCreatedMeasurement.smoothedCovariance(),
+                       firstCreatedMeasurement.referenceSurface());
         reverseDirection = (firstIntersection.intersection.pathLength < 0);
       } else {
         stepper.update(state.stepping, lastParams,
-                       lastCreatedMeasurement.smoothedCovariance());
+                       lastCreatedMeasurement.smoothed(),
+                       lastCreatedMeasurement.smoothedCovariance(),
+                       lastCreatedMeasurement.referenceSurface());
         reverseDirection = (lastIntersection.intersection.pathLength < 0);
       }
       const auto& surface = closerToFirstCreatedMeasurement

Core/src/Propagator/StraightLineStepper.cpp

@@ -30,10 +30,14 @@ StraightLineStepper::curvilinearState(State& state, bool transportCov) const {
       state.pathAccumulated);
 }
 
-void StraightLineStepper::update(State& state, const FreeVector& parameters,
-                                 const Covariance& covariance) const {
-  state.pars = parameters;
+void StraightLineStepper::update(State& state, const FreeVector& freeParams,
+                                 const BoundVector& boundParams,
+                                 const Covariance& covariance,
+                                 const Surface& surface) const {
+  state.pars = freeParams;
   state.cov = covariance;
+  state.jacToGlobal =
+      surface.boundToFreeJacobian(state.geoContext, boundParams);
 }
 
 void StraightLineStepper::update(State& state, const Vector3& uposition,
@@ -68,7 +72,7 @@ void StraightLineStepper::resetState(State& state,
   update(state,
          detail::transformBoundToFreeParameters(surface, state.geoContext,
                                                 boundParams),
-         cov);
+         boundParams, cov, surface);
   state.navDir = navDir;
   state.stepSize = ConstrainedStep(stepSize);
   state.pathAccumulated = 0.;

Tests/UnitTests/Core/Propagator/AtlasStepperTests.cpp

@@ -165,7 +165,8 @@ BOOST_AUTO_TEST_CASE(UpdateFromBound) {
   //         the update method not do anything when it is set. Why?
   state.state_ready = false;
   BOOST_CHECK(params.covariance().has_value());
-  stepper.update(state, freeParams, *params.covariance());
+  stepper.update(state, freeParams, params.parameters(), *params.covariance(),
+                 *plane);
   CHECK_CLOSE_ABS(stepper.position(state), newPos, eps);
   CHECK_CLOSE_ABS(stepper.time(state), newTime, eps);
   CHECK_CLOSE_ABS(stepper.direction(state), newUnitDir, eps);

Tests/UnitTests/Core/Propagator/NavigatorTests.cpp

@@ -169,8 +169,9 @@ struct PropagatorState {
       return curvState;
     }
 
-    void update(State& /*state*/, const FreeVector& /*pars*/,
-                const Covariance& /*cov*/) const {}
+    void update(State& /*state*/, const FreeVector& /*freePars*/,
+                const BoundVector& /*boundPars*/, const Covariance& /*cov*/,
+                const Surface& /*surface*/) const {}
 
     void update(State& /*state*/, const Vector3& /*uposition*/,
                 const Vector3& /*udirection*/, double /*up*/,
@@ -705,4 +706,4 @@ BOOST_AUTO_TEST_CASE(Navigator_target_methods) {
 }
 
 }  // namespace Test
-}  // namespace Acts
+}  // namespace Acts

Tests/UnitTests/Core/Propagator/StepperTests.cpp

@@ -461,7 +461,8 @@ BOOST_AUTO_TEST_CASE(eigen_stepper_test) {
   freeParams[eFreeTime] *= 2;
   freeParams[eFreeQOverP] *= -0.5;
 
-  es.update(esState, freeParams, 2 * (*bp.covariance()));
+  es.update(esState, freeParams, bp.parameters(), 2 * (*bp.covariance()),
+            *plane);
   CHECK_CLOSE_OR_SMALL(es.position(esState), 2. * pos, eps, eps);
   CHECK_CLOSE_OR_SMALL(es.direction(esState), dir, eps, eps);
   CHECK_CLOSE_REL(es.momentum(esState), 2 * absMom, eps);

Tests/UnitTests/Core/Propagator/StraightLineStepperTests.cpp

@@ -357,7 +357,8 @@ BOOST_AUTO_TEST_CASE(straight_line_stepper_test) {
   freeParams[eFreeTime] *= 2;
 
   BOOST_CHECK(bp.covariance().has_value());
-  sls.update(slsState, freeParams, 2 * (*bp.covariance()));
+  sls.update(slsState, freeParams, bp.parameters(), 2 * (*bp.covariance()),
+             *plane);
   CHECK_CLOSE_OR_SMALL(sls.position(slsState), 2. * pos, eps, eps);
   BOOST_CHECK_EQUAL(sls.charge(slsState), 1. * charge);
   CHECK_CLOSE_OR_SMALL(sls.time(slsState), 2. * time, eps, eps);