acts icon indicating copy to clipboard operation
acts copied to clipboard

refactor: change the definition of the rotation parameters

Open XiaocongAi opened this issue 1 year ago • 17 comments

This PR includes the following changes:

  1. Change the definition of the rotation parameters to be around local axes
  2. Extend the AlignmentHelper to get derivative of the surface local axes w.r..t rotation of a (mother) composite object

XiaocongAi avatar Apr 08 '23 08:04 XiaocongAi

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.

codecov[bot] avatar Apr 08 '23 08:04 codecov[bot]

📊: Physics performance monitoring for 0a9faf561b895869bd5bf70f429d8371ddd96273

Full contents

physmon summary

github-actions[bot] avatar Apr 08 '23 09:04 github-actions[bot]

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.

pbutti avatar Apr 12 '23 11:04 pbutti

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!

XiaocongAi avatar Apr 12 '23 14:04 XiaocongAi

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.

paulgessinger avatar Apr 12 '23 16:04 paulgessinger

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.

XiaocongAi avatar Apr 13 '23 03:04 XiaocongAi

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.

stale[bot] avatar May 20 '23 14:05 stale[bot]

Still relevant, I think. @XiaocongAi are we still waiting for a way to properly test this change?

gagnonlg avatar May 23 '23 15:05 gagnonlg

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.

pbutti avatar May 23 '23 15:05 pbutti

Where do we stand on this one?

paulgessinger avatar Jun 20 '23 15:06 paulgessinger

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.

paulgessinger avatar Aug 03 '23 15:08 paulgessinger

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)

  1. 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?

  2. 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.

  3. 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?

  4. 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?

pbutti avatar Nov 07 '23 10:11 pbutti

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.

XiaocongAi avatar Nov 08 '23 20:11 XiaocongAi

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.

pbutti avatar Nov 09 '23 09:11 pbutti

how do we proceed here @pbutti @paulgessinger @XiaocongAi ?

andiwand avatar Mar 18 '24 07:03 andiwand

looks like there is a breaking unit test @XiaocongAi @paulgessinger @pbutti

andiwand avatar Mar 27 '24 09:03 andiwand

Hmm. This seems new. It also is not a precision issue I don't think, so might need a fix.

paulgessinger avatar Mar 27 '24 20:03 paulgessinger

Hi @paulgessinger @andiwand @AJPfleger , sorry again for the delay. It should be ready for the merge.

XiaocongAi avatar Jul 25 '24 05:07 XiaocongAi

:red_circle: Athena integration test results [e7d6bf9f1766fdb4345186ed6976c630f3b275a8]

Build job with this PR failed!

Please investigate the build job for the pipeline!

acts-project-service avatar Aug 12 '24 09:08 acts-project-service