fix: Align `globalToLocal` and `intersection` in `LineSurface` #2287

andiwand · 2023-07-08T09:24:44Z

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

codecov · 2023-07-08T09:57:25Z

Codecov Report

Merging #2287 (7224ca0) into main (1e2eb5d) will increase coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is 19.17%.

❗ Current head 7224ca0 differs from pull request most recent head 5d5c343. Consider uploading reports for the commit 5d5c343 to get more accurate results

@@            Coverage Diff             @@
##             main    #2287      +/-   ##
==========================================
+ Coverage   49.47%   49.49%   +0.01%     
==========================================
  Files         451      451              
  Lines       25484    25469      -15     
  Branches    11720    11710      -10     
==========================================
- Hits        12609    12605       -4     
+ Misses       4582     4581       -1     
+ Partials     8293     8283      -10

Files Changed	Coverage Δ
Core/include/Acts/Surfaces/LineSurface.hpp	`100.00% <ø> (ø)`
Core/src/Surfaces/LineSurface.cpp	`35.37% <19.17%> (-0.43%)`	⬇️

... and 2 files with indirect coverage changes

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

github-actions · 2023-07-08T09:57:59Z

📊 Physics performance monitoring for `5d5c343`

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

Core/src/Surfaces/LineSurface.cpp

Tests/UnitTests/Core/Surfaces/LineSurfaceTests.cpp

andiwand

thanks for the review @felix-russo !

Core/src/Surfaces/LineSurface.cpp

Tests/UnitTests/Core/Surfaces/LineSurfaceTests.cpp

Co-authored-by: felix-russo <72298366+felix-russo@users.noreply.github.com>

Core/include/Acts/Surfaces/LineSurface.hpp

Tests/UnitTests/Core/Surfaces/LineSurfaceTests.cpp

Core/src/Surfaces/LineSurface.cpp

andiwand · 2023-07-25T11:37:05Z

We're ready to go for v28 now. Can you update the PR so we can merge it?

should be ready now @paulgessinger

paulgessinger

Sorry maybe I'm dumb but from the diff it's not at all clear to me what's actually happening here. You're changing both intersect and globalToLocal, and they're not doing the same thing afterwards (and neither did they before I don't think).

What do we want globalToLocal to do (after this)? Check if we're on the line surface, or check if we're on the PCA?

Core/src/Surfaces/LineSurface.cpp

Tests/UnitTests/Core/Surfaces/LineSurfaceTests.cpp

Core/src/Surfaces/LineSurface.cpp

Tests/UnitTests/Core/Surfaces/LineSurfaceTests.cpp

Co-authored-by: Paul Gessinger <hello@paulgessinger.com>

In particular `LineSurface` after acts-project#2287

andiwand · 2023-07-27T08:29:42Z

after #2321 the conflicts should disappear 🤞

kodiakhq · 2023-07-27T08:32:02Z

This PR currently has a merge conflict. Please resolve this and then re-add the automerge label.

acts-project-service · 2023-07-27T17:53:34Z

🔴 Athena integration test results

Build job with this PR failed!

Please investigate the build job for the pipeline!

paulgessinger · 2023-07-28T08:31:22Z

Build failure is likely due to #2269, and not this PR. I'll update this PR once it's resolved.

In particular `LineSurface` after #2287.

andiwand added 3 commits July 7, 2023 17:12

tmp

0b65564

unit test line surface intersection

41629b0

refactor and fix line surface

951a0bb

andiwand added the 🚧 WIP Work-in-progress label Jul 8, 2023

andiwand added this to the next milestone Jul 8, 2023

github-actions bot added the Component - Core Affects the Core module label Jul 8, 2023

fix typo

61f7e95

andiwand marked this pull request as ready for review July 10, 2023 07:23

andiwand removed the 🚧 WIP Work-in-progress label Jul 10, 2023

Merge branch 'main' into fix-line-surface-intersect

465b016

andiwand added 🚧 WIP Work-in-progress and removed 🚧 WIP Work-in-progress labels Jul 10, 2023

Merge branch 'main' into fix-line-surface-intersect

24477fd

felix-russo reviewed Jul 11, 2023

View reviewed changes

andiwand commented Jul 11, 2023

View reviewed changes

andiwand and others added 3 commits July 11, 2023 15:17

Apply suggestions from code review

5b15983

Co-authored-by: felix-russo <72298366+felix-russo@users.noreply.github.com>

pr feedback; refactor code further

e9ef7c5

fix unit test

de953f6

andiwand added the Impact - Minor Nuissance bug and/or affects only a single module label Jul 11, 2023

felix-russo reviewed Jul 11, 2023

View reviewed changes

Core/include/Acts/Surfaces/LineSurface.hpp Show resolved Hide resolved

Tests/UnitTests/Core/Surfaces/LineSurfaceTests.cpp Show resolved Hide resolved

