acts
acts copied to clipboard
refactor: change the definition of the rotation parameters
This PR includes the following changes:
- Change the definition of the rotation parameters to be around local axes
- Extend the
AlignmentHelper
to get derivative of the surface local axes w.r..t rotation of a (mother) composite object
Codecov Report
Attention: Patch coverage is 0%
with 23 lines
in your changes missing coverage. Please review.
Project coverage is 49.58%. Comparing base (
e440b62
) to head (e964f11
). Report is 1 commits behind head on main.
:exclamation: Current head e964f11 differs from pull request most recent head 2a64143
Please upload reports for the commit 2a64143 to get more accurate results.
Files | Patch % | Lines |
---|---|---|
Core/src/Surfaces/detail/AlignmentHelper.cpp | 0.00% | 0 Missing and 23 partials :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #2021 +/- ##
==========================================
+ Coverage 47.27% 49.58% +2.31%
==========================================
Files 510 473 -37
Lines 30095 26820 -3275
Branches 14605 12357 -2248
==========================================
- Hits 14226 13300 -926
+ Misses 5377 4752 -625
+ Partials 10492 8768 -1724
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
📊: Physics performance monitoring for 0a9faf561b895869bd5bf70f429d8371ddd96273
physmon summary
- ✅ Particles fatras
- ✅ Particles geant4
- ✅ Particles ttbar
- ✅ Vertices ttbar
- ✅ Truth tracking (KF)
- ✅ Truth tracking (GSF)
- ✅ Truth tracking (GX2F)
- ✅ CKF | trackfinding | single muon | truth smeared seeding
- ✅ Track Summary CKF | trackfinding | single muon | truth smeared seeding
- ✅ Seeding trackfinding | single muon | truth estimated seeding
- ✅ CKF | trackfinding | single muon | truth estimated seeding
- ✅ Track Summary CKF | trackfinding | single muon | truth estimated seeding
- ✅ Seeding trackfinding | single muon | default seeding
- ✅ CKF | trackfinding | single muon | default seeding
- ✅ Track Summary CKF | trackfinding | single muon | default seeding
- ✅ Seeding trackfinding | single muon | orthogonal seeding
- ✅ CKF | trackfinding | single muon | orthogonal seeding
- ✅ Track Summary CKF | trackfinding | single muon | orthogonal seeding
- ✅ Seeding trackfinding | 4 muon x 50 vertices | default seeding
- ✅ CKF | trackfinding | 4 muon x 50 vertices | default seeding
- ✅ Track Summary CKF | trackfinding | 4 muon x 50 vertices | default seeding
- ✅ Ambisolver | trackfinding | 4 muon x 50 vertices | default seeding
- ✅ IVF notime | trackfinding | 4 muon x 50 vertices | default seeding
- ✅ AMVF gauss notime | trackfinding | 4 muon x 50 vertices | default seeding
- ✅ AMVF grid time | trackfinding | 4 muon x 50 vertices | default seeding
- ✅ Seeding trackfinding | ttbar with 200 pileup | default seeding
- ✅ CKF | trackfinding | ttbar with 200 pileup | default seeding
- ✅ Track Summary CKF | trackfinding | ttbar with 200 pileup | default seeding
- ✅ Ambisolver | trackfinding | ttbar with 200 pileup | default seeding
- ✅ AMVF gauss notime | trackfinding | ttbar with 200 pileup | default seeding
- ✅ AMVF grid time | trackfinding | ttbar with 200 pileup | default seeding
Hi Xiacong,
In order to check a little bit more about the derivative of the residuals wrt local rotations could you point me to the relevant files to run some misalignment examples? Additionally, is there a support for unbiased residual monitoring in place?
I will be tied up a little bit until Friday, but I will be free Friday to check this PR in more detail.
Hi Xiacong,
In order to check a little bit more about the derivative of the residuals wrt local rotations could you point me to the relevant files to run some misalignment examples? Additionally, is there a support for unbiased residual monitoring in place?
I will be tied up a little bit until Friday, but I will be free Friday to check this PR in more detail.
Hi @pbutti , I'm not very sure if the available example in ACTS works well. @LaraCalic , is it possible for you to provide the command you used to run the misalignment example so that we could do some test for the changes made in this PR? Thanks!
I don't believe this is currently tested at all.
My plan is to add Python bindings and add it to our test infrastructure. I think it would be nice to also do monitoring as soon as it's feasible.
The Python bindings and the residual monitoring will be very helpful for future development. In the current example, there are two many alignment parameters as all the modules are decorated with misalignment. So maybe one thing to add in the example is configurable alignable detector elements so that we can test the alignment more subtly.
This issue/PR has been automatically marked as stale because it has not had recent activity. The stale label will be removed if any interaction occurs.
Still relevant, I think. @XiaocongAi are we still waiting for a way to properly test this change?
Hi @gagnonlg my bad, I should finish review this code and try to test it. If @LaraCalic could provide the command to run the misalignment example I can help.
Where do we stand on this one?
I've added two comments. The associated unit test is updated, and otherwise I don't think we have a good way to test this at the moment. I'd be in favor of merging this in it's current form and revisiting in the future.
Hi @XiaocongAi
I am taking a deeper look to the alignment implementation and in particular to this request. I would like to start a discussion with some questions to get better understanding of the current implementation and bring this in as I'm starting to taking a look for alignment for LDMX as well.
I would like to start by discussing the implementation of the rotation derivatives in the alignment helper.
I must say I got a bit lost in the logic on how the composite structure is brought in and how the derivatives are derived (pun not intended)
-
Using R * R^T = 1, would expect d/dtheta R = S * R, where S is the skew-symmetric matrix. From line https://github.com/acts-project/acts/blob/e964f118105f8b7226bd8a13d5feee727430302f/Core/src/Surfaces/detail/AlignmentHelper.cpp#L28 You have d/dx R = R*S. What am I missing?
-
Is there a test that shows the C-matrix computed via https://github.com/acts-project/acts/blob/e964f118105f8b7226bd8a13d5feee727430302f/Core/src/Surfaces/detail/AlignmentHelper.cpp#L15 ? from a known structure, i.e. a module composed by 2 back-to-back planar sensors. I suppose this part of the code is computing the d r_s / d r_c, where r_s are the sub-components rotations and r_c are the composite structure rotations.
-
Still in the computation of the rotation derivatives. In the past I followed Stoye's, coded the in CMSSW to obtain the derivatives of the sensor alignment parameters wrt composite object alignment parameters. https://github.com/cms-sw/cmssw/blob/6d2f66057131baacc2fcbdd203588c41c885b42c/Alignment/CommonAlignmentParametrization/src/FrameToFrameDerivative.cc Here seems like you obtain a tensor of 3 3x3 matrices, but I couldn't derive those. In fact, considering the notation you use for Rs = Rc * Rrel, when I compute d/dalpha Rs = d/dalpha (Rc*Rrel) = (d/dalpha Rc) * Rrel, because d/dalpha Rrel = 0 due to the fact that the relative rotation of the sensor wrt the composite structure won't change If I rotate the composite structure, I think. What am I missing?
-
I think we should add explicitly with rotations we are going to pass to this helper, i.e. local-to-global or global-to-local.
Perhaps we could talk directly about the details?
Hi @pbutti , I'm attaching a personal note AlignmentFormula.pdf (I'm planning to transform it to a white paper soon) where I derived all those equations. It will be great if you can have a look. And of course, we can have a chat about this.
Hi @XiaocongAi,
Thank you for the document. I'll send you an email to organize a slot to talk. I would like to also bring in the computation from Stoye / CMS for the FrameToFrame derivatives (as they return a 6x6 matrix with should be easy to use for Millepede interface). The two computations should, in the end, be equivalent.
how do we proceed here @pbutti @paulgessinger @XiaocongAi ?
looks like there is a breaking unit test @XiaocongAi @paulgessinger @pbutti
Hmm. This seems new. It also is not a precision issue I don't think, so might need a fix.
Hi @paulgessinger @andiwand @AJPfleger , sorry again for the delay. It should be ready for the merge.
Quality Gate passed
Issues
0 New issues
0 Accepted issues
Measures
0 Security Hotspots
46.4% Coverage on New Code
0.0% Duplication on New Code