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

Add derivative of track parameters to alignment parameters #125

Merged
merged 58 commits into from
Jun 11, 2020

Conversation

XiaocongAi
Copy link
Contributor

@XiaocongAi XiaocongAi commented Apr 23, 2020

This PR implements the changes as below as part of preparations for implementing a KalmanFitter-based alignment algorithm prototype (#202)

  • Extend the KalmanFitter to calculate the global covariance matrix, the covariance matrix for all parameters in the track model, in a Kalman filter track fit
  • Extend the Surface to calculate the derivative of track model (represented with bound track parameters) w.r.t. the alignment parameters of its reference surface

@XiaocongAi XiaocongAi self-assigned this Apr 23, 2020
@XiaocongAi XiaocongAi added this to the v0.23.00 milestone Apr 23, 2020
@XiaocongAi XiaocongAi added the Feature Development to integrate a new feature label Apr 23, 2020
@acts-issue-bot acts-issue-bot bot removed the Triage label Apr 23, 2020
@asalzburger asalzburger added the 🚧 WIP Work-in-progress label Apr 24, 2020
@paulgessinger paulgessinger modified the milestones: v0.23.00, v0.24.00 Apr 30, 2020
@XiaocongAi XiaocongAi added the Component - Core Affects the Core module label May 2, 2020
@XiaocongAi XiaocongAi changed the title WIP: Kf based alignment Kf based alignment May 2, 2020
@XiaocongAi XiaocongAi force-pushed the KF-based-alignment branch from d6cf7ea to d106951 Compare May 8, 2020 05:59
@codecov
Copy link

codecov bot commented May 9, 2020

Codecov Report

Merging #125 into master will decrease coverage by 0.09%.
The diff coverage is 31.05%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #125      +/-   ##
==========================================
- Coverage   44.94%   44.85%   -0.10%     
==========================================
  Files         374      376       +2     
  Lines       18708    18866     +158     
  Branches     8859     8970     +111     
==========================================
+ Hits         8409     8463      +54     
+ Misses       4906     4904       -2     
- Partials     5393     5499     +106     
Impacted Files Coverage Δ
Core/include/Acts/Fitter/GainMatrixSmoother.hpp 17.30% <0.00%> (ø)
Core/include/Acts/Fitter/KalmanFitter.hpp 38.54% <ø> (ø)
Core/include/Acts/Surfaces/ConeSurface.hpp 100.00% <ø> (ø)
Core/include/Acts/Surfaces/CylinderSurface.hpp 100.00% <ø> (ø)
Core/include/Acts/Surfaces/DiscSurface.hpp 100.00% <ø> (ø)
Core/include/Acts/Surfaces/LineSurface.hpp 100.00% <ø> (ø)
Core/include/Acts/Surfaces/PlaneSurface.hpp 100.00% <ø> (ø)
Core/include/Acts/Surfaces/Surface.hpp 50.00% <ø> (ø)
Core/include/Acts/Surfaces/detail/Surface.ipp 71.83% <ø> (ø)
Core/src/Surfaces/detail/AlignmentHelper.cpp 7.69% <7.69%> (ø)
... and 11 more

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 4abc783...b3d5d7f. Read the comment docs.

@XiaocongAi XiaocongAi force-pushed the KF-based-alignment branch 3 times, most recently from c90080a to 0ddad9b Compare May 14, 2020 06:17
@paulgessinger paulgessinger modified the milestones: v0.24.00, v0.25.00 May 18, 2020
@XiaocongAi
Copy link
Contributor Author

@asalzburger @FabianKlimpel , this PR is ready to be reviewed. It's part of the issue (#202) which I'd like to discuss at the workshop. If possible, could you help to start to take a look already these days?

@XiaocongAi XiaocongAi changed the title Kf based alignment Add derivative of track parameters to alignment parameters May 19, 2020
Copy link
Contributor

@FabianKlimpel FabianKlimpel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good beside my comments. One general thing: From all these changes none are tested. Could you add some unit tests for them?

@XiaocongAi XiaocongAi force-pushed the KF-based-alignment branch from e982b81 to d99ba53 Compare May 21, 2020 05:03
@XiaocongAi
Copy link
Contributor Author

@FabianKlimpel , thank you a lot for the review. I have made the necessary changes. One thing I'm not so sure is the calculation of derivative of path length w.r.t. alignment parameters (I think they're something similar to what's done in derivativeFactors). Do those calculations look correct to you?

@XiaocongAi XiaocongAi removed the 🚧 WIP Work-in-progress label May 21, 2020
@FabianKlimpel
Copy link
Contributor

@FabianKlimpel , thank you a lot for the review. I have made the necessary changes. One thing I'm not so sure is the calculation of derivative of path length w.r.t. alignment parameters (I think they're something similar to what's done in derivativeFactors). Do those calculations look correct to you?

When I saw the comments to the 2 calculations I already figured out that you calculated the derivativeFactors. I think these are correct.

@XiaocongAi
Copy link
Contributor Author

When I saw the comments to the 2 calculations I already figured out that you calculated the derivativeFactors. I think these are correct.

Great! Thank you!

@paulgessinger paulgessinger modified the milestones: v0.25.00, v0.26.00 May 23, 2020
@msmk0 msmk0 added the Impact - Major Significant bug and/or affects a lot of modules label Jun 2, 2020
Copy link
Contributor

@msmk0 msmk0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @XiaocongAi for starting this work.

I think we should carefully consider how to add this additional functionality here. By adding the global covariance calculation to the existing Kalman filter, the same algorithm object can potentially have quite different runtime behaviour. One could even argue that adding the global covariance calculation makes this not a Kalman fitter anymore as it removes the defining quality of that algorithm, namely that it only considers neighboring information.

We should strongly consider splitting the functionality into a separate algorithm object, e.g. GlobalCovarianceGainMatrixSmoother and GlobalCovarianceKalmanFilter, to be clear about the different purpose.

It would also be nice to have a write-up that explains how the different Jacobians play together and how they are defined. I am still unsure about the necessity to have the separate path length dependence as it introduces a dependence on the track model into the geometry calculations (or it assumes linear tracks and thereby limits its validity). @XiaocongAi I will contact you later this week to try to put together a write-up.

@XiaocongAi XiaocongAi force-pushed the KF-based-alignment branch from 735d124 to b3d5d7f Compare June 11, 2020 16:29
@XiaocongAi
Copy link
Contributor Author

Hi @msmk0 , I have manually rebased, thus should have no problem now.

@msmk0
Copy link
Contributor

msmk0 commented Jun 11, 2020

Thanks, I will override the coverage check.

@msmk0 msmk0 merged commit 6220f45 into acts-project:master Jun 11, 2020
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Jul 13, 2020
…ect#125)

* Add bone structure for KF-based alignment implementation
* Add calculation of global covariance matrix in KF
* Add option to calcuate global track params covariance
* Add enum for alignment parameters
* Add alignment derivatives engine
* Add derivative of bound parameters w.r.t. reference frame rotation
* Add enum for cartesian coordinate indices
* Move rotation derivatives into separate function
* Add layer and volume alignment to bound parameters derivative
* Change alignment frame to local frame
* Break alignment to bound derivative calculation into separate functions
* Fix the alignmentToPathDerivative return
* Move alignment derivative calculations to surface method
* Add static check of alignment parameters defintion
* Implement local3D to bound 2D derivative with separte method
* Re-implement calculation of alignment to path derivative for LineSurface
* Re-implement the local3D to bound local derivative for DiscSurface
* Re-implement the local3D to bound local derivative for LineSurface
* Re-implement the local3D to bound local derivative for CylinderSurface
* Re-implement the local3D to bound local derivative for ConeSurface
* Make the local3D to bound local derivative method pure virtual
* Add unit test for alignment helper
* Pass pointer for global covariance matrix to smoother
* Not use inline for alignment-related derivatives
* Move function definitions in AlignmentHelper.hpp to cpp
* Add derivative test for plane surface
* Add test for line surface derivative calculation
* Add derivative test for DiscSurface
* Add derivative test for CylinderSurface
* Add derivative test for ConeSurface
* Add more Surface tests
* Implement function to calculate global track parameters covariance
* Remove the option to consider only measurement states
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component - Core Affects the Core module Feature Development to integrate a new feature Impact - Major Significant bug and/or affects a lot of modules
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants