fix: Correct the Sign of Drift Distance

[beomki-yeo]

The PR makes the drift distance of line measurement to be postivie always.

[codecov[bot]]

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (6371455) 49.77% compared to head (ff658d1) 49.76%. Report is 181 commits behind head on main.

:exclamation: Current head ff658d1 differs from pull request most recent head eb66084. Consider uploading reports for the commit eb66084 to get more accurate results

Files	Patch %	Lines
Core/src/TrackFitting/GainMatrixUpdater.cpp	0.00%	2 Missing and 7 partials :warning:
Core/include/Acts/TrackFitting/KalmanFitter.hpp	0.00%	0 Missing and 1 partial :warning:
...e/Acts/TrackFitting/detail/KalmanUpdateHelpers.hpp	66.66%	0 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2538      +/-   ##
==========================================
- Coverage   49.77%   49.76%   -0.01%     
==========================================
  Files         466      466              
  Lines       26329    26336       +7     
  Branches    12100    12106       +6     
==========================================
+ Hits        13105    13106       +1     
- Misses       4621     4623       +2     
- Partials     8603     8607       +4     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[XiaocongAi]

[like] Xiaocong Ai reacted to your message:

---

From: Paul Gessinger @.> Sent: Friday, October 13, 2023 8:07:04 AM To: acts-project/acts @.> Cc: Subscribed @.***> Subject: Re: [acts-project/acts] fix: Correct the Sign of Drift Distance (PR #2538)

@paulgessinger commented on this pull request.

I'm wondering if, in order to not introduce the "measurement frame" as a first-class concept, this should be done inside the the KF updater and use a dynamic cast to figure out if it's looking at a LineSurface.

---

In Core/include/Acts/TrackFitting/GlobalChiSquareFitter.hpphttps://github.com/acts-project/acts/pull/2538#discussion_r1357911754:

@@ -58,7 +58,7 @@ struct Gx2FitterExtensions { Delegate<void(const GeometryContext&, const CalibrationContext&, const SourceLink&, TrackStateProxy)>;

• using Updater = Delegate<Result(const GeometryContext&, TrackStateProxy,

• using Updater = Delegate<Result(const GeometryContext&, const Surface*, TrackStateProxy,

Can you not get the surface from the track state?

---

In Core/src/TrackFitting/GainMatrixUpdater.cpphttps://github.com/acts-project/acts/pull/2538#discussion_r1357912886:

@@ -64,8 +64,9 @@ std::tuple<double, std::error_code> GainMatrixUpdater::visitMeasurement( return false; // abort execution }

• auto signCorrected = surface->correctSign(trackState.predicted);

I think you should be able to do

trackState.referenceSurface()

to get the surface of the track state. This way, you don't need to pipe through the surface explicitly.

---

In Core/include/Acts/Surfaces/Surface.hpphttps://github.com/acts-project/acts/pull/2538#discussion_r1357911296:

@@ -389,6 +389,8 @@ class Surface : public virtual GeometryObject, virtual FreeToPathMatrix freeToPathDerivative( const GeometryContext& gctx, const FreeVector& parameters) const;

• virtual BoundVector correctSign(const BoundVector& boundParams) const;

On thing we need to be careful here is that in principle we do not want to have separate coordinate systems between surface and measurement. As we discussed, the sign in the line surface is an edge case.

I think we should really avoid introducing the concept of a more or less generic coordinate transform between surface frame and measurement frame.

— Reply to this email directly, view it on GitHubhttps://github.com/acts-project/acts/pull/2538#pullrequestreview-1675938635, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AHFOG56JDUHSCSZEY5BFU7LX7DZCRAVCNFSM6AAAAAA54LIULCVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMNZVHEZTQNRTGU. You are receiving this because you are subscribed to this thread.Message ID: @.***>

[XiaocongAi]

I'm wondering if, in order to not introduce the "measurement frame" as a first-class concept, this should be done inside the the KF updater and use a dynamic cast to figure out if it's looking at a LineSurface.

I totally agree. This is what we are exactly doing for a drift chamber tracking locally.

[beomki-yeo]

@XiaocongAi Yeah I just saw your CTD slides on CEPC. It's nice that someone is looking into wire measurements...

BTW, the current LlineSurface is only designed for straw tube (circular cross section) so we will need to add a case for drift chamber (sqaure cross section)

Also do we have any test geometry for wire measurements where we can check the physical performance? (Also to @paulgessinger @andiwand )

[acts-project-service]

:red_circle: Athena integration test results [eb66084e5ffdde8bb5d68fa5a71216a5765430d7]

Build job with this PR failed!

Please investigate the build job for the pipeline!

[andiwand]

How should we proceed on this @beomki-yeo @paulgessinger ?

[beomki-yeo]

I have to fix the PR as suggested by Paul. I have been just procrastinating. Do you need this PR merged soon?

[andiwand]

I have to fix the PR as suggested by Paul. I have been just procrastinating. Do you need this PR merged soon?

No hurry just wanted to see what the status is since this PR is getting quite outdated

[beomki-yeo]

I will open it later when I can work on it.