Core/src/Surfaces/LineSurface.cpp Show resolved Hide resolved

asalzburger self-requested a review July 12, 2023 07:27

Merge branch 'main' into fix-line-surface-intersect

c4e3ad9

asalzburger added the automerge label Jul 12, 2023

asalzburger previously approved these changes Jul 12, 2023

View reviewed changes

update references

11e7b0d

andiwand dismissed asalzburger’s stale review via 11e7b0d July 12, 2023 07:36

github-actions bot added Infrastructure Changes to build tools, continous integration, ... Changes Performance labels Jul 12, 2023

github-actions bot added Component - Examples Affects the Examples module Changes Performance labels Jul 25, 2023

paulgessinger reviewed Jul 26, 2023

View reviewed changes

Tests/UnitTests/Core/Surfaces/LineSurfaceTests.cpp Outdated Show resolved Hide resolved

paulgessinger reviewed Jul 26, 2023

View reviewed changes

Tests/UnitTests/Core/Surfaces/LineSurfaceTests.cpp Show resolved Hide resolved

paulgessinger reviewed Jul 26, 2023

View reviewed changes

Tests/UnitTests/Core/Surfaces/LineSurfaceTests.cpp Show resolved Hide resolved

andiwand and others added 5 commits July 26, 2023 14:19

Apply suggestions from code review

391f5d8

Co-authored-by: Paul Gessinger <hello@paulgessinger.com>

new line at end of file

27f33e2

relax auto

f0aa053

improve doc; align tolerance < vs <=

8f18b90

fix tests

d42fae8

paulgessinger added a commit to paulgessinger/acts that referenced this pull request Jul 27, 2023

docs: Improve documentation on surface classes

56e6775

In particular `LineSurface` after acts-project#2287

paulgessinger approved these changes Jul 27, 2023

View reviewed changes

paulgessinger added the automerge label Jul 27, 2023

kodiakhq bot removed the automerge label Jul 27, 2023

paulgessinger mentioned this pull request Jul 27, 2023

docs: Improve documentation on surface classes #2323

Merged

Merge branch 'main' into fix-line-surface-intersect

7224ca0

andiwand added the automerge label Jul 27, 2023

Merge branch 'main' into fix-line-surface-intersect

5d5c343

kodiakhq bot merged commit 96ead7c into acts-project:main Jul 27, 2023
54 checks passed

github-actions bot removed the automerge label Jul 27, 2023

acts-project-service added the Breaks Athena build This PR breaks the Athena build label Jul 27, 2023

andiwand deleted the fix-line-surface-intersect branch July 28, 2023 08:28

kodiakhq bot pushed a commit that referenced this pull request Aug 15, 2023

docs: Improve documentation on surface classes (#2323)

430eb1e

In particular `LineSurface` after #2287.

osbornjd mentioned this pull request Nov 16, 2023

feat: upgrade EICrecon to Acts v30 eic/EICrecon#1072

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Align `globalToLocal` and `intersection` in `LineSurface` #2287

fix: Align `globalToLocal` and `intersection` in `LineSurface` #2287

andiwand commented Jul 8, 2023

codecov bot commented Jul 8, 2023 •

edited

github-actions bot commented Jul 8, 2023 •

edited

andiwand left a comment •

edited

andiwand commented Jul 25, 2023

paulgessinger left a comment

andiwand commented Jul 27, 2023

kodiakhq bot commented Jul 27, 2023

acts-project-service commented Jul 27, 2023 •

edited

paulgessinger commented Jul 28, 2023

fix: Align globalToLocal and intersection in LineSurface #2287

fix: Align globalToLocal and intersection in LineSurface #2287

Conversation

andiwand commented Jul 8, 2023

codecov bot commented Jul 8, 2023 • edited

Codecov Report

github-actions bot commented Jul 8, 2023 • edited

📊 Physics performance monitoring for 5d5c343

Vertexing

Seeding

CKF

Ambiguity resolution

Truth tracking (Kalman Filter)

Truth tracking (GSF)

andiwand left a comment • edited

Choose a reason for hiding this comment

andiwand commented Jul 25, 2023

paulgessinger left a comment

Choose a reason for hiding this comment

andiwand commented Jul 27, 2023

kodiakhq bot commented Jul 27, 2023

acts-project-service commented Jul 27, 2023 • edited

🔴 Athena integration test results

Build job with this PR failed!

paulgessinger commented Jul 28, 2023

fix: Align `globalToLocal` and `intersection` in `LineSurface` #2287

fix: Align `globalToLocal` and `intersection` in `LineSurface` #2287

codecov bot commented Jul 8, 2023 •

edited

github-actions bot commented Jul 8, 2023 •

edited

📊 Physics performance monitoring for `5d5c343`

andiwand left a comment •

edited

acts-project-service commented Jul 27, 2023 •

edited