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.

Sign up for GitHub

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

Jump to bottom

fix: Quick fix LineSurface tolerance inconsistency in globalToLocal #2239

Merged
merged 7 commits into from
Jun 28, 2023
Merged

fix: Quick fix LineSurface tolerance inconsistency in globalToLocal #2239

merged 7 commits into from
Jun 28, 2023

Conversation

andiwand
Copy link
Contributor

@andiwand andiwand commented Jun 23, 2023 •
edited

This is meant as a quick fix for the problem I am experiencing in #2086. It seems like it also appeared somewhere else already and maybe in one of the recent issues.

The problem seems to be that the tolerance is not consistent between intersection and globalToLocal. Since these are used in sequence with the same tolerance globalToLocal will fail as it reports a higher distance than the intersection.

A proper fix would be to align the math of the intersection and conversion so the tolerance has the same meaning in both cases.

should also fix #2226

@github-actions github-actions bot added the Component - Core Affects the Core module label Jun 23, 2023
@andiwand andiwand added this to the next milestone Jun 23, 2023
@codecov
Copy link

codecov bot commented Jun 23, 2023 •
edited

Codecov Report

Merging #2239 (b7a536e) into main (91fcbb7) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #2239   +/-   ##
=======================================
  Coverage   49.35%   49.35%           
=======================================
  Files         445      445           
  Lines       25264    25265    +1     
  Branches    11650    11651    +1     
=======================================
+ Hits        12468    12469    +1     
  Misses       4512     4512           
  Partials     8284     8284
