refactor: Use common direction transform Jacobian #2782

andiwand · 2023-12-08T07:50:30Z

I found the same code in a few places and there seems to be a bit of simplification opportunity in the math

codecov · 2023-12-08T08:25:10Z

Codecov Report

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

Comparison is base (c51f48a) 48.87% compared to head (ea6d780) 48.83%.

Files	Patch %	Lines
Core/src/Propagator/detail/CovarianceEngine.cpp	19.04%	5 Missing and 12 partials ⚠️
Core/include/Acts/Utilities/JacobianHelpers.hpp	38.88%	2 Missing and 9 partials ⚠️
Core/src/Surfaces/DiscSurface.cpp	18.18%	4 Missing and 5 partials ⚠️
Core/src/Surfaces/Surface.cpp	0.00%	0 Missing and 4 partials ⚠️
Core/src/Surfaces/LineSurface.cpp	0.00%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2782      +/-   ##
==========================================
- Coverage   48.87%   48.83%   -0.05%     
==========================================
  Files         485      486       +1     
  Lines       28205    28175      -30     
  Branches    13286    13285       -1     
==========================================
- Hits        13786    13760      -26     
+ Misses       4829     4820       -9     
- Partials     9590     9595       +5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2023-12-08T09:08:43Z

📊: Physics performance monitoring for `ea6d780`

Full contents

physmon summary

Core/include/Acts/Utilities/VectorHelpers.hpp

Core/src/Propagator/detail/CovarianceEngine.cpp

Co-authored-by: Alexander J. Pfleger <70842573+AJPfleger@users.noreply.github.com>

Core/include/Acts/Utilities/JacobianHelpers.hpp

Core/src/Propagator/detail/CovarianceEngine.cpp

Core/src/Surfaces/DiscSurface.cpp

andiwand · 2023-12-11T08:17:55Z

After having a closer look at the code it seems like the jacobians are duplicated in the CovarianceEngine. I will close this one and create another PR where I remove the duplicated code by keeping the JacobianEngine

andiwand · 2023-12-11T08:24:50Z

(closed the wrong pr)

acts-project-service · 2023-12-18T10:10:34Z

🔴 Athena integration test results [`7b26be2`]

🔴 Some tests have failed!

Please investigate the pipeline!

status	job	report
🟢	run_unit_tests
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsBenchmarkWithSpot.sh 8 100
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsWorkflow.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateAmbiguityResolution.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateResolvedTracks.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateTracks.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateActsCoreSpacePoints.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateActsSpacePoints.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateSeeds.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateOrthogonalSeeds.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsValidateClusters.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsPersistifyEDM.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsGSFRefitting.sh
🟢	run_ci_tests: ../athena/AtlasTest/CITest/test/ActsKfRefitting.sh
🟢	run_ci_tests: python3 ../athena/Tracking/Acts/ActsGeometry/test/ActsExtrapolationAlgTest.py
🟢	run_ci_tests: python3 ../athena/Tracking/Acts/ActsGeometry/test/ActsITkTest.py
🟢	run_workflow_tests_run4_mc
🟢	run_workflow_tests_run2_mc
🟢	run_workflow_tests_run2_data
🟢	run_workflow_tests_run3_mc
🔴	run_workflow_tests_run3_data
🟢	run_art_test: test_data18_13TeV_1000evt
🟢	run_art_test: test_ttbarPU40_reco

paulgessinger · 2023-12-18T10:40:17Z

This changes 1 vertex in the Athena run 3 workflow test.

andiwand · 2023-12-18T10:42:40Z

that is very unexpected. I will check the diff again. maybe I messed something up

Core/src/Surfaces/LineSurface.cpp

paulgessinger · 2023-12-21T21:24:00Z

I'm ok to update references and merge this. Do you want to go ahead with this now or wait until January?

andiwand · 2023-12-21T21:41:31Z

would be great - I can update the hashes

paulgessinger · 2023-12-25T20:34:12Z

This causes lots of new navigation failures in the Athena workflows, which somehow we did not see in the previous Athena runs.
We probably will have to revert this until we've had time to investigate.

…ect#2782)" This reverts commit 7b26be2.

paulgessinger · 2023-12-31T13:53:56Z

This is not actually causing any issues. I tracked it down to a combination of #2712 and simultaneous changes on the Athena side.

In #2780 I realized that we do not call any of the functions in `JacobianEngine` on the current main branch and most of them are duplicated in `CovarianceEngine`. Here I try to unify the duplicated code again blocked by - #2782 - #2781

…2907) Output changes are not tolerated by Athena and will be patched to v32.

I found the same code in a few places and there seems to be a bit of simplification opportunity in the math

In acts-project#2780 I realized that we do not call any of the functions in `JacobianEngine` on the current main branch and most of them are duplicated in `CovarianceEngine`. Here I try to unify the duplicated code again blocked by - acts-project#2782 - acts-project#2781

…d of bound vector (#2811) This avoid free->bound->free roundtrips and aligns the interface with `freeToBoundJacobian` related issues - #2810 blocked by - #2789 - #2782 - #2781

I noticed that the concept of a curvilinear surface is a quite spread over the codebase and I wanted to improve this. Here I introduce a surface like class which does not actually inherit from `Acts::Surface` but shares some of the functionality. This way the jacobians are put in a single place and can be used from somewhere else in an expressive fashion. Afterwards it might make sense to let the `CurvilinearTrackParameters` depend on this special surface instead of a `PlaneSurface` or to merge them with `BoundTrackParameters` completely. related issues - #2812 blocked by - #2789 - #2782 - #2781 - #2811

I noticed that the concept of a curvilinear surface is a quite spread over the codebase and I wanted to improve this. Here I introduce a surface like class which does not actually inherit from `Acts::Surface` but shares some of the functionality. This way the jacobians are put in a single place and can be used from somewhere else in an expressive fashion. Afterwards it might make sense to let the `CurvilinearTrackParameters` depend on this special surface instead of a `PlaneSurface` or to merge them with `BoundTrackParameters` completely. related issues - acts-project#2812 blocked by - acts-project#2789 - acts-project#2782 - acts-project#2781 - acts-project#2811

…d of bound vector (acts-project#2811) This avoid free->bound->free roundtrips and aligns the interface with `freeToBoundJacobian` related issues - acts-project#2810 blocked by - acts-project#2789 - acts-project#2782 - acts-project#2781

I noticed that the concept of a curvilinear surface is a quite spread over the codebase and I wanted to improve this. Here I introduce a surface like class which does not actually inherit from `Acts::Surface` but shares some of the functionality. This way the jacobians are put in a single place and can be used from somewhere else in an expressive fashion. Afterwards it might make sense to let the `CurvilinearTrackParameters` depend on this special surface instead of a `PlaneSurface` or to merge them with `BoundTrackParameters` completely. related issues - acts-project#2812 blocked by - acts-project#2789 - acts-project#2782 - acts-project#2781 - acts-project#2811

direction transform jacobian

55508bd

andiwand added this to the next milestone Dec 8, 2023

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

fix

c46ad26

AJPfleger requested changes Dec 8, 2023

View reviewed changes

Core/include/Acts/Utilities/VectorHelpers.hpp Show resolved Hide resolved

Core/src/Propagator/detail/CovarianceEngine.cpp Outdated Show resolved Hide resolved

Core/src/Propagator/detail/CovarianceEngine.cpp Show resolved Hide resolved

andiwand and others added 2 commits December 8, 2023 11:08

Update Core/src/Propagator/detail/CovarianceEngine.cpp

e3ee9d8

Co-authored-by: Alexander J. Pfleger <70842573+AJPfleger@users.noreply.github.com>

Update Core/src/Propagator/detail/CovarianceEngine.cpp

bb38a4c

Co-authored-by: Alexander J. Pfleger <70842573+AJPfleger@users.noreply.github.com>

AJPfleger previously approved these changes Dec 8, 2023

View reviewed changes

paulgessinger reviewed Dec 8, 2023

View reviewed changes

Core/include/Acts/Utilities/JacobianHelpers.hpp Show resolved Hide resolved

Core/src/Propagator/detail/CovarianceEngine.cpp Show resolved Hide resolved

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

andiwand closed this Dec 11, 2023

andiwand reopened this Dec 11, 2023

This was referenced Dec 11, 2023

refactor: Reuse JacobianEngine #2789

Merged

refactor!: Refactor Surface::boundToFreeJacobian to use free instead of bound vector #2811

Merged

refactor: Introduce CurvilinearSurface #2814

Merged

Merge branch 'main' into refactor-direction-transform-jacobian

8b9afba

acts-project-service added the Fails Athena tests This PR causes a failure in the Athena tests label Dec 18, 2023

andiwand commented Dec 18, 2023

View reviewed changes

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

Merge branch 'main' into refactor-direction-transform-jacobian

cdd4f96

update hashes

cb32c64

andiwand dismissed AJPfleger’s stale review via cb32c64 December 21, 2023 21:47

github-actions bot added the Component - Examples Affects the Examples module label Dec 21, 2023

github-actions bot added the Changes Performance label Dec 21, 2023

Merge branch 'main' into refactor-direction-transform-jacobian

ea6d780

paulgessinger approved these changes Dec 22, 2023

View reviewed changes

paulgessinger added the automerge label Dec 22, 2023

kodiakhq bot merged commit 7b26be2 into acts-project:main Dec 22, 2023
54 checks passed

github-actions bot removed the automerge label Dec 22, 2023

acts-project-service added the Breaks Athena build This PR breaks the Athena build label Dec 22, 2023

andiwand deleted the refactor-direction-transform-jacobian branch December 22, 2023 11:29

paulgessinger added a commit to paulgessinger/acts that referenced this pull request Dec 25, 2023

revert: "refactor: Use common direction transform Jacobian (acts-proj…

f427173

…ect#2782)" This reverts commit 7b26be2.

paulgessinger mentioned this pull request Dec 25, 2023

revert: "refactor: Use common direction transform Jacobian (#2782)" #2855

Closed

paulgessinger modified the milestones: next, v32.0.0 Jan 19, 2024

paulgessinger added Fails Athena tests This PR causes a failure in the Athena tests and removed Breaks Athena build This PR breaks the Athena build Fails Athena tests This PR causes a failure in the Athena tests labels Jan 23, 2024

andiwand added a commit to andiwand/acts that referenced this pull request Jan 29, 2024

revert acts-project#2782

1aa6a67

andiwand added a commit to andiwand/acts that referenced this pull request Jan 29, 2024

revert acts-project#2782

e2ac0e0

paulgessinger pushed a commit that referenced this pull request Jan 30, 2024

revert: Use common direction transform Jacobian (originally #2782) (#…

daeea07

…2907) Output changes are not tolerated by Athena and will be patched to v32.

LaraCalic pushed a commit to LaraCalic/acts that referenced this pull request Feb 10, 2024

refactor: Use common direction transform Jacobian (acts-project#2782)

8c8266e

I found the same code in a few places and there seems to be a bit of simplification opportunity in the math

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Use common direction transform Jacobian #2782

refactor: Use common direction transform Jacobian #2782

andiwand commented Dec 8, 2023 •

edited

codecov bot commented Dec 8, 2023 •

edited

github-actions bot commented Dec 8, 2023 •

edited

andiwand commented Dec 11, 2023

andiwand commented Dec 11, 2023

acts-project-service commented Dec 18, 2023 •

edited

paulgessinger commented Dec 18, 2023

andiwand commented Dec 18, 2023

paulgessinger commented Dec 21, 2023 •

edited

andiwand commented Dec 21, 2023

paulgessinger commented Dec 25, 2023 •

edited

paulgessinger commented Dec 31, 2023

refactor: Use common direction transform Jacobian #2782

refactor: Use common direction transform Jacobian #2782

Conversation

andiwand commented Dec 8, 2023 • edited

codecov bot commented Dec 8, 2023 • edited

Codecov Report

github-actions bot commented Dec 8, 2023 • edited

📊: Physics performance monitoring for ea6d780

physmon summary

andiwand commented Dec 11, 2023

andiwand commented Dec 11, 2023

acts-project-service commented Dec 18, 2023 • edited

🔴 Athena integration test results [7b26be2]

🔴 Some tests have failed!

paulgessinger commented Dec 18, 2023

andiwand commented Dec 18, 2023

paulgessinger commented Dec 21, 2023 • edited

andiwand commented Dec 21, 2023

paulgessinger commented Dec 25, 2023 • edited

paulgessinger commented Dec 31, 2023

andiwand commented Dec 8, 2023 •

edited

codecov bot commented Dec 8, 2023 •

edited

github-actions bot commented Dec 8, 2023 •

edited

📊: Physics performance monitoring for `ea6d780`

acts-project-service commented Dec 18, 2023 •

edited

🔴 Athena integration test results [`7b26be2`]

paulgessinger commented Dec 21, 2023 •

edited

paulgessinger commented Dec 25, 2023 •

edited