Impacted Files Coverage Δ
Core/src/Surfaces/LineSurface.cpp 35.80% <100.00%> (+0.39%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@github-actions
Copy link

github-actions bot commented Jun 23, 2023 •
edited

📊 Physics performance monitoring for b7a536e

Summary
Full report
Seeding: seeded, truth estimated, orthogonal
CKF: seeded, truth smeared, truth estimated, orthogonal
IVF: seeded, truth smeared, truth estimated, orthogonal
AMVF: seeded, truth smeared, truth estimated, orthogonal
Ambiguity resolution: seeded, orthogonal
Truth tracking
Truth tracking (GSF)

Vertexing

Vertexing vs. mu
IVF seeded

IVF truth_smeared

IVF truth_estimated

IVF orthogonal

AMVF seeded

AMVF truth_smeared

AMVF truth_estimated

AMVF orthogonal

Seeding

Seeding seeded

Seeding truth_estimated

Seeding orthogonal

CKF

CKF seeded

CKF truth_smeared

CKF truth_estimated

CKF orthogonal

Ambiguity resolution

seeded

Truth tracking (Kalman Filter)

Truth tracking

Truth tracking (GSF)

Truth tracking

This was referenced Jun 23, 2023
Merged
Closed
@andiwand
Copy link
Contributor Author

andiwand commented Jun 26, 2023 •
edited

logged into this issue again and this is the output I get

intersect distance 0.940883 position  7.57337e-05 -6.11766e-05      44.2963 momentum -0.949711  0.197314  -0.24314
globalToLocal distance 0.000101456 position  7.57337e-05 -6.11766e-05      44.2963 momentum  0.949711 -0.197314   0.24314

you can see that both functions get the same input but conclude to a different distance.

@paulgessinger @asalzburger

@asalzburger
Copy link
Contributor

logged into this issue again and this is the output I get

intersect distance 0.940883 position  7.57337e-05 -6.11766e-05      44.2963 momentum -0.949711  0.197314  -0.24314
globalToLocal distance 0.000101456 position  7.57337e-05 -6.11766e-05      44.2963 momentum  0.949711 -0.197314   0.24314

you can see that both functions get the same input but conclude to a different distance.

@paulgessinger @asalzburger

There is a sign flip in the momentum, this looks like it's the cause.

@andiwand
Copy link
Contributor Author

Nope.

intersect distance 0.940883 position  7.57337e-05 -6.11766e-05      44.2963 momentum -0.949711  0.197314  -0.24314
distance 0.000101456 position  7.57337e-05 -6.11766e-05      44.2963 momentum -0.949711  0.197314  -0.24314

I don't think the direction should make a difference in this case and it seems to not do so.

If I remember correctly there are a bunch of random sign flips in the navigation which might cause this inconsistency. But I don't think this is relevant now.

@paulgessinger @asalzburger

@asalzburger
Copy link
Contributor

Nope.

intersect distance 0.940883 position  7.57337e-05 -6.11766e-05      44.2963 momentum -0.949711  0.197314  -0.24314
distance 0.000101456 position  7.57337e-05 -6.11766e-05      44.2963 momentum -0.949711  0.197314  -0.24314

I don't think the direction should make a difference in this case and it seems to not do so.

If I remember correctly there are a bunch of random sign flips in the navigation which might cause this inconsistency. But I don't think this is relevant now.

@paulgessinger @asalzburger

ok, understood - probably time to have a look at the code lines.

@andiwand
Copy link
Contributor Author

ok, understood - probably time to have a look at the code lines.

I agree but I wont have time for this in the near future. So I would propose to merge this quick fix and reference the underlying problem in an issue + in the code.

@andiwand
Copy link
Contributor Author

linked #2226 in the description

@paulgessinger
Copy link
Member

@andiwand thanks!

@github-actions github-actions bot added the Component - Examples Affects the Examples module label Jun 28, 2023
@andiwand
Copy link
Contributor Author

updated the hashes @paulgessinger

@kodiakhq kodiakhq bot merged commit c8beee7 into acts-project:main Jun 28, 2023
52 checks passed
@andiwand andiwand deleted the quick-fix-linesurface-g2l branch June 28, 2023 11:49
@paulgessinger paulgessinger modified the milestones: next, v27.1.0 Jul 4, 2023
noemina pushed a commit to noemina/acts that referenced this pull request Jul 5, 2023
@andiwand @noemina
fix: Quick fix LineSurface tolerance inconsistency in `globalToLoca…
29a93b8 
…l` (acts-project#2239)

This is meant as a quick fix for the problem I am experiencing in acts-project#2086. It seems like it also appeared somewhere else already and maybe in one of the recent issues.

The problem seems to be that the tolerance is not consistent between `intersection` and `globalToLocal`. Since these are used in sequence with the same tolerance `globalToLocal` will fail as it reports a higher distance than the `intersection`.

A proper fix would be to align the math of the intersection and conversion so the tolerance has the same meaning in both cases.

should also fix acts-project#2226
@andiwand andiwand mentioned this pull request Jul 8, 2023
Merged
kodiakhq bot pushed a commit that referenced this pull request Jul 24, 2023
@andiwand
fix: Improve full chain pulls by enabling Fatras interactions (#2086)
dbcba62 
Our full chain pulls are in a bad state. Looks like the reconstruction and simulation energy loss did not match up. This PR switches the Fatras interactions on which should bring our pulls back to standard normal distribution.

Fixes
- #1643

Blocked by
- #2157
- #2239
- #2295
- #2293
- #2294
paulgessinger pushed a commit to paulgessinger/acts that referenced this pull request Jul 24, 2023
@andiwand @paulgessinger
fix: Improve full chain pulls by enabling Fatras interactions (acts-p…
9e75691 
…roject#2086)

Our full chain pulls are in a bad state. Looks like the reconstruction and simulation energy loss did not match up. This PR switches the Fatras interactions on which should bring our pulls back to standard normal distribution.

Fixes
- acts-project#1643

Blocked by
- acts-project#2157
- acts-project#2239
- acts-project#2295
- acts-project#2293
- acts-project#2294
kodiakhq bot pushed a commit that referenced this pull request Jul 27, 2023
@andiwand
fix: Align globalToLocal and intersection in LineSurface (#2287)
96ead7c 
This is supposed to be a proper fix after the quick fix in #2239

In `globalToLocal` we did not respect the direction which is important for the line surface.

I tried to make the code a little more clear and move away from constructor initialization in the mathy part. I also added a unit test to check the intersection against the propagation.

I will let the CI judge how much this breaks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulgessinger paulgessinger paulgessinger approved these changes

@benjaminhuth benjaminhuth benjaminhuth approved these changes

Assignees
No one assigned
Labels
Component - Core Affects the Core module Component - Examples Affects the Examples module
Projects
None yet
Milestone
v27.1.0
Development

Successfully merging this pull request may close these issues.

Bug: Combinatorial Kalman filter fails during track finding
4 participants
@andiwand @asalzburger @paulgessinger @benjaminhuth